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

On Tue, Aug 4, 2026 at 7:37 AM Jun Yang <[email protected]> wrote:
>
> 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);
>  }
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/[email protected]/

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
>