can: take rtnl_lock() before leaving the notifier list

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().

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

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.

---
 net/can/isotp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index f5dc9d04bd68..186daa818647 100644
@@ -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

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.

---
 net/can/bcm.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

index 3d637a1e0ac1..f439bdbf37b1 100644
@@ -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

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().

---
 net/can/raw.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index 0acd4f6c6dd6..925c0c1d243f 100644
@@ -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

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   |