Re: [PATCH v7 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 6 potential issue(s) to consider:

New issues:
- [High] Time-of-check to time-of-use (TOCTOU) race in `err_out_drop` corrupts the state of concurrent transfers.
- [High] Reordering timer cancellation before state reset allows RX handlers to re-arm stale timers on an idle socket.
- [High] Missing memory barrier in `err_out_drop` allows store reordering, causing data corruption.
- [High] Suppressing the timeout error in the TX timer handler causes false success and indefinite blocking.

Pre-existing issues:
- [High] `hrtimer_cancel()` calls in RX paths race with concurrent `sendmsg()`, permanently hanging new transfers.
- [High] `isotp_rcv_echo()` unconditionally clears `cfecho` before generation validation, corrupting new transfers.
--

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

can: isotp: fix race between RX/TX timers and frame reception

This commit addresses a race where hrtimer_cancel() waits for a concurrent
timer handler to finish, potentially allowing the handler to reset the state
machine. It introduces a generation counter to disambiguate new transfers
from aborted ones.

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..f05703ddd2eae 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -371,6 +372,7 @@ static void isotp_send_cframe(struct isotp_sock *so);
>  static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  {
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
>  
>  	if (so->tx.state != ISOTP_WAIT_FC &&
>  	    so->tx.state != ISOTP_WAIT_FIRST_FC)
> @@ -378,6 +380,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  
>  	hrtimer_cancel(&so->txtimer);
>  

[Severity: High]
This is a pre-existing issue, but could canceling the timer before validating
the generation counter create a race that permanently hangs new transfers?

If the TX timer concurrently times out, the handler resets the state to
ISOTP_IDLE. A concurrent sendmsg() could then immediately claim the socket,
increment tx_gen, and arm txtimer for a new transfer.

When hrtimer_cancel() resumes from blocking in isotp_rcv_fc(), wouldn't it
inadvertently cancel the newly armed timer? Even though the subsequent tx_gen
check aborts processing the stale frame, the new transfer is left without a
watchdog timer and could hang permanently if a frame is dropped.

This exact same race appears to exist in isotp_rcv_echo() as well.

> +	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up on
> +	 * this job (state changed) or a new sendmsg() claim and reach the
> +	 * same state again (tx_gen changed) - either way this FC is stale.
> +	 */
> +	if ((so->tx.state != ISOTP_WAIT_FC &&
> +	     so->tx.state != ISOTP_WAIT_FIRST_FC) ||
> +	    READ_ONCE(so->tx_gen) != gen)
> +		return 1;
> +
[ ... ]
> @@ -870,6 +889,7 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	struct sock *sk = (struct sock *)data;
>  	struct isotp_sock *so = isotp_sk(sk);
>  	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	u32 gen = READ_ONCE(so->tx_gen);
>  
>  	/* only handle my own local echo CF/SF skb's (no FF!) */
>  	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
> @@ -881,6 +901,14 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  

[Severity: High]
This is a pre-existing issue, but does unconditionally clearing the echo tag
here corrupt concurrent new transfers?

Since the cleanup happens before the tx_gen check, if a new transfer starts
while hrtimer_cancel() is blocking, the new transfer will set its own cfecho
tag. Won't this delayed store to cfecho overwrite the new transfer's tag,
causing its future echo frames to be dropped and the transfer to hang?

> +	/* hrtimer_cancel() may have let isotp_tx_timer_handler() give up
> +	 * on this job (state changed) or a new sendmsg() claim and reach
> +	 * ISOTP_SENDING again (tx_gen changed) - either way this echo is
> +	 * stale.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
> +		return;
> +
[ ... ]
> @@ -1143,13 +1179,20 @@ 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: cancel timers before touching state, so a concurrent
> +	 * sendmsg() can't claim ISOTP_SENDING while a stale timer is armed
> +	 */
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);

[Severity: High]
By reordering the timer cancellation before the state reset, is there a risk
that RX handlers could re-arm stale timers?

While spinning here to cancel timers, the socket is left in an active state
(like ISOTP_SENDING). A concurrent RX handler, such as isotp_rcv_echo(), could
pass its state checks and arm a timer (txfrtimer or txtimer) after we have
already finished canceling them here.

When the state is subsequently forced to ISOTP_IDLE below, it leaves an armed
timer on an idle socket. If a new sendmsg() then claims the socket, could
this stale timer fire and overwrite the new transfer's timeout or falsely abort
it?

>  err_out_drop:
> -	/* drop this PDU and unlock a potential wait queue */
> -	so->tx.state = ISOTP_IDLE;
> +	/* drop this PDU and unlock a potential wait queue, but only if no
> +	 * concurrent sendmsg() has since claimed the socket for a new
> +	 * transfer while we were cancelling timers above
> +	 */
> +	if (READ_ONCE(so->tx_gen) == my_gen) {
> +		so->cfecho = 0;
> +		so->tx.state = ISOTP_IDLE;
> +	}

[Severity: High]
Could this non-atomic check create a TOCTOU (time-of-check to time-of-use)
race that corrupts concurrent transfers?

If err_event_drop cancels timers, a concurrent RX handler could have already
set the state to ISOTP_IDLE. A concurrent sendmsg() could then claim the
socket and set the state to ISOTP_SENDING.

If this thread evaluates READ_ONCE(so->tx_gen) == my_gen just before the new
transfer bumps the generation, it will then execute so->tx.state = ISOTP_IDLE.
Won't this unconditionally corrupt the new transfer's lockless state machine?

[Severity: High]
Is a memory barrier missing here when releasing the state machine?

Since there is no smp_store_release(), the compiler or CPU might reorder
the stores to cfecho and tx.state. If ISOTP_IDLE becomes visible first, a
concurrent sendmsg() could claim the socket and set a new cfecho tag.

Wouldn't the delayed store to so->cfecho = 0 then erroneously clear the tag
for the new transfer, causing it to drop its own echoes and hang?

>  	wake_up_interruptible(&so->wait);
>  
>  	return err;

Looking at isotp_tx_timer_handler() in the same file:

	if (READ_ONCE(so->tx_gen) == gen) {
		/* report 'communication error on send' */
		sk->sk_err = ECOMM;
		if (!sock_flag(sk, SOCK_DEAD))
			sk_error_report(sk);
	}

[Severity: High]
Will suppressing the timeout error here cause the original waiting thread to
block indefinitely?

If a new transfer claims the socket (tx_gen != gen), the error is skipped,
but the original sendmsg() thread waiting in wait_tx_done is still woken up.
Because it checks so->tx.state == ISOTP_IDLE (which is false, since the new
transfer set it to ISOTP_SENDING), it goes back to sleep, effectively waiting
for an unrelated transfer to finish.

When it finally wakes up, sock_error() will return 0 because the error was
suppressed, masking the timeout failure as a successful transmission.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net?part=3