Re: [PATCH] can: isotp: hold a reference to the bound netdevice

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling synchronize_rcu() while holding rtnl_lock() introduces severe latency spikes and potential denial of service system-wide.

Pre-existing issues:
- [Critical] Skipping synchronize_rcu() in isotp_release() when the socket is unbound leads to a use-after-free of the socket structure by re-armed hrtimers.
--

commit da93bcd8877d85ffe0b043e3045c3374b10e38e5
Author: Vasileios Almpanis <vasilisalmpanis@gmail.com>

can: isotp: hold a reference to the bound netdevice
    
isotp_release() looked up the bound netdevice with
dev_get_by_index(so->ifindex) and only unregistered the socket's CAN
receivers if that lookup succeeded. This is unreliable while the device
is going away.

Cc: stable@vger.kernel.org
Closes: https://syzkaller.appspot.com/bug?id=58676a0f698531996a42612c552e894a55b9732b

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..308d18040b6db 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1219,27 +1222,22 @@ static int isotp_release(struct socket *sock)
>  	list_del(&so->notifier);
>  	spin_unlock(&isotp_notifier_lock);
>  
> +	rtnl_lock();
>  	lock_sock(sk);
>  
>  	/* remove current filters & unregister */
> -	if (so->bound) {
[ ... ]
> +	if (so->bound && so->dev) {
> +		if (isotp_register_rxid(so))
> +			can_rx_unregister(net, so->dev, so->rxid,
> +					  SINGLE_MASK(so->rxid),
> +					  isotp_rcv, sk);
> +
> +		can_rx_unregister(net, so->dev, so->txid,
> +				  SINGLE_MASK(so->txid),
> +				  isotp_rcv_echo, sk);
> +		netdev_put(so->dev, &so->dev_tracker);
> +		so->dev = NULL;
> +		synchronize_rcu();

[Severity: High]
Does holding rtnl_lock() in isotp_release() (net/can/isotp.c) across
synchronize_rcu() cause severe latency spikes and potential denial of
service system-wide?

synchronize_rcu() blocks until all currently active RCU read-side critical
sections across the system finish, which stalls all other network
configuration operations while rtnl_lock() is held.

[Severity: Critical]
This is a pre-existing issue, but skipping synchronize_rcu() when the socket
is already unbound (e.g., by NETDEV_UNREGISTER where isotp_notify() sets
so->bound = 0) appears to allow a use-after-free.

If synchronize_rcu() is bypassed, isotp_release() proceeds to cancel the
socket's hrtimers and drop the user reference without waiting for concurrent
receive operations to complete.

Could a concurrent CAN receive operation like isotp_rcv_ff() or
isotp_rcv_echo() executing within an RCU read-side critical section
unconditionally re-arm the hrtimer after isotp_release() has cancelled it,
leading to a use-after-free when the timer later fires on the freed socket
memory?

>  	}
>  
>  	hrtimer_cancel(&so->txfrtimer);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616123915.674534-1-vasilisalmpanis@gmail.com?part=1