Re: [PATCH v3 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:

New issues:
- [High] Unconditional `synchronize_rcu()` in `isotp_release()` introduces a local Denial of Service vulnerability.

Pre-existing issues:
- [Critical] Race condition between `isotp_release()` and `NETDEV_UNREGISTER` leads to a permanently leaked CAN filter and a critical Use-After-Free.
--

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

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

This commit forces an unconditional synchronize_rcu() in isotp_release()
regardless of whether the socket was bound. This is intended to close a race
condition with NETDEV_UNREGISTER clearing so->bound and cancelling timers
while an in-flight isotp_rcv() is executing.

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: Critical]
This is a pre-existing issue, but can a race condition between isotp_release()
and namespace migration lead to a leaked CAN filter and a use-after-free?

If a CAN device is moved to a different network namespace while concurrently
closing a bound ISOTP socket, dev_change_net_namespace() calls
unlist_netdevice(dev), synchronize_net(), and then broadcasts
NETDEV_UNREGISTER.

If isotp_release() runs during synchronize_net(), dev_get_by_index() returns
NULL because the device is already unlisted:

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

This causes isotp_release() to silently skip can_rx_unregister(). When
NETDEV_UNREGISTER is subsequently broadcast, isotp_notify() skips the socket
since it is no longer in the list. Does this leave the CAN filters in the
device's dev_rcv_lists with a dangling pointer to the freed socket, allowing
a received CAN frame to trigger a use-after-free in isotp_rcv() when the
device is brought up in the new netns?


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

[Severity: High]
Does moving synchronize_rcu() outside the bound-checking block introduce a
denial of service vector for unbound sockets?

Because the wait is now unconditional, it is executed for every closed
socket. Unbound sockets never arm timers. Could an unprivileged user
trivially write a loop that repeatedly creates and closes unbound CAN_ISOTP
sockets to spam the RCU subsystem with back-to-back grace period requests?

Would skipping this wait via a flag indicating if the socket was ever bound
prevent this performance degradation?

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