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

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

Pre-existing issues:
- [High] A race condition between `isotp_release()` and a device network namespace move (or unregistration) causes `isotp_release()` to skip calling `can_rx_unregister()`. This leaves a dynamically allocated `struct receiver` linked in the CAN device's `dev_rcv_lists`, holding a dangling pointer to the socket `sk` which is subsequently freed. When the device receives packets in the new network namespace, this triggers a use-after-free on the socket.
--

--- Patch [1]: [PATCH v5 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit f75831bce5f2c912b988e63e3d43f06bda5b25c6
Author: Oliver Hartkopp <[email protected]>

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

isotp_notify() unregisters the CAN filters and clears so->bound without waiting
for a grace period. This commit updates isotp_release() to always call
synchronize_rcu() before cancelling timers to prevent a use-after-free timer
callback on a freed socket.

Closes: 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);

[Severity: High]
This is a pre-existing issue, but does this code have a race condition when a
device is moved to another network namespace?

If a CAN device is moved to a new namespace, dev_get_by_index() called
earlier in isotp_release() will return NULL:

    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {
        if (isotp_register_rxid(so))
            can_rx_unregister(net, dev, so->rxid, ...

This causes isotp_release() to silently skip calling can_rx_unregister().

At the same time, isotp_release() prematurely removes the socket from
isotp_notifier_list. As a result, the notifier iteration skips the socket:

isotp_notifier() {
    ...
    list_for_each_entry(isotp_busy_notifier, &isotp_notifier_list, notifier) {
        spin_unlock(&isotp_notifier_lock);
        isotp_notify(isotp_busy_notifier, msg, dev);
    ...
}

This prevents the fallback unregistration in isotp_notify().

This leaves a dynamically allocated struct receiver linked in the CAN
device's dev_rcv_lists with a dangling pointer to the socket, which is
subsequently freed.

When the device receives packets in the new network namespace, could this
trigger a use-after-free on the socket?

> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* 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