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

[PATCH can-next v3 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().

For bcm, RTNL is only held for the notifier list removal and the
removal of the device specific filters. The procfs entry, the tx_ops
and the 'any' CAN device filters are removed after RTNL is released,
when bcm_notify() can not access the socket anymore.

Tested with PROVE_LOCKING, KASAN and DEBUG_ATOMIC_SLEEP while creating,
binding and releasing raw/bcm/isotp sockets on a vcan device that is
continuously moved between two network namespaces, set down and
re-created, with concurrent CAN traffic and bcm procfs readers.
A bcm procfs reader stalled via userfaultfd does not block RTNL while
bcm_release() waits for it in remove_proc_entry(), also not with a
concurrent removal of a CAN netdev in the socket's namespace.

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

Changes in v3:
- bcm: remove the socket from the notifier list and the device specific
  filters under RTNL first. Then remove the procfs entry, the tx_ops and
  the 'any' CAN device filters without RTNL. In v2 a stalled procfs
  reader could still block bcm_notify() (under RTNL) on lock_sock()
  while bcm_release() waited in remove_proc_entry(), and the 'any' CAN
  device filters were removed under RTNL.
  Clearing bo->bcm_proc_read in bcm_release() is not needed anymore.
- bcm: replace the stale comment about a concurrent NETDEV_UNREGISTER
  in the rx_ops loop of bcm_release()

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   | 47 +++++++++++++++++++++--------------------------
 net/can/isotp.c | 17 +++++++++--------
 net/can/raw.c   | 13 +++++++------
 3 files changed, 37 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH can-next v3 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 f5dc9d0..186daa8 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1479,16 +1479,17 @@ static int isotp_release(struct socket *sock)
 	/* 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
@@ -1496,11 +1497,11 @@ static int isotp_release(struct socket *sock)
 	 */
 	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.43.0

[PATCH can-next v3 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 device specific filters are removed.
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 held for the notifier list removal and the removal of the
device specific filters. Once the socket has left the notifier list,
bcm_notify() can not access it anymore. So the procfs entry, the tx_ops
and the filters for 'any' CAN device, which live in the socket's
namespace, are removed after RTNL has been released. This way a stalled
reader of the world-readable procfs entry can not hold up RTNL, and the
removal of the 'any' CAN device filters does not delay RTNL anymore.

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 | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1..b1bb3db 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1926,17 +1926,30 @@ static int bcm_release(struct socket *sock)
 
 	/* remove bcm_ops, timer, rx_unregister(), etc. */
 
+	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);
-	while (bcm_busy_notifier == bo) {
-		spin_unlock(&bcm_notifier_lock);
-		schedule_timeout_uninterruptible(1);
-		spin_lock(&bcm_notifier_lock);
-	}
+	WARN_ON_ONCE(bcm_busy_notifier == bo);
 	list_del(&bo->notifier);
 	spin_unlock(&bcm_notifier_lock);
 
 	lock_sock(sk);
 
+	/* Remove the device specific subscriptions under RTNL */
+	list_for_each_entry(op, &bo->rx_ops, list)
+		if (op->ifindex && op->rx_reg_dev)
+			bcm_rx_unreg(op->rx_reg_dev, op);
+
+	rtnl_unlock();
+
+	/* bcm_notify() can not access this socket anymore. Release the other
+	 * resources without holding RTNL, e.g. remove_proc_entry() waits for
+	 * concurrent readers of the procfs entry.
+	 */
 #if IS_ENABLED(CONFIG_PROC_FS)
 	/* remove procfs entry */
 	if (net->can.bcmproc_dir && bo->bcm_proc_read)
@@ -1948,31 +1961,13 @@ static int bcm_release(struct socket *sock)
 		bcm_remove_op(op);
 	}
 
-	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.
-		 */
-		if (op->ifindex) {
-			/*
-			 * Only remove subscriptions that had not
-			 * been removed due to NETDEV_UNREGISTER
-			 * in bcm_notifier()
-			 *
-			 * op->rx_reg_dev is a tracked reference taken
-			 * when the subscription was registered, so it
-			 * stays valid here even if a concurrent
-			 * NETDEV_UNREGISTER already unlisted the device.
-			 */
-			if (op->rx_reg_dev)
-				bcm_rx_unreg(op->rx_reg_dev, op);
-		} else
+	/* 'any' CAN device subscriptions live in the socket's namespace */
+	list_for_each_entry(op, &bo->rx_ops, list)
+		if (!op->ifindex)
 			can_rx_unregister(net, NULL, op->can_id,
 					  REGMASK(op->can_id),
 					  bcm_rx_handler, op);
 
-	}
-
 	synchronize_rcu();
 
 	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
-- 
2.43.0

[PATCH can-next v3 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 0acd4f6..925c0c1 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -422,16 +422,17 @@ static int raw_release(struct socket *sock)
 	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 */
-- 
2.43.0