[PATCH net] sctp: fix use-after-free of cached ASCONF chunk

Yuxiang Yang posted 1 patch 1 month, 3 weeks ago
There is a newer version of this series
net/sctp/associola.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH net] sctp: fix use-after-free of cached ASCONF chunk
Posted by Yuxiang Yang 1 month, 3 weeks ago
addip_last_asconf caches the outstanding outbound ASCONF chunk. The normal
ASCONF-ACK completion path releases the chunk and clears the pointer.

However, sctp_asconf_queue_teardown() releases the cached chunk without
clearing addip_last_asconf. During peer restart handling,
sctp_sf_do_dupcook_a() queues SCTP_CMD_PURGE_ASCONF_QUEUE, which invokes
sctp_asconf_queue_teardown() while the association remains alive and leaves
the pointer dangling.

A delayed authenticated ASCONF-ACK can then reach sctp_sf_do_asconf_ack(),
which accesses the stale chunk and passes it to sctp_process_asconf_ack(),
causing a use-after-free and a second release.

Clear addip_last_asconf after releasing the cached chunk. This restores the
invariant that a non-NULL pointer always references a live chunk.

Fixes: a000c01e60e4 ("sctp: stop pending timers and purge queues when peer restart asoc")
Cc: stable@vger.kernel.org
Assisted-by: Claude-Code:GLM-5.2
Signed-off-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
---
 net/sctp/associola.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index b6ac096..cf3e807 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1709,6 +1709,8 @@ void sctp_asconf_queue_teardown(struct sctp_association *asoc)
 	sctp_assoc_free_asconf_queue(asoc);
 
 	/* Free any cached ASCONF chunk. */
-	if (asoc->addip_last_asconf)
+	if (asoc->addip_last_asconf) {
 		sctp_chunk_free(asoc->addip_last_asconf);
+		asoc->addip_last_asconf = NULL;
+	}
 }
-- 
2.25.1
Re: [PATCH net] sctp: fix use-after-free of cached ASCONF chunk
Posted by Xin Long 1 month, 3 weeks ago
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 5b0ae616e1ff9..737f8ea9d482f 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -1713,6 +1713,8 @@ void sctp_asconf_queue_teardown(struct sctp_association *asoc)
> >          sctp_assoc_free_asconf_queue(asoc);
> >
> >          /* Free any cached ASCONF chunk. */
> > -        if (asoc->addip_last_asconf)
> > +        if (asoc->addip_last_asconf) {
> >                  sctp_chunk_free(asoc->addip_last_asconf);
> > +                asoc->addip_last_asconf = NULL;
> This is a pre-existing issue, but does setting this pointer to NULL here
> create a NULL pointer dereference in the T4 RTO timer handler?
> During a peer restart initiated by an unexpected COOKIE ECHO,
> sctp_sf_do_dupcook_a() queues a command to purge the ASCONF queue, which
> calls this function. It also queues a command to stop the T4 RTO timer
> using del_timer().
> Because del_timer() is not synchronous, if the T4 RTO timer is already
> executing on another CPU, it will block on bh_lock_sock(). Once the
> restart processing completes and the socket lock is released, the delayed
> timer resumes and can unconditionally dereference the now-NULL pointer:
> net/sctp/sm_statefuns.c:sctp_sf_t4_timer_expire() {
>         struct sctp_chunk *chunk = asoc->addip_last_asconf;
>         struct sctp_transport *transport = chunk->transport;
>         ...
> }
> Previously this race caused a use-after-free, but with this change, it seems
> it will result in a guaranteed kernel panic. Are there any synchronization
> mechanisms preventing the timer from running and dereferencing this pointer
> after the queue is purged?

Hi, Yuxiang,

Please address the issue reported by both Sashikos in this patch by adding
a NULL check in sctp_sf_t4_timer_expire(), like:

@@ -6105,7 +6105,12 @@ enum sctp_disposition sctp_sf_t4_timer_expire(
                                        struct sctp_cmd_seq *commands)
 {
        struct sctp_chunk *chunk = asoc->addip_last_asconf;
-       struct sctp_transport *transport = chunk->transport;
+       struct sctp_transport *transport;
+
+       if (!chunk)
+               return SCTP_DISPOSITION_CONSUME;
+
+       transport = chunk->transport;

        SCTP_INC_STATS(net, SCTP_MIB_T4_RTO_EXPIREDS);

Otherwise, this change would turn the existing issue into a NULL pointer
dereference.

Thanks.
[PATCH net v2] sctp: fix use-after-free of cached ASCONF chunk
Posted by Yuxiang Yang 1 month, 3 weeks ago
addip_last_asconf caches the outstanding outbound ASCONF chunk. The normal
ASCONF-ACK completion path releases the chunk and clears the pointer.

However, sctp_asconf_queue_teardown() releases the cached chunk without
clearing addip_last_asconf. During peer restart handling,
sctp_sf_do_dupcook_a() queues SCTP_CMD_PURGE_ASCONF_QUEUE, which invokes
sctp_asconf_queue_teardown() while the association remains alive and leaves
the pointer dangling.

A delayed authenticated ASCONF-ACK can then reach sctp_sf_do_asconf_ack(),
which accesses the stale chunk and passes it to sctp_process_asconf_ack(),
causing a use-after-free and a second release.

Clearing the pointer exposes a race with T4 expiry. Peer restart handling
queues the timer stop before the purge, but SCTP_CMD_TIMER_STOP uses
timer_delete(), which does not wait for a callback already running on
another CPU. Such a callback can reach sctp_sf_t4_timer_expire() after
the purge and dereference NULL.

Clear addip_last_asconf after releasing the cached chunk, and make
sctp_sf_t4_timer_expire() consume a stale T4 expiry if no outstanding
ASCONF remains.

Fixes: a000c01e60e4 ("sctp: stop pending timers and purge queues when peer restart asoc")
Cc: stable@vger.kernel.org
Suggested-by: Xin Long <lucien.xin@gmail.com>
Assisted-by: Claude-Code:GLM-5.2
Signed-off-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
---
Changes in v2:
- Guard sctp_sf_t4_timer_expire() against a cleared cached ASCONF pointer,
  as requested by Xin Long.
- Explain why the T4 callback may run after the restart purge.

 net/sctp/associola.c    | 4 +++-
 net/sctp/sm_statefuns.c | 6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 5b0ae61..737f8ea 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1713,6 +1713,8 @@ void sctp_asconf_queue_teardown(struct sctp_association *asoc)
 	sctp_assoc_free_asconf_queue(asoc);
 
 	/* Free any cached ASCONF chunk. */
-	if (asoc->addip_last_asconf)
+	if (asoc->addip_last_asconf) {
 		sctp_chunk_free(asoc->addip_last_asconf);
+		asoc->addip_last_asconf = NULL;
+	}
 }
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 708fa07..3a8e16b 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -6145,8 +6145,12 @@ enum sctp_disposition sctp_sf_t4_timer_expire(
 					struct sctp_cmd_seq *commands)
 {
 	struct sctp_chunk *chunk = asoc->addip_last_asconf;
-	struct sctp_transport *transport = chunk->transport;
+	struct sctp_transport *transport;
+
+	if (!chunk)
+		return SCTP_DISPOSITION_CONSUME;
 
+	transport = chunk->transport;
 	SCTP_INC_STATS(net, SCTP_MIB_T4_RTO_EXPIREDS);
 
 	/* ADDIP 4.1 B1) Increment the error counters and perform path failure
-- 
2.25.1