Re: [PATCH v5 4/4] can: isotp: fix lock-free state transition in tx timer handler

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

New issues:
- [Medium] Missing READ_ONCE() on lockless state read allows compiler load tearing to defeat cmpxchg protection

Pre-existing issues:
- [High] TOCTOU vulnerability in RX handlers allows state machine corruption and out-of-bounds reads
- [High] Unsafe teardown order in error path releases state machine before canceling async timers
- [High] Premature state release causes misattribution of timeout errors to concurrent transmissions
--

--- Patch [4]: [PATCH v5 4/4] can: isotp: fix lock-free state transition in tx timer handler ---
commit 1142eb2123699c3ef59ba2ca431beca7fc493cd1
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix lock-free state transition in tx timer handler

Commit 051737439eae ("can: isotp: fix race between isotp_sendsmg() and
isotp_release()") introduced a lock-free state machine check
to prevent race conditions between the TX timer and concurrent state
updates. However, the original patch missed replacing the initial
state checks and left the late assignment of ISOTP_IDLE as a blind,
non-atomic write.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c0edbd35516cc..e49a58e188e6b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -922,9 +922,16 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
>  					     txtimer);
>  	struct sock *sk = &so->sk;
> +	u32 old_state = so->tx.state;

[Severity: Medium]
Could this lockless state read be subject to compiler load tearing without a
READ_ONCE()?

If the compiler decides to reload so->tx.state directly into the cmpxchg
macro arguments under register pressure, and the state changes concurrently
(e.g., to ISOTP_SENDING), wouldn't the reloaded value cause cmpxchg to compare
the new state against itself?

Could this blind success erroneously abort the new transmission?

>  
>  	/* 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 timeout if state is unchanged to avoid overwriting
> +	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
> +	 */
> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)

[Severity: High]
This isn't a bug introduced by this patch, but does releasing the state
machine ownership here cause misattribution of timeout errors to concurrent
transmissions?

Since the state is transitioned to ISOTP_IDLE before setting sk->sk_err and
calling wake_up_interruptible(), could a concurrent sender claim ISOTP_SENDING
in this window?

If so, when the wait queue is woken, the original timed-out thread might wake
up, see the state is no longer ISOTP_IDLE, and go back to sleep.

Could the new sender later complete successfully but read the stale
sk->sk_err = ECOMM left by the timer, consuming the error, while the original
thread eventually wakes up and incorrectly reads success?

>  		return HRTIMER_NORESTART;
>  
>  	/* we did not get any flow control or echo frame in time */
> @@ -934,8 +941,6 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	if (!sock_flag(sk, SOCK_DEAD))
>  		sk_error_report(sk);
>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;

[Severity: High]
This is a pre-existing issue, but while reviewing the state machine changes,
is there a TOCTOU vulnerability in the RX handlers?

Looking at isotp_rcv_fc() in net/can/isotp.c:

    if (so->tx.state != ISOTP_WAIT_FC && ...)
        return 0;
    hrtimer_cancel(&so->txtimer);
    // ... blindly proceeds to send

The state is checked before the blocking hrtimer_cancel() and never
re-verified.

If a concurrent timeout aborts the transmission (setting state to ISOTP_IDLE)
while the RX thread is blocked in hrtimer_cancel(), and a new thread enters
isotp_sendmsg(), claims ISOTP_SENDING, and begins mutating so->tx.buf, what
happens when hrtimer_cancel() unblocks?

Would the RX handler blindly assume its earlier state check is still valid,
overwrite the state, and call isotp_send_cframe(), reading from the new
thread's partially initialized buffer?

A similar pattern appears in isotp_rcv_echo():

    if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
        return;
    hrtimer_cancel(&so->txtimer);
    // ... checks state but cfecho check was bypassed

The cfecho validation occurs before hrtimer_cancel(), allowing a concurrent
new transmission to slip past the check.

[Severity: High]
This is a pre-existing issue, but does isotp_sendmsg() have an unsafe
teardown order in its error path?

Looking at err_event_drop in isotp_sendmsg():

    err_event_drop:
        /* got signal: force tx state machine to be idle */
        so->tx.state = ISOTP_IDLE;
        hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);

If isotp_sendmsg() is interrupted by a signal, it sets so->tx.state =
ISOTP_IDLE before canceling the asynchronous timers.

Could a concurrent thread immediately enter isotp_sendmsg(), claim the state,
and start configuring so->tx?

If so, could the original thread's un-canceled txfrtimer then fire, see the
new thread's ISOTP_SENDING state, and prematurely call isotp_send_cframe() on
the new thread's incomplete buffer?

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