[PATCH net v2] sctp: re-point retained control chunks on association migration

[PATCH net v2] sctp: re-point retained control chunks on association migration

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

sctp_control_set_owner_w() records the owning socket in a control chunk's
skb->sk.  sctp_sock_migrate() re-owns the association's DATA chunks via
sctp_for_each_tx_datachunk(), but that walk keys off chunk->msg and so
skips control chunks: any control chunk the association still holds (for
example the saved stream-reset request asoc->strreset_chunk, the ASCONF
request/ack lists, or asoc->addip_last_asconf) keeps pointing at the old
socket after the association is moved to the new one.

Once the old socket is freed, a later retransmit reaches
sctp_packet_transmit() -> skb_set_owner_w(head, chunk->skb->sk) and
operates on the freed socket -- refcount_add() on its sk_wmem_alloc,
then sk->sk_write_space() from sock_wfree() -- a use-after-free of
struct sock.

Rename sctp_for_each_tx_datachunk() to sctp_for_each_tx_chunk() and walk
the control chunks the association retains there as well, so migration
re-owns them with the same clear/set bracketing already used for DATA
chunks.  sctp_set_owner_w_migrate() picks the right owner helper by
testing chunk->msg, which is NULL for control chunks.

The per-chunk owner test that traverse_and_process() already applies is
split out into sctp_process_tx_chunk() and reused for the control lists.
A chunk can sit on two of them at once -- asoc->strreset_chunk and
asoc->addip_last_asconf both stay queued on outqueue.control_chunk_list
until they are flushed -- and the test keeps such a chunk from being
cleared or re-owned twice, which would otherwise leak an shkey reference.

sctp_control_set_owner_w() re-reads chunk->shkey from asoc->shkey, so
sctp_set_owner_w_migrate() releases the reference sctp_clear_owner_w()
took by value instead of re-reading chunk->shkey, which would drop the
wrong key if the active key changed while the chunk was queued.

Fixes: d04adf1b3551 ("sctp: reset owner sk for data chunks on out queues when migrating a sock")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
This is based on David Lee's

  [PATCH] sctp: hold shkey across socket migration
  https://lore.kernel.org/netdev/20260731120558.558957-1-david.lee@trailofbits.com/

which adds sctp_set_owner_w_migrate()

v2:
 - Rename sctp_for_each_tx_datachunk() to sctp_for_each_tx_chunk() and
   move the control-chunk traversal into it, rather than adding a
   separate sctp_for_each_tx_ctrlchunk() helper (Xin Long).
 - Handle control chunks in sctp_set_owner_w_migrate() by testing
   chunk->msg, dropping the sctp_ctrl_set_owner_w() helper (Xin Long).
   Control chunks now go through the full clear/set bracketing instead of
   a bare skb->sk store, so sctp_control_set_owner_w() is no longer
   static.
 - Factor the existing owner test out of traverse_and_process() into
   sctp_process_tx_chunk() so the control lists get it too.

v1: https://lore.kernel.org/netdev/20260730090537.27629-1-juny24602@gmail.com/

 include/net/sctp/sm.h    |  1 +
 net/sctp/sm_make_chunk.c |  2 +-
 net/sctp/socket.c        | 53 ++++++++++++++++++++++++++++++----------
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 3bfd261a53cc..76605d1ee839 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -252,6 +252,7 @@ struct sctp_chunk *sctp_make_fwdtsn(const struct sctp_association *asoc,
 				    struct sctp_fwdtsn_skip *skiplist);
 struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc,
 				  __u16 key_id);
+void sctp_control_set_owner_w(struct sctp_chunk *chunk);
 struct sctp_chunk *sctp_make_strreset_req(const struct sctp_association *asoc,
 					  __u16 stream_num, __be16 *stream_list,
 					  bool out, bool in);
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 0ae30c3c8913..7684686798cf 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -94,7 +94,7 @@ static void sctp_control_release_owner(struct sk_buff *skb)
 	}
 }
 
-static void sctp_control_set_owner_w(struct sctp_chunk *chunk)
+void sctp_control_set_owner_w(struct sctp_chunk *chunk)
 {
 	struct sctp_association *asoc = chunk->asoc;
 	struct sk_buff *skb = chunk->skb;
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 4a08023d52aa..d09b9f139070 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -155,9 +155,24 @@ static void sctp_clear_owner_w(struct sctp_chunk *chunk)
 
 static void sctp_set_owner_w_migrate(struct sctp_chunk *chunk)
 {
-	sctp_set_owner_w(chunk);
-	if (chunk->shkey)
-		sctp_auth_shkey_release(chunk->shkey);
+	struct sctp_shared_key *shkey = chunk->shkey;
+
+	if (chunk->msg)
+		sctp_set_owner_w(chunk);
+	else
+		sctp_control_set_owner_w(chunk);
+
+	if (shkey)
+		sctp_auth_shkey_release(shkey);
+}
+
+static void sctp_process_tx_chunk(struct sctp_association *asoc,
+				  struct sctp_chunk *chunk, bool clear,
+				  void (*cb)(struct sctp_chunk *))
+{
+	if ((clear && asoc->base.sk == chunk->skb->sk) ||
+	    (!clear && asoc->base.sk != chunk->skb->sk))
+		cb(chunk);
 }
 
 #define traverse_and_process()	\
@@ -165,17 +180,14 @@ do {				\
 	msg = chunk->msg;	\
 	if (msg == prev_msg)	\
 		continue;	\
-	list_for_each_entry(c, &msg->chunks, frag_list) {	\
-		if ((clear && asoc->base.sk == c->skb->sk) ||	\
-		    (!clear && asoc->base.sk != c->skb->sk))	\
-			cb(c);	\
-	}			\
+	list_for_each_entry(c, &msg->chunks, frag_list)	\
+		sctp_process_tx_chunk(asoc, c, clear, cb);	\
 	prev_msg = msg;		\
 } while (0)
 
-static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
-				       bool clear,
-				       void (*cb)(struct sctp_chunk *))
+static void sctp_for_each_tx_chunk(struct sctp_association *asoc,
+				   bool clear,
+				   void (*cb)(struct sctp_chunk *))
 
 {
 	struct sctp_datamsg *msg, *prev_msg = NULL;
@@ -198,6 +210,21 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
 
 	list_for_each_entry(chunk, &q->out_chunk_list, list)
 		traverse_and_process();
+
+	list_for_each_entry(chunk, &q->control_chunk_list, list)
+		sctp_process_tx_chunk(asoc, chunk, clear, cb);
+
+	list_for_each_entry(chunk, &asoc->asconf_ack_list, transmitted_list)
+		sctp_process_tx_chunk(asoc, chunk, clear, cb);
+
+	list_for_each_entry(chunk, &asoc->addip_chunk_list, list)
+		sctp_process_tx_chunk(asoc, chunk, clear, cb);
+
+	if (asoc->strreset_chunk)
+		sctp_process_tx_chunk(asoc, asoc->strreset_chunk, clear, cb);
+
+	if (asoc->addip_last_asconf)
+		sctp_process_tx_chunk(asoc, asoc->addip_last_asconf, clear, cb);
 }
 
 static void sctp_for_each_rx_skb(struct sctp_association *asoc, struct sock *sk,
@@ -9640,9 +9667,9 @@ static int 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, true, sctp_clear_owner_w);
+	sctp_for_each_tx_chunk(assoc, true, sctp_clear_owner_w);
 	sctp_assoc_migrate(assoc, newsk);
-	sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w_migrate);
+	sctp_for_each_tx_chunk(assoc, false, sctp_set_owner_w_migrate);
 
 	/* If the association on the newsk is already closed before accept()
 	 * is called, set RCV_SHUTDOWN flag.
-- 
2.55.0

Re: [PATCH net v2] sctp: re-point retained control chunks on association migration

[Xin Long]

On Tue, Aug 4, 2026 at 7:37 AM Jun Yang <juny24602@gmail.com> wrote:
>
> From: Jun Yang <junvyyang@tencent.com>
>
> sctp_control_set_owner_w() records the owning socket in a control chunk's
> skb->sk.  sctp_sock_migrate() re-owns the association's DATA chunks via
> sctp_for_each_tx_datachunk(), but that walk keys off chunk->msg and so
> skips control chunks: any control chunk the association still holds (for
> example the saved stream-reset request asoc->strreset_chunk, the ASCONF
> request/ack lists, or asoc->addip_last_asconf) keeps pointing at the old
> socket after the association is moved to the new one.
>
> Once the old socket is freed, a later retransmit reaches
> sctp_packet_transmit() -> skb_set_owner_w(head, chunk->skb->sk) and
> operates on the freed socket -- refcount_add() on its sk_wmem_alloc,
> then sk->sk_write_space() from sock_wfree() -- a use-after-free of
> struct sock.
>
> Rename sctp_for_each_tx_datachunk() to sctp_for_each_tx_chunk() and walk
> the control chunks the association retains there as well, so migration
> re-owns them with the same clear/set bracketing already used for DATA
> chunks.  sctp_set_owner_w_migrate() picks the right owner helper by
> testing chunk->msg, which is NULL for control chunks.
>
> The per-chunk owner test that traverse_and_process() already applies is
> split out into sctp_process_tx_chunk() and reused for the control lists.
> A chunk can sit on two of them at once -- asoc->strreset_chunk and
> asoc->addip_last_asconf both stay queued on outqueue.control_chunk_list
> until they are flushed -- and the test keeps such a chunk from being
> cleared or re-owned twice, which would otherwise leak an shkey reference.
>
> sctp_control_set_owner_w() re-reads chunk->shkey from asoc->shkey, so
> sctp_set_owner_w_migrate() releases the reference sctp_clear_owner_w()
> took by value instead of re-reading chunk->shkey, which would drop the
> wrong key if the active key changed while the chunk was queued.
>
> Fixes: d04adf1b3551 ("sctp: reset owner sk for data chunks on out queues when migrating a sock")
> Cc: stable@kernel.org
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Assisted-by: tencentos-corvus-ai:kimi-k3
> Signed-off-by: Jun Yang <junvyyang@tencent.com>
> ---
> This is based on David Lee's
>
>   [PATCH] sctp: hold shkey across socket migration
>   https://lore.kernel.org/netdev/20260731120558.558957-1-david.lee@trailofbits.com/
>
> which adds sctp_set_owner_w_migrate()
>
> v2:
>  - Rename sctp_for_each_tx_datachunk() to sctp_for_each_tx_chunk() and
>    move the control-chunk traversal into it, rather than adding a
>    separate sctp_for_each_tx_ctrlchunk() helper (Xin Long).
>  - Handle control chunks in sctp_set_owner_w_migrate() by testing
>    chunk->msg, dropping the sctp_ctrl_set_owner_w() helper (Xin Long).
>    Control chunks now go through the full clear/set bracketing instead of
>    a bare skb->sk store, so sctp_control_set_owner_w() is no longer
>    static.
>  - Factor the existing owner test out of traverse_and_process() into
>    sctp_process_tx_chunk() so the control lists get it too.
>
> v1: https://lore.kernel.org/netdev/20260730090537.27629-1-juny24602@gmail.com/
>
>  include/net/sctp/sm.h    |  1 +
>  net/sctp/sm_make_chunk.c |  2 +-
>  net/sctp/socket.c        | 53 ++++++++++++++++++++++++++++++----------
>  3 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 3bfd261a53cc..76605d1ee839 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -252,6 +252,7 @@ struct sctp_chunk *sctp_make_fwdtsn(const struct sctp_association *asoc,
>                                     struct sctp_fwdtsn_skip *skiplist);
>  struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc,
>                                   __u16 key_id);
> +void sctp_control_set_owner_w(struct sctp_chunk *chunk);
>  struct sctp_chunk *sctp_make_strreset_req(const struct sctp_association *asoc,
>                                           __u16 stream_num, __be16 *stream_list,
>                                           bool out, bool in);
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 0ae30c3c8913..7684686798cf 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -94,7 +94,7 @@ static void sctp_control_release_owner(struct sk_buff *skb)
>         }
>  }
>
> -static void sctp_control_set_owner_w(struct sctp_chunk *chunk)
> +void sctp_control_set_owner_w(struct sctp_chunk *chunk)
>  {
>         struct sctp_association *asoc = chunk->asoc;
>         struct sk_buff *skb = chunk->skb;
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 4a08023d52aa..d09b9f139070 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -155,9 +155,24 @@ static void sctp_clear_owner_w(struct sctp_chunk *chunk)
>
>  static void sctp_set_owner_w_migrate(struct sctp_chunk *chunk)
>  {
> -       sctp_set_owner_w(chunk);
> -       if (chunk->shkey)
> -               sctp_auth_shkey_release(chunk->shkey);
> +       struct sctp_shared_key *shkey = chunk->shkey;
> +
> +       if (chunk->msg)
> +               sctp_set_owner_w(chunk);
> +       else
> +               sctp_control_set_owner_w(chunk);
> +
> +       if (shkey)
> +               sctp_auth_shkey_release(shkey);
> +}
> +
> +static void sctp_process_tx_chunk(struct sctp_association *asoc,
> +                                 struct sctp_chunk *chunk, bool clear,
> +                                 void (*cb)(struct sctp_chunk *))
> +{
> +       if ((clear && asoc->base.sk == chunk->skb->sk) ||
> +           (!clear && asoc->base.sk != chunk->skb->sk))
> +               cb(chunk);
>  }
>
>  #define traverse_and_process() \
> @@ -165,17 +180,14 @@ do {                              \
>         msg = chunk->msg;       \
>         if (msg == prev_msg)    \
>                 continue;       \
> -       list_for_each_entry(c, &msg->chunks, frag_list) {       \
> -               if ((clear && asoc->base.sk == c->skb->sk) ||   \
> -                   (!clear && asoc->base.sk != c->skb->sk))    \
> -                       cb(c);  \
> -       }                       \
> +       list_for_each_entry(c, &msg->chunks, frag_list) \
> +               sctp_process_tx_chunk(asoc, c, clear, cb);      \
>         prev_msg = msg;         \
>  } while (0)
>
> -static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
> -                                      bool clear,
> -                                      void (*cb)(struct sctp_chunk *))
> +static void sctp_for_each_tx_chunk(struct sctp_association *asoc,
> +                                  bool clear,
> +                                  void (*cb)(struct sctp_chunk *))
>
>  {
>         struct sctp_datamsg *msg, *prev_msg = NULL;
> @@ -198,6 +210,21 @@ static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
>
>         list_for_each_entry(chunk, &q->out_chunk_list, list)
>                 traverse_and_process();
> +
> +       list_for_each_entry(chunk, &q->control_chunk_list, list)
> +               sctp_process_tx_chunk(asoc, chunk, clear, cb);
> +
> +       list_for_each_entry(chunk, &asoc->asconf_ack_list, transmitted_list)
> +               sctp_process_tx_chunk(asoc, chunk, clear, cb);
> +
> +       list_for_each_entry(chunk, &asoc->addip_chunk_list, list)
> +               sctp_process_tx_chunk(asoc, chunk, clear, cb);
> +
> +       if (asoc->strreset_chunk)
> +               sctp_process_tx_chunk(asoc, asoc->strreset_chunk, clear, cb);
> +
> +       if (asoc->addip_last_asconf)
> +               sctp_process_tx_chunk(asoc, asoc->addip_last_asconf, clear, cb);
>  }
You can just use cb(...) here, no need to extract sctp_process_tx_chunk().

These checks of clear and asoc->base.sk were introduced by:

5c3e82fe1596 ("sctp: fix refcount bug in sctp_wfree")

The issue only existed on DATA chunks.

Also, please hold this patch until upstream applies the dependence:

https://lore.kernel.org/netdev/20260731120558.558957-1-david.lee@trailofbits.com/

Otherwise, your patch will fail on these sashiko reviews as "Failed To Apply".

Thanks.

>
>  static void sctp_for_each_rx_skb(struct sctp_association *asoc, struct sock *sk,
> @@ -9640,9 +9667,9 @@ static int 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, true, sctp_clear_owner_w);
> +       sctp_for_each_tx_chunk(assoc, true, sctp_clear_owner_w);
>         sctp_assoc_migrate(assoc, newsk);
> -       sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w_migrate);
> +       sctp_for_each_tx_chunk(assoc, false, sctp_set_owner_w_migrate);
>
>         /* If the association on the newsk is already closed before accept()
>          * is called, set RCV_SHUTDOWN flag.
> --
> 2.55.0
>