Re: [PATCH net] sctp: reset owner sk for data chunks on out queues when migrating a sock

[David Miller]

From: Xin Long 
Date: Sat, 28 Oct 2017 02:13:29 +0800

> 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 
> Signed-off-by: Xin Long 

Applied and queued up for -stable, thank you.

Re: [PATCH net] sctp: reset owner sk for data chunks on out queues when migrating a sock

[Marcelo Ricardo Leitner]

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 
> Signed-off-by: Xin Long 

Acked-by: Marcelo Ricardo Leitner 

> ---
>  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 = >outqueue;
> + struct sctp_transport *t;
> + struct sctp_chunk *chunk;
> +
> + list_for_each_entry(t, >peer.transport_addr_list, transports)
> + list_for_each_entry(chunk, >transmitted, transmitted_list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, >retransmit, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, >sacked, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, >abandoned, list)
> + cb(chunk);
> +
> + list_for_each_entry(chunk, >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
> 

[PATCH net] sctp: reset owner sk for data chunks on out queues when migrating a sock

[Xin Long]

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 
Signed-off-by: Xin Long 
---
 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 = >outqueue;
+   struct sctp_transport *t;
+   struct sctp_chunk *chunk;
+
+   list_for_each_entry(t, >peer.transport_addr_list, transports)
+   list_for_each_entry(chunk, >transmitted, transmitted_list)
+   cb(chunk);
+
+   list_for_each_entry(chunk, >retransmit, list)
+   cb(chunk);
+
+   list_for_each_entry(chunk, >sacked, list)
+   cb(chunk);
+
+   list_for_each_entry(chunk, >abandoned, list)
+   cb(chunk);
+
+   list_for_each_entry(chunk, >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