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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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