[PATCH can-next v2 0/3] can: take rtnl_lock() before leaving the notifier list

[PATCH can-next v2 0/3] can: take rtnl_lock() before leaving the notifier list

[Oliver Hartkopp]

Norbert Szetei reported a use-after-free in isotp: isotp_release()
removes the socket from the netdev notifier list before it takes
rtnl_lock(). A socket that leaves the list in this window is skipped by
the notifier and has to remove its CAN filters itself. If the bound
device is moved to another network namespace in this window,
can_rx_unregister() silently skips the removal due to the namespace
mismatch and the remaining receivers point to the freed socket.

bcm has the same window and is affected in the same way. raw has the
window too, but removes its filters with dev_net(ro->dev) under RTNL
and is therefore not affected.

This series takes rtnl_lock() before the socket leaves the notifier
list in all three protocols. As the notifier chain is always invoked
under RTNL, the busy-wait loops on the *_busy_notifier variables are
dead code and are replaced with WARN_ON_ONCE().

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/

Changes in v2:
- bcm: remove the procfs entry and the tx_ops before taking RTNL so
  that remove_proc_entry() can not stall RTNL waiting for readers of
  the world-readable procfs entry
- bcm, raw: reword the commit messages so that they do not refer to
  patches outside of this series
- send all three patches as one series for can-next

Oliver Hartkopp (3):
  can: isotp: fix filter removal race between release and netns change
  can: bcm: fix filter removal race between release and netns change
  can: raw: take rtnl_lock() before leaving the notifier list

 net/can/bcm.c   | 35 +++++++++++++++++++++++++----------
 net/can/isotp.c | 17 +++++++++--------
 net/can/raw.c   | 13 +++++++------
 3 files changed, 41 insertions(+), 24 deletions(-)

-- 
2.53.0

[PATCH can-next v2 1/3] can: isotp: fix filter removal race between release and netns change

[Oliver Hartkopp]

isotp_release() removes the socket from isotp_notifier_list before it
takes rtnl_lock(). As the netdev notifier chain runs under RTNL, a
socket that leaves the list in this window is skipped by isotp_notify()
and has to remove its CAN filters itself.

This fails when the bound device is moved to another network namespace
in this window: isotp_release() passes sock_net(sk) to
can_rx_unregister(), which silently returns when this namespace does not
match dev_net(dev) anymore. As can_rx_register() stores rcv->sk without
taking a reference, the remaining receivers point to the freed socket
and travel with the device into the new namespace:

  BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0
  Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578
   can_rcv_filter+0x4af/0x8c0
   can_receive+0x28d/0x3c0
   can_rcv+0x2a9/0x310
   __netif_receive_skb_one_core+0x21a/0x260
   process_backlog+0x210/0x760

Take rtnl_lock() before removing the socket from the notifier list, so
that isotp_release() and isotp_notify() can not both skip the removal.

As the notifier chain is always invoked under RTNL (see ASSERT_RTNL() in
call_netdevice_notifiers_info()), isotp_busy_notifier can not point to
the released socket anymore. Replace the now dead busy-wait loop with a
WARN_ON_ONCE(). isotp_notifier_lock is still needed, as isotp_init()
adds new sockets to isotp_notifier_list without holding RTNL.

Additionally use dev_net(so->dev) for the can_rx_unregister() calls, as
the CAN filters live in the device's namespace. With the new locking
both namespaces are equal there, but dev_net(so->dev) is stable under
RTNL and always names the correct namespace.

Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
Reported-by: Norbert Szetei <norbert@doyensec.com>
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index f5dc9d04bd68..186daa818647 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1477,32 +1477,33 @@ static int isotp_release(struct socket *sock)
 	WRITE_ONCE(so->rx.state, ISOTP_IDLE);
 
 	/* forced SHUTDOWN may have skipped IDLE (gave up on a signal) */
 	wake_up_interruptible(&so->wait);
 
+	rtnl_lock();
+
+	/* The netdev notifier chain runs under RTNL, so isotp_notifier() can't
+	 * be walking isotp_notifier_list and point to this socket. The spinlock
+	 * is still needed as isotp_init() adds sockets without holding RTNL.
+	 */
 	spin_lock(&isotp_notifier_lock);
-	while (isotp_busy_notifier == so) {
-		spin_unlock(&isotp_notifier_lock);
-		schedule_timeout_uninterruptible(1);
-		spin_lock(&isotp_notifier_lock);
-	}
+	WARN_ON_ONCE(isotp_busy_notifier == so);
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
 	/* remove current filters & unregister
 	 * tracked reference so->dev is taken at bind() time with rtnl_lock
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
-			can_rx_unregister(net, so->dev, so->rxid,
+			can_rx_unregister(dev_net(so->dev), so->dev, so->rxid,
 					  SINGLE_MASK(so->rxid),
 					  isotp_rcv, sk);
 
-		can_rx_unregister(net, so->dev, so->txid,
+		can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
 				  SINGLE_MASK(so->txid),
 				  isotp_rcv_echo, sk);
 		netdev_put(so->dev, &so->dev_tracker);
 	}
 
-- 
2.53.0

[PATCH can-next v2 2/3] can: bcm: fix filter removal race between release and netns change

[Oliver Hartkopp]

bcm_release() removes the socket from bcm_notifier_list before it takes
any lock that serializes against the netdev notifier chain, which runs
under RTNL. A socket that leaves the list in this window is skipped by
bcm_notify() and has to remove its CAN filters itself.

This fails when the bound device is moved to another network namespace
in this window: bcm_rx_unreg() reads dev_net(dev) without holding RTNL,
and can_rx_unregister() silently returns when this namespace does not
match dev_net(dev) anymore. The op is freed anyway and the remaining
receiver leads to a use-after-free in bcm_rx_handler().

Take rtnl_lock() before removing the socket from the notifier list, so
that bcm_release() and bcm_notify() can not both skip the removal and
dev_net(dev) is stable while the filters are removed. RTNL is dropped
before synchronize_rcu(). As the notifier chain is always invoked under
RTNL, bcm_busy_notifier can not point to the released socket anymore.
Replace the now dead busy-wait loop with a WARN_ON_ONCE().

RTNL is only needed for the notifier list removal and the rx_ops
unregistration. Remove the procfs entry and the tx_ops before taking
RTNL, as remove_proc_entry() waits for concurrent readers of the
world-readable procfs entry, which could otherwise stall RTNL from a
plain close(). bo->bcm_proc_read is cleared so that a NETDEV_UNREGISTER
in bcm_notify() does not remove the entry a second time.

Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: Norbert Szetei <norbert@doyensec.com>
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..f439bdbf37b1 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1924,32 +1924,45 @@ static int bcm_release(struct socket *sock)
 	net = sock_net(sk);
 	bo = bcm_sk(sk);
 
 	/* remove bcm_ops, timer, rx_unregister(), etc. */
 
-	spin_lock(&bcm_notifier_lock);
-	while (bcm_busy_notifier == bo) {
-		spin_unlock(&bcm_notifier_lock);
-		schedule_timeout_uninterruptible(1);
-		spin_lock(&bcm_notifier_lock);
-	}
-	list_del(&bo->notifier);
-	spin_unlock(&bcm_notifier_lock);
-
+	/* Remove the procfs entry and the tx_ops before taking RTNL:
+	 * remove_proc_entry() waits for concurrent readers of the entry and
+	 * must not hold up RTNL for them. Clear bo->bcm_proc_read so that
+	 * bcm_notify() does not remove the entry a second time.
+	 */
 	lock_sock(sk);
 
 #if IS_ENABLED(CONFIG_PROC_FS)
 	/* remove procfs entry */
-	if (net->can.bcmproc_dir && bo->bcm_proc_read)
+	if (net->can.bcmproc_dir && bo->bcm_proc_read) {
 		remove_proc_entry(bo->procname, net->can.bcmproc_dir);
+		bo->bcm_proc_read = NULL;
+	}
 #endif /* CONFIG_PROC_FS */
 
 	list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
 		list_del_rcu(&op->list);
 		bcm_remove_op(op);
 	}
 
+	release_sock(sk);
+
+	rtnl_lock();
+
+	/* The netdev notifier chain runs under RTNL, so bcm_notifier() can't
+	 * be walking bcm_notifier_list and point to this socket. The spinlock
+	 * is still needed as bcm_init() adds sockets without holding RTNL.
+	 */
+	spin_lock(&bcm_notifier_lock);
+	WARN_ON_ONCE(bcm_busy_notifier == bo);
+	list_del(&bo->notifier);
+	spin_unlock(&bcm_notifier_lock);
+
+	lock_sock(sk);
+
 	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
 		/*
 		 * Don't care if we're bound or not (due to netdev problems)
 		 * can_rx_unregister() is always a save thing to do here.
 		 */
@@ -1971,10 +1984,12 @@ static int bcm_release(struct socket *sock)
 					  REGMASK(op->can_id),
 					  bcm_rx_handler, op);
 
 	}
 
+	rtnl_unlock();
+
 	synchronize_rcu();
 
 	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
 		list_del_rcu(&op->list);
 		bcm_remove_op(op);
-- 
2.53.0

[PATCH can-next v2 3/3] can: raw: take rtnl_lock() before leaving the notifier list

[Oliver Hartkopp]

raw_release() removes the socket from raw_notifier_list before it takes
rtnl_lock(). As the netdev notifier chain runs under RTNL, a socket that
leaves the list in this window is skipped by raw_notify() and has to
remove its CAN filters itself.

For raw this is not harmful: raw_release() removes the filters with
dev_net(ro->dev) under RTNL, so they are found even when the bound
device has been moved to another network namespace in this window. The
receivers only remain on the moved device until raw_release() gets
RTNL.

Take rtnl_lock() before removing the socket from the notifier list, so
that raw, isotp and bcm use the same locking scheme in their release
functions. As the notifier chain is always invoked under RTNL,
raw_busy_notifier can not point to the released socket anymore. Replace
the now dead busy-wait loop with a WARN_ON_ONCE().

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/raw.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/can/raw.c b/net/can/raw.c
index 0acd4f6c6dd6..925c0c1d243f 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -420,20 +420,21 @@ static int raw_release(struct socket *sock)
 		return 0;
 
 	ro = raw_sk(sk);
 	net = sock_net(sk);
 
+	rtnl_lock();
+
+	/* The netdev notifier chain runs under RTNL, so raw_notifier() can't
+	 * be walking raw_notifier_list and point to this socket. The spinlock
+	 * is still needed as raw_init() adds sockets without holding RTNL.
+	 */
 	spin_lock(&raw_notifier_lock);
-	while (raw_busy_notifier == ro) {
-		spin_unlock(&raw_notifier_lock);
-		schedule_timeout_uninterruptible(1);
-		spin_lock(&raw_notifier_lock);
-	}
+	WARN_ON_ONCE(raw_busy_notifier == ro);
 	list_del(&ro->notifier);
 	spin_unlock(&raw_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
 	/* remove current filters & unregister */
 	if (ro->bound) {
 		if (ro->dev) {
-- 
2.53.0

Re: [PATCH can-next v2 0/3] can: take rtnl_lock() before leaving the notifier list

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

On 14.10.2026 16:21:11, Oliver Hartkopp wrote:
> Norbert Szetei reported a use-after-free in isotp: isotp_release()
> removes the socket from the netdev notifier list before it takes
> rtnl_lock(). A socket that leaves the list in this window is skipped by
> the notifier and has to remove its CAN filters itself. If the bound
> device is moved to another network namespace in this window,
> can_rx_unregister() silently skips the removal due to the namespace
> mismatch and the remaining receivers point to the freed socket.
>
> bcm has the same window and is affected in the same way. raw has the
> window too, but removes its filters with dev_net(ro->dev) under RTNL
> and is therefore not affected.
>
> This series takes rtnl_lock() before the socket leaves the notifier
> list in all three protocols. As the notifier chain is always invoked
> under RTNL, the busy-wait loops on the *_busy_notifier variables are
> dead code and are replaced with WARN_ON_ONCE().
>
> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/

Applied to linux-can-next.

Thanks,
Marc

pw-bot: accepted

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]