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

From: Jun Yang <[email protected]>

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: [email protected]
Reported-by: TencentOS Corvus AI <[email protected]>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <[email protected]>
---
This is based on David Lee's

  [PATCH] sctp: hold shkey across socket migration
  https://lore.kernel.org/netdev/[email protected]/

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/[email protected]/

 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