[PATCH] can: isotp: add missing synchronize_rcu() in isotp_notify()

[PATCH] can: isotp: add missing synchronize_rcu() in isotp_notify()

[Oliver Hartkopp]

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, so when NETDEV_UNREGISTER runs first it
skips that synchronize_rcu() and can cancel the timer while an
in-flight isotp_rcv() is still executing and about to re-arm it via
isotp_send_fc(), leading to a use-after-free timer callback on the
freed socket.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
Reported-by: Nico Yip <zdi-disclosures@trendmicro.com> (ZDI-CAN-31764)
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c48b4a818297..008cee547213 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1573,10 +1573,17 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 						  isotp_rcv, sk);
 
 			can_rx_unregister(dev_net(dev), dev, so->txid,
 					  SINGLE_MASK(so->txid),
 					  isotp_rcv_echo, sk);
+
+			/* wait for in-flight isotp_rcv() callers to finish
+			 * before clearing so->bound, so that isotp_release()
+			 * cannot observe so->bound == 0 and skip its own
+			 * synchronize_rcu() while a callback is still running
+			 */
+			synchronize_rcu();
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
 		release_sock(sk);
-- 
2.53.0

Re: [PATCH] can: isotp: add missing synchronize_rcu() in isotp_notify()

[sashiko-bot]

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

New issues:
- [High] System-wide RTNL lock stall and DoS due to O(N) sequential synchronize_rcu() calls in a netdevice notifier.

Pre-existing issues:
- [High] Race condition in isotp_release() and bcm_release() during concurrent device unregistration leads to skipped filter unregistration, leaking receiver entries and causing UAF on CAN frame reception.
--

--- Patch [1]: [PATCH] can: isotp: add missing synchronize_rcu() in isotp_notify() ---
commit e2052e358a8bcbb3b1bce1aa1280f280d7e67f64
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: add missing synchronize_rcu() in isotp_notify()

isotp_notify() unregisters the CAN filters without waiting for an RCU grace
period. This commit adds a synchronize_rcu() to wait for in-flight isotp_rcv()
callers to finish before clearing so->bound. This prevents a use-after-free
timer callback on a freed socket if isotp_release() skips its own
synchronize_rcu() when NETDEV_UNREGISTER runs first.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..008cee5472130 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1575,6 +1575,13 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
>  			can_rx_unregister(dev_net(dev), dev, so->txid,
>  					  SINGLE_MASK(so->txid),
>  					  isotp_rcv_echo, sk);
> +
> +			/* wait for in-flight isotp_rcv() callers to finish
> +			 * before clearing so->bound, so that isotp_release()
> +			 * cannot observe so->bound == 0 and skip its own
> +			 * synchronize_rcu() while a callback is still running
> +			 */
> +			synchronize_rcu();

[Severity: High]
Does this regression introduce an O(N) synchronize_rcu() stall under the
rtnl_lock?

Looking at isotp_notifier(), it loops over all sockets and calls isotp_notify():

net/can/isotp.c: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);
        ...
}

Because netdevice notifiers are invoked with the rtnl_lock held, iterating over
thousands of sockets bound to the interface will trigger sequential RCU grace
periods. Can this stall the rtnl_lock for minutes and freeze network
configuration tasks?


[Severity: High]
This is a pre-existing issue, but does a race condition in isotp_release()
also lead to skipped filter unregistration and a use-after-free?

If a socket is closed exactly while the device it is bound to is being
unregistered, dev_get_by_index() can return NULL because unlist_netdevice()
is called before the NETDEV_UNREGISTER notifier fires:

net/core/dev.c:unregister_netdevice_many_notify() {
    ...
    unlist_netdevice(dev);
    ...
    synchronize_net();
    ...
    call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
    ...
}

If isotp_release() executes in this window, it removes the socket from
isotp_notifier_list, then calls dev_get_by_index() which returns NULL:

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

As a result, isotp_release() completely skips filter unregistration and the
synchronize_rcu() call. The notifier will subsequently skip this socket
because it is no longer in the list. The CAN receive filter remains active
and points to the socket which is then freed.

Could this cause a use-after-free if an isotp_rcv() callback is currently
in-flight or triggered by pending loopback frames? The same logic flaw
appears to exist in bcm_release().

>  		}
>  
>  		so->ifindex = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707083228.47629-1-socketcan@hartkopp.net?part=1