[PATCH can-next v2 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 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