Re: [PATCH v6 5/5] can: isotp: fix state machine corruption on signal interruption

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

Pre-existing issues:
- [High] Signal interruption of a transmission (e.g., during CAN_ISOTP_WAIT_TX_DONE) or a TX timeout aborts an active transmission by forcing state to ISOTP_IDLE but fails to clear `so->cfecho`. This exposes a race condition with delayed local loopback echoes that can leak uninitialized kernel heap memory or corrupt subsequent transmissions.
--

--- Patch [5]: [PATCH v6 5/5] can: isotp: fix state machine corruption on signal interruption ---
commit 0fb83cf387b5c9feb7682bf6c41fda7dbcb7b3d2
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix state machine corruption on signal interruption

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. Since this thread never owned the active transmission, it
abruptly aborts another thread's legitimate transfer, so return the
interruption error directly.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 0826ae8372467..b8562f8c6de04 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1007,7 +1007,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		/* wait for complete transmission of current pdu */
>  		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>  		if (err)
> -			goto err_event_drop;
> +			return err;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does resetting the state to 
ISOTP_IDLE without clearing so->cfecho expose a race condition with delayed 
local loopback echoes?

For example, if a transmission is interrupted during CAN_ISOTP_WAIT_TX_DONE, 
the wait loop is aborted and the state is reset:

net/can/isotp.c:isotp_sendmsg() {
    ...
err_event_drop:
	/* got signal: force tx state machine to be ISOTP_IDLE */
	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;
	wake_up_interruptible(&so->wait);

	return err;
}

Similarly, a TX timeout resets the state without clearing so->cfecho:

net/can/isotp.c:isotp_tx_timer_handler() {
    ...
	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 */
    ...
}

If a user starts a new transmission with a payload larger than so->tx.buflen,
the kernel sets so->tx.state to ISOTP_SENDING and allocates a new uninitialized
buffer. 

If a delayed loopback echo from the aborted transmission arrives at this time, 
could it be erroneously processed by the echo handler?

net/can/isotp.c:isotp_rcv_echo() {
    ...
	/* local echo skb with consecutive frame has been consumed */
	so->cfecho = 0;

	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
	 * hrtimer_cancel() above waited for it to finish, already reporting
	 * the tx error and resetting the state; don't resume a tx job that
	 * has already been given up on.
	 */
	if (so->tx.state != ISOTP_SENDING)
		return;

	if (so->tx.idx >= so->tx.len) {
		/* we are done */
    ...
}

Because so->cfecho still matches and so->tx.state is now ISOTP_SENDING, could
this result in transmitting uninitialized kernel heap memory over the CAN bus
by continuing transmission using the old so->tx.idx and so->tx.len but the new
uninitialized so->tx.buf?

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