Re: [PATCH v4 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

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

Pre-existing issues:
- [High] Race condition during network device unregistration causes `isotp_release()` to skip CAN filter unregistration, leading to a memory leak and a Use-After-Free.
- [High] Data race on `so->rx.state` and `so->tx.state` between hrtimer callbacks and socket operations.
--

--- Patch [1]: [PATCH v4 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit 6feb0d6dc5d6600aa28544bf14c75b5f268d9b41
Author: Oliver Hartkopp <[email protected]>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister()
and clears so->bound without waiting for a grace period. isotp_release()
uses so->bound to decide whether it needs to call synchronize_rcu()
before cancelling so->rxtimer. When NETDEV_UNREGISTER runs first, it
skips that synchronize_rcu() and can cancel the timer while an in-flight
isotp_rcv() is executing, leading to a use-after-free timer callback.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);
> -				synchronize_rcu();
>  			}
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but could this cause a memory leak
and a Use-After-Free during concurrent network device unregistration?

In isotp_release(), the socket is prematurely removed from the notifier list:

    list_del(&so->notifier);

If the device is concurrently being unregistered, this subsequent lookup fails:

    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {

This failure causes the critical can_rx_unregister() steps to be skipped.

Since the socket is no longer on the notifier list, the subsequent
NETDEV_UNREGISTER broadcast will not call isotp_notify() to clean up the
filters either.

The socket memory is subsequently freed, leaving the stale filter in the
device's receiver list, which retains a dangling pointer to the socket,
leading to a Use-After-Free when a delayed CAN frame is processed.

[Severity: High]
This is a pre-existing issue, but does this non-atomic assignment race with
concurrent protocol state machine transitions?

In isotp_rx_timer_handler():

    so->rx.state = ISOTP_IDLE;

This modifies the RX state without holding so->rx_lock, racing with
concurrent CAN frame reception in sendmsg and softirq RX handlers.

For example, isotp_rcv() acquires the lock and begins processing a frame.
If isotp_rcv_cf() observes ISOTP_WAIT_DATA, it proceeds to cancel the timer.
hrtimer_cancel() blocks until the racing timer callback finishes. Once the
timer finishes, it has already changed the state to ISOTP_IDLE and signaled
an ETIMEDOUT error. However, isotp_rcv_cf() then resumes and processes the
frame into the buffer as if the state were still valid, hopelessly corrupting
the sequence.

A similar data race occurs on so->tx.state in isotp_tx_timer_handler():

    so->tx.state = ISOTP_IDLE;

This uses plain unprotected writes against concurrent atomic operations
(cmpxchg) and RX softirq modifications, leading to unexpected packet drops,
infinite wait stalls, and protocol failure.

> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

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