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

[David Laight <david.laight.linux@gmail.com>]

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: