Re: [PATCH v8 3/4] 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] Error paths in `isotp_sendmsg()` unconditionally cancel hrtimers, which can inadvertently destroy the active timers of a newly started concurrent transfer.
- [High] A delayed local echo frame from a previous transfer can bypass the `tx_gen` validation and corrupt a newly started transfer due to a TOCTOU race with `so->cfecho`.
- [High] The timeout handler incorrectly suppresses the socket timeout error (`ECOMM`) if a concurrent transfer claims the socket, causing the timed-out `sendmsg()` to silently drop data and falsely report success.

Pre-existing issues:
- [High] Concurrent `sendmsg` calls cause the active sender to hang waiting for a new sender to finish, leading to hangs or cross-transfer status leaks.
--

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

This commit introduces so->rx_lock to protect the TX state machine against
concurrent hrtimer execution and frame reception. It also adds a tx_gen
generation counter to safely distinguish between old and new TX transfers
when cancelling timers or processing echo frames locklessly.

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..c5b44d298618b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -870,22 +888,42 @@ 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;
>  
>  	/* only handle my own local echo CF/SF skb's (no FF!) */
> -	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
> +	if (skb->sk != sk)
>  		return;
>  
> +	/* unlike isotp_rcv_fc()/isotp_rcv_cf(), not already under so->rx_lock
> +	 * (no isotp_rcv() caller here), so take it ourselves
> +	 */
> +	spin_lock(&so->rx_lock);
> +
> +	/* so->cfecho may since belong to a new transfer; recheck under lock */
> +	if (so->cfecho != *(u32 *)cf->data)
> +		goto out_unlock;
> +
> +	gen = READ_ONCE(so->tx_gen);

[Severity: High]
Can a delayed echo frame from a previous transfer bypass the validation and
corrupt a newly started transfer here?

If transfer 1 times out, it leaves its echo tag in so->cfecho.
If transfer 2 then claims the socket, it locklessly increments so->tx_gen
in isotp_sendmsg() before acquiring so->rx_lock to clear so->cfecho.

If a delayed echo for transfer 1 arrives in that small window, isotp_rcv_echo()
will acquire so->rx_lock, match the old so->cfecho (which hasn't been cleared
yet), and read the new so->tx_gen.

Does this allow the stale echo to pass the generation check below and disrupt
the protocol state machine of the new transfer?

>  
>  	/* cancel local echo timeout */
> -	hrtimer_cancel(&so->txtimer);
> +	hrtimer_cancel(&so->echotimer);
>  
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  
> +	/* so->rx_lock stops a new claim from arming a timer of its own
> +	 * while we're still here, but not from the lock-free cmpxchg()
> +	 * claim itself: state alone can't tell our SENDING from a new
> +	 * transfer's, so also gate on gen.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
> +		goto out_unlock;
[ ... ]
> -static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
> +/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get
> + * cancelled under so->rx_lock elsewhere, so this must stay lock-free to
> + * avoid deadlocking with that; uses so->tx_gen instead to avoid tainting
> + * a new transfer with an error from the one that just timed out.
> + */
> +static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  {
> -	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
> -					     txtimer);
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
> +	u32 old_state = READ_ONCE(so->tx.state);
>  
>  	/* don't handle timeouts in IDLE or SHUTDOWN state */
> -	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
> +	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
> +		return HRTIMER_NORESTART;
> +
> +	/* only claim the timeout if the state is still unchanged */
> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
>  		return HRTIMER_NORESTART;
>  
>  	/* we did not get any flow control or echo frame in time */
>  
> -	/* report 'communication error on send' */
> -	sk->sk_err = ECOMM;
> -	if (!sock_flag(sk, SOCK_DEAD))
> -		sk_error_report(sk);
> +	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]
Does this suppress the socket timeout error if a concurrent transfer claims
the socket right after the state transitions to ISOTP_IDLE?

Once cmpxchg() sets so->tx.state to ISOTP_IDLE, a concurrent thread calling
isotp_sendmsg() can immediately claim the state and increment so->tx_gen.

If that happens before the READ_ONCE(so->tx_gen) == gen check, the error
reporting is skipped. Does this cause the original timed-out isotp_sendmsg()
call to falsely report success to userspace instead of returning an error?

>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;
>  }
[ ... ]
> @@ -1120,13 +1207,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
>  			       __func__, ERR_PTR(err));
>  
> +		spin_lock_bh(&so->rx_lock);
>  		/* no transmission -> no timeout monitoring */
> -		hrtimer_cancel(&so->txtimer);
> -
> -		/* reset consecutive frame echo tag */
> -		so->cfecho = 0;
> -
> -		goto err_out_drop;
> +		hrtimer_cancel(tx_hrt);

[Severity: High]
Can this unconditionally cancel the timer of a newly started concurrent
transfer?

If can_send() fails, this path cancels tx_hrt before verifying ownership
via tx_gen. If a concurrent sendmsg() has already claimed the socket and
started its own timers, will this cancel the new transfer's timers and
cause it to hang indefinitely without timeout monitoring?

> +		goto err_out_drop_locked;
>  	}
>  
>  	if (wait_tx_done) {
> @@ -1142,14 +1226,28 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  
>  	return size;

[Severity: High]
This isn't a bug introduced by this patch, but does the wait condition here
handle concurrent senders correctly?

When using CAN_ISOTP_WAIT_TX_DONE, isotp_sendmsg() waits on:
    wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);

If transfer 1's transmission completes (setting ISOTP_IDLE and waking waiters),
but transfer 2 concurrently claims ISOTP_SENDING before transfer 1 wakes up,
transfer 1 will evaluate so->tx.state == ISOTP_IDLE as false.

Will transfer 1 then go back to sleep, incorrectly hanging while waiting on
transfer 2's completion?

>  
> +err_out_drop:
> +	spin_lock_bh(&so->rx_lock);
> +	goto err_out_drop_locked;
>  err_event_drop:
> -	/* got signal: force tx state machine to be idle */
> -	so->tx.state = ISOTP_IDLE;
> +	/* got signal: cancel timers under so->rx_lock, held through
> +	 * err_out_drop_locked below, so a concurrent claim's own drain
> +	 * step can't proceed until we're done
> +	 */
> +	spin_lock_bh(&so->rx_lock);
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
> -err_out_drop:
> -	/* drop this PDU and unlock a potential wait queue */
> -	so->tx.state = ISOTP_IDLE;
> +	hrtimer_cancel(&so->echotimer);

[Severity: High]
Similar to the can_send() error path above, this unconditionally cancels
all TX timers without checking if the socket is still owned by the current
transfer's generation.

If a timeout already occurred and a new transfer is active, will cancelling
these timers destroy the active timers of the new transfer?

> +err_out_drop_locked:
> +	/* so->rx_lock stops a new claim from cancelling anything of its own
> +	 * while we're still here, but not from the lock-free cmpxchg()
> +	 * claim itself; only drop our own PDU if that hasn't happened
> +	 */
> +	if (READ_ONCE(so->tx_gen) == gen) {
> +		so->cfecho = 0;
> +		so->tx.state = ISOTP_IDLE;
> +	}
> +	spin_unlock_bh(&so->rx_lock);
>  	wake_up_interruptible(&so->wait);
>  
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=3