[PATCH net v3 0/2] sctp: re-point retained control chunks on association migration

[PATCH net v3 0/2] sctp: re-point retained control chunks on association migration

[Jun Yang]

Patch 1 includes David Lee's shared-key fix [1] without code changes, so
the control-chunk fix in patch 2 can be applied with its prerequisite.

Since v2 [2], patch 2 calls cb() directly for control chunks, as Xin
suggested, and keeps the existing DATA checks.

[1] https://lore.kernel.org/netdev/20260731120558.558957-1-david.lee@trailofbits.com/
[2] https://lore.kernel.org/netdev/20260804113705.45754-1-juny24602@gmail.com/

David Lee (1):
  sctp: hold shkey across socket migration

Jun Yang (1):
  sctp: re-point retained control chunks on association migration

 include/net/sctp/sm.h    |  1 +
 net/sctp/sm_make_chunk.c |  2 +-
 net/sctp/socket.c        | 41 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 38 insertions(+), 6 deletions(-)

base-commit: a644f09b2090ad22a13fbcf9d141084f573108ef

[PATCH net v3 1/2] sctp: hold shkey across socket migration

[Jun Yang]

From: David Lee <david.lee@trailofbits.com>

sctp_sock_migrate() transfers queued DATA skbs from the old socket to the
new one. skb_orphan() invokes sctp_wfree() during that transfer and drops
the skb-owned shared-key reference.

If userspace has removed that key from the association, this can be the
final reference. The following sctp_set_owner_w() then dereferences the
freed chunk->shkey while trying to take the new owner reference.

Take a temporary shared-key reference before orphaning the skb and release
it after the new owner has taken its reference. This preserves the selected
authentication key throughout the ownership transfer.

Bug found and triaged by OpenAI Security Research and
validated by Trail of Bits.

Fixes: 1b1e0bc99474 ("sctp: add refcnt support for sh_key")
Assisted-by: Codex:gpt-5.6-sol gpt-5.5-cyber
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Acked-by: Xin Long <lucien.xin@gmail.com>
---
Original submission:
https://lore.kernel.org/netdev/20260731120558.558957-1-david.lee@trailofbits.com/

 net/sctp/socket.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index c7b9e325ec1c..4a08023d52aa 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -147,9 +147,19 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk)
 
 static void sctp_clear_owner_w(struct sctp_chunk *chunk)
 {
+	/* Keep the shkey alive until the new owner takes its reference. */
+	if (chunk->shkey)
+		sctp_auth_shkey_hold(chunk->shkey);
 	skb_orphan(chunk->skb);
 }
 
+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);
+}
+
 #define traverse_and_process()	\
 do {				\
 	msg = chunk->msg;	\
@@ -9632,7 +9642,7 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
 	lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
 	sctp_for_each_tx_datachunk(assoc, true, sctp_clear_owner_w);
 	sctp_assoc_migrate(assoc, newsk);
-	sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w);
+	sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w_migrate);
 
 	/* If the association on the newsk is already closed before accept()
 	 * is called, set RCV_SHUTDOWN flag.

Re: [PATCH net v3 1/2] sctp: hold shkey across socket migration

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit author, David Lee <david.lee@trailofbits.com>, has no
  Signed-off-by line.

--

sctp: hold shkey across socket migration

sctp_sock_migrate() orphans queued tx chunks before moving the association
to the new socket. skb_orphan() calls sctp_wfree(), which can drop the last
reference on chunk->shkey before sctp_set_owner_w() uses it again. This
patch takes a temporary shkey reference in sctp_clear_owner_w() and drops
it in a new sctp_set_owner_w_migrate() helper, after the new owner has
taken its own reference.

> Fixes: 1b1e0bc99474 ("sctp: add refcnt support for sh_key")
> Assisted-by: Codex:gpt-5.6-sol gpt-5.5-cyber
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> Acked-by: Xin Long <lucien.xin@gmail.com>

[Severity: Low]
This isn't a bug, but is there a Signed-off-by missing for the patch
author?

The Author: line is David Lee <david.lee@trailofbits.com>. The only
Signed-off-by is from Kyle Zeng <kylebot@openai.com>, and there is no
Co-developed-by tag.

The original submission for this patch seems to have come from
david.lee@trailofbits.com. That means the submitter has not signed off
either.

Documentation/process/submitting-patches.rst requires a DCO sign-off
from the author. checkpatch.pl reports this case as "Missing
Signed-off-by: line by nominal patch author".

Which of these is the intended attribution?

  - Kyle Zeng is the author. The patch would then need a From: Kyle Zeng
    line and a sign-off from the submitter.

  - David Lee is the author. He would then need to add his own
    Signed-off-by, plus a Co-developed-by for Kyle Zeng if he co-wrote
    the patch.

The next patch in the series, "sctp: re-point retained control chunks
on association migration", has the same trailers.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926100359.78731-1-juny24602%40gmail.com

Re: [PATCH net v3 1/2] sctp: hold shkey across socket migration

[Xin Long]

On Tue, Sep 29, 2026 at 8:06 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit author, David Lee <david.lee@trailofbits.com>, has no
>   Signed-off-by line.
>
> --
>
> sctp: hold shkey across socket migration
>
> sctp_sock_migrate() orphans queued tx chunks before moving the association
> to the new socket. skb_orphan() calls sctp_wfree(), which can drop the last
> reference on chunk->shkey before sctp_set_owner_w() uses it again. This
> patch takes a temporary shkey reference in sctp_clear_owner_w() and drops
> it in a new sctp_set_owner_w_migrate() helper, after the new owner has
> taken its own reference.
>
> > Fixes: 1b1e0bc99474 ("sctp: add refcnt support for sh_key")
> > Assisted-by: Codex:gpt-5.6-sol gpt-5.5-cyber
> > Signed-off-by: Kyle Zeng <kylebot@openai.com>
> > Acked-by: Xin Long <lucien.xin@gmail.com>
>
> [Severity: Low]
> This isn't a bug, but is there a Signed-off-by missing for the patch
> author?
>
> The Author: line is David Lee <david.lee@trailofbits.com>. The only
> Signed-off-by is from Kyle Zeng <kylebot@openai.com>, and there is no
> Co-developed-by tag.
>
> The original submission for this patch seems to have come from
> david.lee@trailofbits.com. That means the submitter has not signed off
> either.
>
> Documentation/process/submitting-patches.rst requires a DCO sign-off
> from the author. checkpatch.pl reports this case as "Missing
> Signed-off-by: line by nominal patch author".
>
> Which of these is the intended attribution?
>
>   - Kyle Zeng is the author. The patch would then need a From: Kyle Zeng
>     line and a sign-off from the submitter.
>
>   - David Lee is the author. He would then need to add his own
>     Signed-off-by, plus a Co-developed-by for Kyle Zeng if he co-wrote
>     the patch.
>
> The next patch in the series, "sctp: re-point retained control chunks
> on association migration", has the same trailers.
>
Please add Signed-off-by for the author as well:

Signed-off-by: David Lee <david.lee@trailofbits.com>

Thanks.

[PATCH net v3 2/2] sctp: re-point retained control chunks on association migration

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

sctp_sock_migrate() transfers DATA chunk ownership but leaves retained
control chunks pointing at the old socket. A later retransmission can
therefore access the socket after it has been freed.

Extend the migration walk to cover retained control chunks and use
sctp_control_set_owner_w() to assign their new owner.

Save the old chunk->shkey before setting the new owner and release that
key afterward, since sctp_control_set_owner_w() may select a different
asoc->shkey.

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:hy4-preview
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

v3:
 - Call cb() directly for control chunks.
 - Include the shared-key prerequisite as patch 1/2.

v2: https://lore.kernel.org/netdev/20260804113705.45754-1-juny24602@gmail.com/
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        | 37 +++++++++++++++++++++++++++++--------
 3 files changed, 31 insertions(+), 9 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 84a4c97d0f75..b2eb7568a58e 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..efee9e3e671e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -155,9 +155,15 @@ 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);
 }
 
 #define traverse_and_process()	\
@@ -173,9 +179,9 @@ do {				\
 	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 +204,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)
+		cb(chunk);
+
+	list_for_each_entry(chunk, &asoc->asconf_ack_list, transmitted_list)
+		cb(chunk);
+
+	list_for_each_entry(chunk, &asoc->addip_chunk_list, list)
+		cb(chunk);
+
+	if (asoc->strreset_chunk)
+		cb(asoc->strreset_chunk);
+
+	if (asoc->addip_last_asconf)
+		cb(asoc->addip_last_asconf);
 }
 
 static void sctp_for_each_rx_skb(struct sctp_association *asoc, struct sock *sk,
@@ -9640,9 +9661,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.

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In net/sctp/socket.c, sctp_for_each_tx_chunk() walks the control
  chunks with a bare cb(chunk) on q->control_chunk_list,…
- [Medium] sctp_set_owner_w_migrate() passes control chunks to
  sctp_control_set_owner_w().

--

sctp: re-point retained control chunks on association migration

This patch extends the tx walk in sctp_sock_migrate() to cover retained
control chunks as well: control_chunk_list, asconf_ack_list,
addip_chunk_list, strreset_chunk and addip_last_asconf. Their owner is
moved to the new socket. It exports sctp_control_set_owner_w() for chunks
that have no datamsg, and saves the old chunk->shkey so that key can be
released after the new owner is set.

> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 4a08023d52aae..efee9e3e671ef 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -155,9 +155,15 @@ 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);
>  }

[Severity: Medium]

For control chunks with chunk->auth set, sctp_control_set_owner_w() always
does this:

	if (chunk->auth) {
		chunk->shkey = asoc->shkey;
		sctp_auth_shkey_hold(chunk->shkey);
	}

After peeloff/accept, a retained control chunk (an ASCONF-ACK,
addip_last_asconf or strreset_chunk) that was built under key K is
switched to whatever key is active at migration time. Is that intended?

The DATA path through sctp_set_owner_w() keeps the key that was already
selected. The prerequisite shkey patch also describes the transfer as
keeping the selected authentication key.

The commit message says:

  "since sctp_control_set_owner_w() may select a different asoc->shkey."

It doesn't mention that retransmissions of these chunks would then be
authenticated with a different key id.

Can this also lose the SCTP_AUTH_FREE_KEY event for K? Suppose K was
deactivated while chunk C still holds it, so K's refcnt is 2. K cannot
be the active key at that point, per sctp_auth_deact_key_id():

clear pass:
  sctp_clear_owner_w()
    sctp_auth_shkey_hold(K)            K refcnt 3
    skb_orphan()
      sctp_control_release_owner()     sees refcnt 3, no event
        sctp_auth_shkey_release(K)     K refcnt 2

set pass:
  sctp_set_owner_w_migrate()
    sctp_control_set_owner_w()         C->shkey = K', hold K'
    sctp_auth_shkey_release(K)         K refcnt 1

The last chunk reference on K is dropped by the plain
sctp_auth_shkey_release() in sctp_set_owner_w_migrate(). That call skips
the refcnt == 2 check, so no later code path sends SCTP_AUTH_FREE_KEY
for K.

[ ... ]

> @@ -198,6 +204,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)
> +		cb(chunk);
> +
> +	list_for_each_entry(chunk, &asoc->asconf_ack_list, transmitted_list)
> +		cb(chunk);
> +
> +	list_for_each_entry(chunk, &asoc->addip_chunk_list, list)
> +		cb(chunk);
> +
> +	if (asoc->strreset_chunk)
> +		cb(asoc->strreset_chunk);
> +
> +	if (asoc->addip_last_asconf)
> +		cb(asoc->addip_last_asconf);
>  }

[Severity: High]

These new walks call cb() without any filter. The DATA walks go through
traverse_and_process(), which checks c->skb->sk against asoc->base.sk so
each chunk is processed only once. Can a retained control chunk be
reachable from more than one of these lists or pointers at once?

sctp_process_asconf() links the ASCONF-ACK on asconf_ack_list through
transmitted_list:

	sctp_chunk_hold(asconf_ack);
	list_add_tail(&asconf_ack->transmitted_list,
		      &asoc->asconf_ack_list);

sctp_sf_do_asconf() then queues the same chunk on control_chunk_list
through ->list with SCTP_CMD_REPLY. sctp_assoc_lookup_asconf_ack()
already skips entries where sctp_chunk_pending() is true, so being on
both lists is an expected state.

strreset_chunk and addip_last_asconf are also queued on
control_chunk_list while the pointer is still held.

These chunks can stay on control_chunk_list across a flush.
sctp_outq_flush_ctrl() skips them while src_out_of_asoc_ok is set, which
userspace can trigger through sctp_send_asconf_del_ip():

		if (ctx->asoc->src_out_of_asoc_ok &&
		    chunk->chunk_hdr->type != SCTP_CID_ASCONF)
			continue;

It also puts back chunks that fail to transmit:

			if (status != SCTP_XMIT_OK) {
				/* put the chunk back */
				list_add(&chunk->list, &ctx->q->control_chunk_list);

Take such a chunk with chunk->auth set, old key K and new key K'. The
refcounts go like this:

clear pass, first visit:
  sctp_clear_owner_w()        hold K
    skb_orphan()              destructor releases K, clears destructor
clear pass, second visit:
  sctp_clear_owner_w()        hold K
    skb_orphan()              no destructor, nothing released
set pass, first visit:
  sctp_set_owner_w_migrate()  hold K', release K
set pass, second visit:
  sctp_set_owner_w_migrate()  hold K', release K' (chunk->shkey is K')

Does this leak a reference on K? The net change is +1 on K. The struct
sctp_shared_key and its sctp_auth_bytes would then never be freed, and
the refcnt == 2 check in sctp_control_release_owner() could never fire
for K.

A local user who controls both ends over loopback could repeat this for
each association.

Could these walks skip chunks that were already handled, for example
with the same skb->sk check that traverse_and_process() uses?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260926100359.78731-1-juny24602%40gmail.com

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

[Xin Long]

On Tue, Sep 29, 2026 at 8:06 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] In net/sctp/socket.c, sctp_for_each_tx_chunk() walks the control
>   chunks with a bare cb(chunk) on q->control_chunk_list,…
> - [Medium] sctp_set_owner_w_migrate() passes control chunks to
>   sctp_control_set_owner_w().
>
> --
>
> sctp: re-point retained control chunks on association migration
>
> This patch extends the tx walk in sctp_sock_migrate() to cover retained
> control chunks as well: control_chunk_list, asconf_ack_list,
> addip_chunk_list, strreset_chunk and addip_last_asconf. Their owner is
> moved to the new socket. It exports sctp_control_set_owner_w() for chunks
> that have no datamsg, and saves the old chunk->shkey so that key can be
> released after the new owner is set.
>
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 4a08023d52aae..efee9e3e671ef 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -155,9 +155,15 @@ 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);
> >  }
>
> [Severity: Medium]
>
> For control chunks with chunk->auth set, sctp_control_set_owner_w() always
> does this:
>
>         if (chunk->auth) {
>                 chunk->shkey = asoc->shkey;
>                 sctp_auth_shkey_hold(chunk->shkey);
>         }
>
> After peeloff/accept, a retained control chunk (an ASCONF-ACK,
> addip_last_asconf or strreset_chunk) that was built under key K is
> switched to whatever key is active at migration time. Is that intended?
>
> The DATA path through sctp_set_owner_w() keeps the key that was already
> selected. The prerequisite shkey patch also describes the transfer as
> keeping the selected authentication key.
>
> The commit message says:
>
>   "since sctp_control_set_owner_w() may select a different asoc->shkey."
>
> It doesn't mention that retransmissions of these chunks would then be
> authenticated with a different key id.
>
> Can this also lose the SCTP_AUTH_FREE_KEY event for K? Suppose K was
> deactivated while chunk C still holds it, so K's refcnt is 2. K cannot
> be the active key at that point, per sctp_auth_deact_key_id():
>
> clear pass:
>   sctp_clear_owner_w()
>     sctp_auth_shkey_hold(K)            K refcnt 3
>     skb_orphan()
>       sctp_control_release_owner()     sees refcnt 3, no event
>         sctp_auth_shkey_release(K)     K refcnt 2
>
> set pass:
>   sctp_set_owner_w_migrate()
>     sctp_control_set_owner_w()         C->shkey = K', hold K'
>     sctp_auth_shkey_release(K)         K refcnt 1
>
> The last chunk reference on K is dropped by the plain
> sctp_auth_shkey_release() in sctp_set_owner_w_migrate(). That call skips
> the refcnt == 2 check, so no later code path sends SCTP_AUTH_FREE_KEY
> for K.
>
> [ ... ]
We should not allow sctp_control_set_owner_w() to select a different
asoc->shkey if chunk->shkey already exists like in sctp_set_owner_w(),
and we may need a change like:

        if (chunk->auth) {
-               chunk->shkey = asoc->shkey;
+               if (!chunk->shkey)
+                       chunk->shkey = asoc->shkey;
                sctp_auth_shkey_hold(chunk->shkey);
        }

in sctp_control_set_owner_w() for this issue.

>
> > @@ -198,6 +204,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)
> > +             cb(chunk);
> > +
> > +     list_for_each_entry(chunk, &asoc->asconf_ack_list, transmitted_list)
> > +             cb(chunk);
> > +
> > +     list_for_each_entry(chunk, &asoc->addip_chunk_list, list)
> > +             cb(chunk);
> > +
> > +     if (asoc->strreset_chunk)
> > +             cb(asoc->strreset_chunk);
> > +
> > +     if (asoc->addip_last_asconf)
> > +             cb(asoc->addip_last_asconf);
> >  }
>
> [Severity: High]
>
> These new walks call cb() without any filter. The DATA walks go through
> traverse_and_process(), which checks c->skb->sk against asoc->base.sk so
> each chunk is processed only once. Can a retained control chunk be
> reachable from more than one of these lists or pointers at once?
>
> sctp_process_asconf() links the ASCONF-ACK on asconf_ack_list through
> transmitted_list:
>
>         sctp_chunk_hold(asconf_ack);
>         list_add_tail(&asconf_ack->transmitted_list,
>                       &asoc->asconf_ack_list);
>
> sctp_sf_do_asconf() then queues the same chunk on control_chunk_list
> through ->list with SCTP_CMD_REPLY. sctp_assoc_lookup_asconf_ack()
> already skips entries where sctp_chunk_pending() is true, so being on
> both lists is an expected state.
>
> strreset_chunk and addip_last_asconf are also queued on
> control_chunk_list while the pointer is still held.
>
> These chunks can stay on control_chunk_list across a flush.
> sctp_outq_flush_ctrl() skips them while src_out_of_asoc_ok is set, which
> userspace can trigger through sctp_send_asconf_del_ip():
>
>                 if (ctx->asoc->src_out_of_asoc_ok &&
>                     chunk->chunk_hdr->type != SCTP_CID_ASCONF)
>                         continue;
>
> It also puts back chunks that fail to transmit:
>
>                         if (status != SCTP_XMIT_OK) {
>                                 /* put the chunk back */
>                                 list_add(&chunk->list, &ctx->q->control_chunk_list);
>
> Take such a chunk with chunk->auth set, old key K and new key K'. The
> refcounts go like this:
>
> clear pass, first visit:
>   sctp_clear_owner_w()        hold K
>     skb_orphan()              destructor releases K, clears destructor
> clear pass, second visit:
>   sctp_clear_owner_w()        hold K
>     skb_orphan()              no destructor, nothing released
> set pass, first visit:
>   sctp_set_owner_w_migrate()  hold K', release K
> set pass, second visit:
>   sctp_set_owner_w_migrate()  hold K', release K' (chunk->shkey is K')
>
> Does this leak a reference on K? The net change is +1 on K. The struct
> sctp_shared_key and its sctp_auth_bytes would then never be freed, and
> the refcnt == 2 check in sctp_control_release_owner() could never fire
> for K.
>
> A local user who controls both ends over loopback could repeat this for
> each association.
>
> Could these walks skip chunks that were already handled, for example
> with the same skb->sk check that traverse_and_process() uses?
>
It looks like bringing back sctp_process_tx_chunk() from your v2 patch
will fix this. Sorry about it.

Thanks.