On Sat, Oct 28, 2017 at 02:13:29AM +0800, Xin Long wrote:
> Now when migrating sock to another one in sctp_sock_migrate(), it only
> resets owner sk for the data in receive queues, not the chunks on out
> queues.
>
> It would cause that data chunks length on the sock is not consistent
> with sk sk_wmem_alloc. When closing the sock or freeing these chunks,
> the old sk would never be freed, and the new sock may crash due to
> the overflow sk_wmem_alloc.
>
> syzbot found this issue with this series:
>
> r0 = socket$inet_sctp()
> sendto$inet(r0)
> listen(r0)
> accept4(r0)
> close(r0)
>
> Although listen() should have returned error when one TCP-style socket
> is in connecting (I may fix this one in another patch), it could also
> be reproduced by peeling off an assoc.
>
> This issue is there since very beginning.
>
> This patch is to reset owner sk for the chunks on out queues so that
> sk sk_wmem_alloc has correct value after accept one sock or peeloff
> an assoc to one sock.
>
> Note that when resetting owner sk for chunks on outqueue, it has to
> sctp_clear_owner_w/skb_orphan chunks before changing assoc->base.sk
> first and then sctp_set_owner_w them after changing assoc->base.sk,
> due to that sctp_wfree and it's callees are using assoc->base.sk.
>
> Reported-by: Dmitry Vyukov <dvyu...@google.com>
> Signed-off-by: Xin Long <lucien....@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> ---
> net/sctp/socket.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 17841ab..6f45d17 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -170,6 +170,36 @@ static inline void sctp_set_owner_w(struct sctp_chunk
> *chunk)
> sk_mem_charge(sk, chunk->skb->truesize);
> }
>
> +static void sctp_clear_owner_w(struct sctp_chunk *chunk)
> +{
> + skb_orphan(chunk->skb);
> +}
> +
> +static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
> + void (*cb)(struct sctp_chunk *))
> +
> +{
> + struct sctp_outq *q = &asoc->outqueue;
> + struct sctp_transport *t;
> + struct sctp_chunk *chunk;
> +
> + list_for_each_entry(t, &asoc->peer.transport_addr_list, transports)
> + list_for_each_entry(chunk, &t->transmitted, transmitted_list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, &q->retransmit, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, &q->sacked, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, &q->abandoned, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, &q->out_chunk_list, list)
> + cb(chunk);
> +}
> +
> /* Verify that this is a valid address. */
> static inline int sctp_verify_addr(struct sock *sk, union sctp_addr *addr,
> int len)
> @@ -8212,7 +8242,9 @@ static void sctp_sock_migrate(struct sock *oldsk,
> struct sock *newsk,
> * paths won't try to lock it and then oldsk.
> */
> lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> + sctp_for_each_tx_datachunk(assoc, sctp_clear_owner_w);
> sctp_assoc_migrate(assoc, newsk);
> + sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w);
>
> /* If the association on the newsk is already closed before accept()
> * is called, set RCV_SHUTDOWN flag.
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
