[PATCH net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START

[PATCH net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START

[Xin Long]

The SCTP_CMD_TIMER_START handler checks timer_pending() before calling
timer_reduce(). The timer can expire and detach between these operations,
causing timer_reduce() to rearm the timer without taking the association
reference required for the newly armed timer.

The timer callback later unconditionally drops its association reference,
which can leave the association reference count unbalanced and result in
use-after-free during association teardown.

Use the return value of timer_reduce() to determine whether the timer was
actually armed. Take the association reference only when timer_reduce()
successfully starts a new timer, closing the race between checking the
timer state and rearming it.

This issue was reported by Nico Yip (@_cyeaa_) working with TrendAI Zero
Day Initiative.

Fixes: 20a785aa52c8 ("sctp: Don't add the shutdown timer if its already been added")
Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/sm_sideeffect.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 94716406d602..0d99b7e8c082 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -1545,17 +1545,8 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
 			timeout = asoc->timeouts[cmd->obj.to];
 			BUG_ON(!timeout);
 
-			/*
-			 * SCTP has a hard time with timer starts.  Because we process
-			 * timer starts as side effects, it can be hard to tell if we
-			 * have already started a timer or not, which leads to BUG
-			 * halts when we call add_timer. So here, instead of just starting
-			 * a timer, if the timer is already started, and just mod
-			 * the timer with the shorter of the two expiration times
-			 */
-			if (!timer_pending(timer))
+			if (!timer_reduce(timer, jiffies + timeout))
 				sctp_association_hold(asoc);
-			timer_reduce(timer, jiffies + timeout);
 			break;
 
 		case SCTP_CMD_TIMER_RESTART:
-- 
2.47.1

Re: [PATCH net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START

[David Laight]

On Wed, 26 Aug 2026 15:49:04 -0400
Xin Long <lucien.xin@gmail.com> wrote:

> The SCTP_CMD_TIMER_START handler checks timer_pending() before calling
> timer_reduce(). The timer can expire and detach between these operations,
> causing timer_reduce() to rearm the timer without taking the association
> reference required for the newly armed timer.
> 
> The timer callback later unconditionally drops its association reference,
> which can leave the association reference count unbalanced and result in
> use-after-free during association teardown.
> 
> Use the return value of timer_reduce() to determine whether the timer was
> actually armed. Take the association reference only when timer_reduce()
> successfully starts a new timer, closing the race between checking the
> timer state and rearming it.
> 
> This issue was reported by Nico Yip (@_cyeaa_) working with TrendAI Zero
> Day Initiative.
> 
> Fixes: 20a785aa52c8 ("sctp: Don't add the shutdown timer if its already been added")
> Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/sm_sideeffect.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 94716406d602..0d99b7e8c082 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -1545,17 +1545,8 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
>  			timeout = asoc->timeouts[cmd->obj.to];
>  			BUG_ON(!timeout);
>  
> -			/*
> -			 * SCTP has a hard time with timer starts.  Because we process
> -			 * timer starts as side effects, it can be hard to tell if we
> -			 * have already started a timer or not, which leads to BUG
> -			 * halts when we call add_timer. So here, instead of just starting
> -			 * a timer, if the timer is already started, and just mod
> -			 * the timer with the shorter of the two expiration times
> -			 */
> -			if (!timer_pending(timer))
> +			if (!timer_reduce(timer, jiffies + timeout))
>  				sctp_association_hold(asoc);
> -			timer_reduce(timer, jiffies + timeout);

Doesn't that just add a different timing window?
If the timer expires before the hold() it will release the reference the
calling code owns.
So you either need:
			sctp_association_hold(asoc);
			if (timer_reduce(timer, jiffies + timeout))
				sctp_association_release(asoc);
or to hold two references across all the code.

David

>  			break;
>  
>  		case SCTP_CMD_TIMER_RESTART:

Re: [PATCH net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START

[Xin Long]

On Thu, Aug 27, 2026 at 5:27 AM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Wed, 26 Aug 2026 15:49:04 -0400
> Xin Long <lucien.xin@gmail.com> wrote:
>
> > The SCTP_CMD_TIMER_START handler checks timer_pending() before calling
> > timer_reduce(). The timer can expire and detach between these operations,
> > causing timer_reduce() to rearm the timer without taking the association
> > reference required for the newly armed timer.
> >
> > The timer callback later unconditionally drops its association reference,
> > which can leave the association reference count unbalanced and result in
> > use-after-free during association teardown.
> >
> > Use the return value of timer_reduce() to determine whether the timer was
> > actually armed. Take the association reference only when timer_reduce()
> > successfully starts a new timer, closing the race between checking the
> > timer state and rearming it.
> >
> > This issue was reported by Nico Yip (@_cyeaa_) working with TrendAI Zero
> > Day Initiative.
> >
> > Fixes: 20a785aa52c8 ("sctp: Don't add the shutdown timer if its already been added")
> > Reported-by: Zero Day Initiative <zdi-disclosures@trendmicro.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/sm_sideeffect.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index 94716406d602..0d99b7e8c082 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -1545,17 +1545,8 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
> >                       timeout = asoc->timeouts[cmd->obj.to];
> >                       BUG_ON(!timeout);
> >
> > -                     /*
> > -                      * SCTP has a hard time with timer starts.  Because we process
> > -                      * timer starts as side effects, it can be hard to tell if we
> > -                      * have already started a timer or not, which leads to BUG
> > -                      * halts when we call add_timer. So here, instead of just starting
> > -                      * a timer, if the timer is already started, and just mod
> > -                      * the timer with the shorter of the two expiration times
> > -                      */
> > -                     if (!timer_pending(timer))
> > +                     if (!timer_reduce(timer, jiffies + timeout))
> >                               sctp_association_hold(asoc);
> > -                     timer_reduce(timer, jiffies + timeout);
>
> Doesn't that just add a different timing window?
> If the timer expires before the hold() it will release the reference the
> calling code owns.
Yes, but I think it will be harmless, since this path holds the sock lock,
and the timer handler will restart the timer and take another reference to
the asoc before releasing it in sctp_generate_timeout_event().

I've seen similar behavior in the SCTP_CMD_TIMER_RESTART case, so I would
keep it consistent with that for now.

Thanks.

> So you either need:
>                         sctp_association_hold(asoc);
>                         if (timer_reduce(timer, jiffies + timeout))
>                                 sctp_association_release(asoc);
> or to hold two references across all the code.
>
> David
>
> >                       break;
> >
> >               case SCTP_CMD_TIMER_RESTART:
>

Re: [PATCH net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 26 Aug 2026 15:49:04 -0400 you wrote:
> The SCTP_CMD_TIMER_START handler checks timer_pending() before calling
> timer_reduce(). The timer can expire and detach between these operations,
> causing timer_reduce() to rearm the timer without taking the association
> reference required for the newly armed timer.
> 
> The timer callback later unconditionally drops its association reference,
> which can leave the association reference count unbalanced and result in
> use-after-free during association teardown.
> 
> [...]

Here is the summary with links:
  - [net] sctp: fix a TOCTOU race in SCTP_CMD_TIMER_START
    https://git.kernel.org/netdev/net/c/2188569e7e1b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html