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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The lock-free state claim mechanism in isotp_sendmsg() races with the generation bump, causing state clobbering that leads to concurrent buffer corruption and an Out-Of-Bounds (OOB) read leaking kernel memory to the CAN bus.
--

--- Patch [3]: [PATCH v9 3/4] can: isotp: fix race between RX/TX timers and frame reception ---
commit b972a6bcac8d8b55930b5c1372f223dd7d9a46de
Author: Oliver Hartkopp <[email protected]>

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

This commit attempts to fix a race by introducing a generation counter
(tx_gen) to distinguish between old and new TX transfers. It also expands
rx_lock to cover the TX side for proper synchronization.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 44c044eb83e12..c4ca8a64e7ce1 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)
[ ... ]
> +	/* 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;

[Severity: Critical]
Does the echo receiver similarly validate against a stale tx_gen if a new
transfer is blocked waiting for rx_lock?

If the generation hasn't been bumped yet, it appears this could process echoes
for the wrong transfer and inadvertently clobber its state.

> @@ -980,6 +1050,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			goto err_event_drop;
>  	}
>  
> +	/* new transfer: bump the generation (gen is used below and by
> +	 * isotp_tx_timeout()/isotp_rcv_echo()) and drain the old one -
> +	 * cancel its timers ourselves in case one is still concurrently
> +	 * completing it, and take so->rx_lock so no isotp_rcv_fc()/
> +	 * isotp_rcv_echo() call for it is still in flight. The bump and
> +	 * the so->cfecho reset happen in the same so->rx_lock section so
> +	 * isotp_rcv_echo() can never observe the new gen alongside the
> +	 * old, not-yet-cleared cfecho tag.
> +	 */
> +	spin_lock_bh(&so->rx_lock);
> +	gen = READ_ONCE(so->tx_gen) + 1;
> +	WRITE_ONCE(so->tx_gen, gen);

[Severity: Critical]
Because cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) happens before
this lock is acquired, is there a window where a new transfer has claimed the
state but hasn't bumped tx_gen yet?

> @@ -1142,14 +1231,36 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  
>  	return size;
>  
> +err_out_drop:
> +	spin_lock_bh(&so->rx_lock);
> +	goto err_out_drop_locked;
>  err_event_drop:
[ ... ]
> +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;

[Severity: Critical]
Can this lock-free claim in isotp_sendmsg() race with the generation bump and
cause state clobbering?

If Thread A is in err_event_drop() and holds rx_lock, and Thread C (a timeout
timer executing isotp_tx_timeout()) locklessly sets the state to ISOTP_IDLE:

Thread C (isotp_tx_timeout)
   cmpxchg(&so->tx.state, old_state, ISOTP_IDLE);

Then Thread B calls isotp_sendmsg(), sees ISOTP_IDLE, claims it to
ISOTP_SENDING locklessly, and blocks on rx_lock:

Thread B (isotp_sendmsg)
   cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING);
   ...
   spin_lock_bh(&so->rx_lock); // blocks

When Thread A resumes in err_out_drop_locked, it checks tx_gen. Since Thread
B is blocked, tx_gen hasn't been bumped yet. Will Thread A then incorrectly
pass the check and reset the state to ISOTP_IDLE?

Thread A (err_out_drop_locked)
   if (READ_ONCE(so->tx_gen) == gen) {
       so->tx.state = ISOTP_IDLE;
   }

Once Thread A drops the lock, Thread B will bump tx_gen and start writing to
tx.buf. But because the state is now ISOTP_IDLE, Thread D could successfully
call isotp_sendmsg() and concurrently claim the socket.

Could this lead to Threads B and D concurrently modifying tx.idx in
isotp_fill_dataframe(), causing an out-of-bounds read that leaks adjacent
kernel memory over the CAN bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3