Re: [PATCH v5 3/4] can: isotp: fix race between RX timer and consecutive frame reception

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

Pre-existing issues:
- [High] Missing state validation after hrtimer_cancel in the TX path allows resurrection of aborted transmissions and state machine corruption.
--

--- Patch [3]: [PATCH v5 3/4] can: isotp: fix race between RX timer and consecutive frame reception ---
commit 8ee7739c2adf82e961ff6ad45d8c572723ba74e7
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix race between RX timer and consecutive frame reception

When receiving a Consecutive Frame (CF), hrtimer_cancel() is called to stop
the RX timer. The patch verifies that the state is still ISOTP_WAIT_DATA after
canceling the hrtimer to avoid state machine corruption.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index e7597ca3e140b..c0edbd35516cc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -577,6 +577,14 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
>  
>  	hrtimer_cancel(&so->rxtimer);
>  
> +	/* isotp_rx_timer_handler() may have raced us for so->rx.state
> +	 * while hrtimer_cancel() above waited for it to finish, already
> +	 * reporting ETIMEDOUT and resetting the reception; don't process
> +	 * this CF into a reassembly that has already been given up on.
> +	 */
> +	if (so->rx.state != ISOTP_WAIT_DATA)
> +		return 1;
> +

[Severity: High]
This is a pre-existing issue, but does a similar race condition exist in the
TX paths?

Looking at isotp_rcv_fc(), we call hrtimer_cancel() on the txtimer without
validating the TX state afterwards:

net/can/isotp.c:isotp_rcv_fc() {
    ...
    hrtimer_cancel(&so->txtimer);

    if ((cf->len < ae + FC_CONTENT_SZ) ||
    ...
}

Similarly in isotp_rcv_echo():

net/can/isotp.c:isotp_rcv_echo() {
    ...
    /* cancel local echo timeout */
    hrtimer_cancel(&so->txtimer);

    /* local echo skb with consecutive frame has been consumed */
    so->cfecho = 0;
    ...
}

If a flow control or echo CAN frame is received at the exact same time the
TX timer expires, could isotp_rcv_fc() or isotp_rcv_echo() block in
hrtimer_cancel() waiting for isotp_tx_timer_handler() to finish?

If the timer handler sets so->tx.state = ISOTP_IDLE and fails the socket,
could the receiver functions then blindly resume the transmission,
potentially resurrecting an aborted transfer and corrupting the state
machine?

Furthermore, since the state would briefly be ISOTP_IDLE, might a concurrent
sendmsg() succeed, resulting in two threads concurrently modifying the TX
buffer and state?

>  	/* CFs are never longer than the FF */
>  	if (cf->len > so->rx.ll_dl)
>  		return 1;

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