Re: [PATCH v6 3/5] can: isotp: fix race between RX/TX timers and frame reception

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving `so->tx.state = ISOTP_IDLE` after `hrtimer_cancel()` in the `err_event_drop` path introduces a race condition with softirq receive handlers, leading to stray timers that can corrupt subsequent transmissions.
- [High] The delayed `so->tx.state = ISOTP_IDLE` assignment in the error path unconditionally clobbers the state, which can corrupt a new, concurrently running transfer.
- [High] The newly added TX state checks after `hrtimer_cancel()` in `isotp_rcv_fc()` and `isotp_rcv_echo()` are vulnerable to an ABA race condition, allowing stale frames to corrupt new transfers.

Pre-existing issues:
- [High] Missing cleanup of `so->cfecho` in `isotp_sendmsg()` error paths allows stale local echoes to corrupt subsequent transfers.
--

--- Patch [3]: [PATCH v6 3/5] can: isotp: fix race between RX/TX timers and frame reception ---
commit 4f481801d9d9059ae72ceaf6467d32f307eddbb5
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: isotp: fix race between RX/TX timers and frame reception

This patch fixes a race condition between hrtimer execution and frame
reception by checking the state after hrtimer_cancel(). It also reorders
timer cancellation and state reset in isotp_sendmsg()'s error path to
prevent concurrent sendmsg calls from corrupting state.

Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 44c044eb83e12..5ef2a610fc172 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -378,6 +378,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  
>  	hrtimer_cancel(&so->txtimer);
>  
> +	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
> +	 * hrtimer_cancel() above waited for it to finish, already reporting
> +	 * the tx error and resetting the state; don't resume a tx job that
> +	 * has already been given up on.
> +	 */
> +	if (so->tx.state != ISOTP_WAIT_FC &&
> +	    so->tx.state != ISOTP_WAIT_FIRST_FC)
> +		return 1;
> +

[Severity: High]
Is this lockless state check vulnerable to an ABA transition?

While hrtimer_cancel() spins waiting for a timer handler, the timer handler
could reset the state to ISOTP_IDLE. If a concurrent isotp_sendmsg() from
another thread starts a new transfer during this window, it could locklessly
transition the state back to the exact same expected state
(e.g., ISOTP_WAIT_FIRST_FC).

When hrtimer_cancel() returns, this check would pass because the state
matches, but it would be evaluating a different transfer. Could this allow a
stale Flow Control frame to corrupt the new transfer?

[ ... ]
> @@ -881,6 +898,14 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  
> +	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
> +	 * hrtimer_cancel() above waited for it to finish, already reporting
> +	 * the tx error and resetting the state; don't resume a tx job that
> +	 * has already been given up on.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING)
> +		return;
> +

[Severity: High]
Could the same ABA race happen here?

If a timer resets the state to ISOTP_IDLE while hrtimer_cancel() waits,
and a concurrent isotp_sendmsg() starts a new transfer and sets the state to
ISOTP_SENDING, this check would pass for the new transfer.

Would this allow a stale local echo to erroneously resume the newly started
transfer?

[ ... ]
> @@ -1143,8 +1168,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	return size;
>  
>  err_event_drop:
> -	/* got signal: force tx state machine to be idle */
> -	so->tx.state = ISOTP_IDLE;
> +	/* got signal: force tx state machine to be ISOTP_IDLE */
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  err_out_drop:

[Severity: High]
Does delaying the state reset to after hrtimer_cancel() create a window
where concurrent RX softirqs can observe an active state?

If isotp_sendmsg() is interrupted by a signal, it now calls
hrtimer_cancel() while so->tx.state is still visibly active
(e.g., ISOTP_SENDING). If a CAN receive handler (like isotp_rcv_echo())
executes concurrently in softirq context, it locklessly checks
so->tx.state, sees it is active, and can re-arm the transmission timers.

When this error path completes, it sets the state to ISOTP_IDLE, leaving
the newly armed timers active. When a subsequent sendmsg() begins, could
these stray timers fire unexpectedly and corrupt the new transfer's
sequence?

[Severity: High]
Does the delayed assignment of ISOTP_IDLE in err_out_drop clobber
concurrently claimed state?

While hrtimer_cancel() waits for a timer handler to finish (which resets
the state to ISOTP_IDLE), a concurrent thread could call isotp_sendmsg(),
successfully claim the ISOTP_IDLE state, and begin a new transfer.

When the original thread finishes hrtimer_cancel(), it unconditionally
executes so->tx.state = ISOTP_IDLE (located under err_out_drop:). Could
this abruptly clobber the new transfer's state and expose its buffer to
concurrent overwrites?

[Severity: High]
This is a pre-existing issue, but does the error path fail to clean up
so->cfecho?

If a transmission is aborted via a signal, the state is forced to
ISOTP_IDLE but so->cfecho is not cleared and continues to hold the local
echo tag of the aborted frame.

When a subsequent sendmsg() sets the state to ISOTP_SENDING and begins
preparing the buffer, a stale echo could arrive. If isotp_rcv_echo() sees
a matching so->cfecho and an active ISOTP_SENDING state, could it process
the echo and initiate a transmission containing uninitialized heap data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v6-0-a13c2539093d@hartkopp.net?part=3