Re: [PATCH v7 4/5] 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:
- [High] Silent suppression of timeout errors in isotp_tx_timer_handler leads to false success reports.
- [High] isotp_sendmsg evaluates an uninitialized variable and unconditionally cancels the timers of an unrelated active transmission when interrupted by a signal.
- [Medium] Time-of-Check to Time-of-Use race in isotp_tx_timer_handler falsely taints new transfers with ECOMM.

Pre-existing issues:
- [High] Incomplete fix for lock-free state transitions leaves identical vulnerable blind writes in isotp_rcv_echo, isotp_rcv_fc, and isotp_sendmsg.
--

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

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

Commit 051737439eae 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 initial state
checks and left the late assignment of ISOTP_IDLE as a blind write.
Fix this by sampling the initial state and using cmpxchg() to
atomically move to ISOTP_IDLE, safely bailing out if the timeout is
stale.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index f05703ddd2eae..fdb2109d7c27e 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -942,20 +942,33 @@ 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 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 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)
>  		return HRTIMER_NORESTART;

[Severity: Medium]
When the timer handler successfully releases the state machine to ISOTP_IDLE
here, doesn't it open a race window?

If a concurrent isotp_sendmsg() claims the state (ISOTP_SENDING), but hasn't
yet bumped so->tx_gen, the old generation matches the check below, and the new
transfer is falsely tainted with ECOMM.

>  
>  	/* 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);
> +	/* Once the cmpxchg() above released ISOTP_IDLE, a concurrent
> +	 * sendmsg() may already have claimed it for a new transfer
> +	 * (tx_gen changed); don't taint that new transfer with an error
> +	 * that belongs to the one that just timed out.
> +	 */
> +	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]
If a concurrent isotp_sendmsg() immediately claims the socket and updates
tx_gen before this check, the ECOMM error is skipped entirely. 

Does this mean the timeout error is silently suppressed, and an application
waiting for the transfer to finish is falsely notified of success (returning
byte size instead of error)?

>  
> -	/* 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 do other paths still perform non-atomic
blind writes to ISOTP_IDLE?

For instance, in isotp_rcv_echo():

net/can/isotp.c:isotp_rcv_echo() {
    ...
	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
		return;

	if (so->tx.idx >= so->tx.len) {
		/* we are done */
		so->tx.state = ISOTP_IDLE;
		wake_up_interruptible(&so->wait);
		return;
	}
    ...
}

If this thread is preempted or delayed immediately after checking tx_gen, and
the timer handler runs and releases the state so a new transfer can start,
could the delayed thread wake up and blindly force the new transfer's state
to ISOTP_IDLE?

A similar blind write to ISOTP_IDLE occurs in isotp_rcv_fc():

net/can/isotp.c:isotp_rcv_fc() {
    ...
	default:
		/* stop this tx job */
		so->tx.state = ISOTP_IDLE;
		wake_up_interruptible(&so->wait);
	}
    ...
}


[Severity: High]
Is there a problem with uninitialized variables and unconditional timer
cancellations in the error paths of isotp_sendmsg()?

net/can/isotp.c:isotp_sendmsg() {
    ...
		/* wait for complete transmission of current pdu */
		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
		if (err)
			goto err_event_drop;
	}

	/* new transfer: bump generation and drop any echo tag left behind
	 * by a previously aborted transfer
	 */
	my_gen = so->tx_gen + 1;
    ...
err_event_drop:
	/* 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);
err_out_drop:
	/* 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;
	}
    ...
}

If wait_event_interruptible() is interrupted by a signal, it jumps to
err_event_drop. At this point, my_gen is uninitialized. The code then evaluates
if (READ_ONCE(so->tx_gen) == my_gen), which compares against uninitialized
stack memory.

Furthermore, could calling hrtimer_cancel() unconditionally here cancel the
active timers of an unrelated, ongoing transmission that currently owns the
socket?

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