Skip to contentSkip to conversation

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

PATCH New Oliver Hartkopp 2 participants 2 messages Jul 7

Subsystems (2)
CAN network drivers CAN network layer
lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

PATCH 1/1 can: isotp: add missing synchronize_rcu() in isotp_notify()

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 <[email protected]> (ZDI-CAN-31764)
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/isotp.c | 7 +++++++
 1 file changed, 7 insertions(+)
net/can/isotp.c +7 -0
index c48b4a818297..008cee547213 100644
@@ -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);
Signature · 1 line
2.53.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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 <[email protected]>

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.

Quoted 5 lines
net/can/isotp.c +0 -0
index c48b4a818297e..008cee5472130 100644

net/can/isotp.c static void isotp_notify(struct isotp_sock *so, unsigned long msg, · line 1584

Quoted 10 lines
net/can/isotp.c +7 -0
@@ -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().

Quoted 3 lines
net/can/isotp.c +0 -0
 		}
 
 		so->ifindex = 0;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1