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

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

New issues:
- [Medium] Unconditional synchronize_rcu() in socket release path allows unprivileged users to spam RCU grace periods.

Pre-existing issues:
- [Critical] Missing rtnl_lock() in isotp_bind() allows race with NETDEV_UNREGISTER, leading to leaked CAN RX filters and a use-after-free on the socket.
--

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

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

This commit addresses a use-after-free timer callback on a freed socket. It
makes isotp_release() unconditionally call synchronize_rcu() before cancelling
timers, regardless of so->bound, to prevent races with NETDEV_UNREGISTER.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
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)
[ ... ]
>  				dev_put(dev);
> -				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();

[Severity: Medium]
Will this unconditional synchronize_rcu() allow unprivileged users to spam RCU
grace periods?

If an unprivileged user repeatedly creates and immediately closes unbound CAN
ISOTP sockets, it seems this path will be hit every time. While previously this
required a successful bind to a valid CAN interface, this patch makes the call
unconditional, which might allow it to be triggered on any system with the CAN
ISOTP module available.

> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

[Severity: Critical]
This is a pre-existing issue, but does isotp_bind() lack rtnl_lock()
protection?

Looking at isotp_bind(), it acquires lock_sock() but does not appear to acquire
rtnl_lock(). Can this allow a race with device unregistration?

net/can/isotp.c:isotp_bind() {
    ...
	dev = dev_get_by_index(net, addr->can_ifindex);
	...
	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, "isotp", sk);
    ...
}

If an unprivileged user concurrently binds a CAN ISOTP socket to a virtual CAN
device while deleting the device, it seems this could race with the device
unregistration notifier in isotp_notify().

If the notifier runs before so->ifindex is set, it might fail to unregister the
filters, allowing isotp_bind() to register the filters on the dying device.

When the socket is closed, isotp_release() handles it:

net/can/isotp.c:isotp_release() {
    ...
		dev = dev_get_by_index(net, so->ifindex);
		if (dev) {
			if (isotp_register_rxid(so))
				can_rx_unregister(net, dev, so->rxid,...);
    ...
}

If dev_get_by_index() fails because the device is now unlisted, it appears the
CAN RX filters are never unregistered, which could leave a dangling
struct receiver in the device's dev_rcv_lists.

If a delayed packet is processed before the device is freed, could isotp_rcv()
be invoked with the freed socket pointer, resulting in a use-after-free?

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