can: protect the receive lists with a per-device lock

The receive lists of a CAN device are located in the device's can_ml_priv and travel with the device when it is moved to another network namespace. But they are protected by the per-netns rcvlists_lock. So can_rx_unregister() needs dev_net(dev) to pick the right lock and silently returns when the passed namespace does not match dev_net(dev) anymore.

isotp_release() and bcm_release() leave their notifier list before they take any lock that serializes against the netdev notifier, which runs under RTNL. When the bound device is moved to another namespace in this window, the notifier skips the socket and the release path has to remove the filters itself. isotp_release() passes the socket's namespace, which does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev), which can_rx_unregister() reads again for its check, so a namespace change between both reads makes the check fail as well. In both cases can_rx_unregister() silently returns. As can_rx_register() stores rcv->sk without a reference, the remaining receivers point to the freed socket (isotp) or the freed op (bcm):

  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

Besides this, the lists of a device are protected by the rcvlists_lock of the namespace that is passed in. Callers which pass dev_net(dev) read it at different times, so the same lists may be modified under the locks of different namespaces while the device is moved.

Taking RTNL in these release paths is no option: raw_bind(), raw_setsockopt() and raw_release() already hold RTNL while adding and removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged users can trigger repeatedly and which makes RTNL a global bottleneck.

Instead fix the root cause and add a spinlock to struct can_dev_rcv_lists, which protects the receive lists of a CAN device or the 'any' CAN device lists of a namespace. It is initialized in can_set_ml_priv(), which is used by all CAN network devices. With this, can_rx_unregister() finds the subscription independently of the current namespace of the device. The protocols keep the device valid with their tracked references. The per-netns rcvlists_lock only protects the receive list statistics anymore, and CAN_RAW, CAN_BCM and CAN_ISOTP pass the namespace that has been used for can_rx_register().

So isotp_release() and bcm_release() now remove their filters correctly without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and isotp_bind() do not need RTNL anymore and add and remove their filters under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind() publishes the device inside the RCU section of its lookup, and raw_notify() and isotp_notify() recheck the bound device under lock_sock(). As RTNL does not keep NETDEV_DOWN out of bind() anymore, bind() checks IFF_UP after publishing the bound device, with a barrier pair against raw_notify() and isotp_notify(), so a down transition during bind() is still reported with ENETDOWN.

---
Changes in v6:
- isotp_bind(): check the device (CAN device, MTU) before publishing so->dev, so bind() can not fail after so->dev has become visible to isotp_notify(). A failing bind() can not get a needless ENETDOWN from
a concurrent NETDEV_DOWN anymore (Sashiko review)
- v5: https://lore.kernel.org/linux-can/[email protected]/

Changes in v5:
- raw_bind() / isotp_bind(): check IFF_UP after publishing the bound device, with an smp_mb() pair against raw_notify() / isotp_notify() for NETDEV_DOWN, so a down transition during bind() is reported with ENETDOWN again, as with RTNL before (Sashiko review). isotp_bind() already published so->dev before the IFF_UP check, but without a barrier the load could still pass the store.
- raw_bind() no longer reports ENETDOWN when bind() fails
- rcvlists_lock comment names the rcv_entries(_max) stats (Sashiko review)
- can_rx_unregister() kernel-doc and commit message: @net should be the namespace of can_rx_register(), otherwise only the statistics become imprecise (j1939, Sashiko review)
- v4: https://lore.kernel.org/linux-can/[email protected]/

Changes in v4:
- rebased on linux-can-next/testing: the context in raw_bind() changed with "can: raw: remove redundant NULL check before netdev_hold()", which made v3 fail to apply
- added "can: proc: skip stats calculation after reset in can_stat_update()" from linux-can-next/testing to the base sequence
- no functional change
- v3: https://lore.kernel.org/linux-can/[email protected]/

Changes in v3:
- not RFC anymore, sent as new series with the patches 1-3 from the linux-can-next pull request to build on top of mainline (see cover letter)
- commit message: describe the isotp and bcm cases separately (bcm passes dev_net(dev), which can_rx_unregister() reads again for its check) and mention that the lists of a device could be modified under the locks of different namespaces
- can_rx_unregister() kernel-doc: @net also selects the 'any' CAN device lists for @dev == NULL
- v2: https://lore.kernel.org/linux-can/[email protected]/

Changes in v2:
- v1 relied on the NETDEV_UNREGISTER rebroadcast in netdev_wait_allrefs_any() to catch a bind() racing with the notifier, which is not sent on a namespace change. raw_bind() and isotp_bind() now publish the device inside the RCU section of dev_get_by_index_rcu() (CAN_RAW: new ro->bind_dev hint with smp_store_release() / smp_load_acquire(), CAN_ISOTP: so->dev is published early and reset when bind() fails)
- moved the implementation details from the commit message to the notes
- shortened the code comments
- replaced the rebind rate measurement with an RTNL latency measurement
- v1: https://lore.kernel.org/linux-can/[email protected]/

Notes:

- bind() vs. NETDEV_UNREGISTER: Both on unregistration and on a namespace change, unlist_netdevice() and synchronize_net() precede NETDEV_UNREGISTER. bind() publishes the device (ro->bind_dev / so->dev) inside the RCU section of dev_get_by_index_rcu(). So either the notifier sees it after synchronize_net() and handles the socket under lock_sock(), or the lookup does not find the unlisted device and bind() fails with -ENODEV. The NETDEV_UNREGISTER rebroadcast in netdev_wait_allrefs_any() is not relied on, as it does not happen on a namespace change.

- raw_notify() / isotp_notify() keep a lockless READ_ONCE() fast path, so RTNL only waits for lock_sock() of sockets that are bound or being bound to the vanishing device. CAN_RAW needs the separate ro->bind_dev hint, as ro->dev still points to the previously bound device until a rebind completes. ro->bind_dev is never dereferenced; a stale value only causes a needless lock_sock(). raw_notify() reads ro->bind_dev with smp_load_acquire() before ro->dev, paired with smp_store_release() in raw_bind(): A later bind() can only overwrite ro->bind_dev after the previous bind() has set ro->dev and released lock_sock(). Otherwise the notifier could see a stale ro->dev and the ro->bind_dev of a subsequent failing rebind to another device, and miss the binding. CAN_ISOTP does not support rebinding and publishes so->dev directly after all checks that can fail. bcm_notify() always takes lock_sock() and bcm_rx_setup() looks up the device under lock_sock(), so bcm needs no extra handling.

- raw_setsockopt() copies the filters from user space before lock_sock(), so a stalled copy can not block a NETDEV_UNREGISTER that waits for lock_sock() under RTNL. The dev->reg_state check is only a shortcut now (READ_ONCE()) and does not catch a namespace change.

- NETDEV_DOWN: raw_notify() / isotp_notify() check ro->dev / so->dev locklessly. They are written with WRITE_ONCE() in bind() and in the notifier. IFF_UP is cleared in __dev_close_many() before netif_close_many() or __dev_notify_flags() send NETDEV_DOWN in the same context, and bind() checks IFF_UP after publishing the bound device. With an smp_mb() on both sides, either the notifier sees the binding or bind() sees the cleared IFF_UP.

- j1939 benefits as well: After a namespace change its can_rx_unregister() with dev_net(ndev) picked the rcvlists_lock of the wrong namespace. The receive list statistics may still become imprecise in this case, as before.

- Tested on a lockdep + KASAN kernel:
- 10 min CAN_RAW / CAN_BCM / CAN_ISOTP traffic and procfs reads in two namespaces while moving the CAN device between them (2272 moves)
- CAN_RAW / CAN_ISOTP bind and CAN_RAW rebind churn (incl. failing rebinds to a non-CAN device) against 1007 namespace moves, checking that no filters are left on the device in the other namespace, and against 611 device deletions
- bcm_release() with a stalled procfs reader does not block RTNL
- RTNL latency (no-op SIOCSIFMTU on lo) under CAN_RAW 512 filter

    churn: avg ~1 us, max ~1.2 ms (before: avg ~250 us, max ~24 ms)

 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  39 ++++++----
 net/can/bcm.c              |  43 +++-------
 net/can/isotp.c            |  58 ++++++++------
 net/can/raw.c              | 155 ++++++++++++++++++++++++-------------
 6 files changed, 180 insertions(+), 120 deletions(-)

index 1e99fda2b380..cfefa4a385be 100644
@@ -43,10 +43,11 @@
 #define CAN_ML_H
 
 #include <linux/can.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/spinlock.h>
 
 /* exposed CAN device capabilities for network layer */
 #define CAN_CAP_CC BIT(0) /* CAN CC aka Classical CAN */
 #define CAN_CAP_FD BIT(1) /* CAN FD */
 #define CAN_CAP_XL BIT(2) /* CAN XL */

@@ -57,10 +58,11 @@
 #define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)
 
 enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX };
 
 struct can_dev_rcv_lists {
+	spinlock_t lock; /* protects the receive lists and entries */
 	struct hlist_head rx[RX_MAX];
 	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
 	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
 	int entries;
 };

@@ -79,10 +81,11 @@ static inline struct can_ml_priv *can_get_ml_priv(struct net_device *dev)
 }
 
 static inline void can_set_ml_priv(struct net_device *dev,
 				   struct can_ml_priv *ml_priv)
 {
+	spin_lock_init(&ml_priv->dev_rcv_lists.lock);
 	netdev_set_ml_priv(dev, ml_priv, ML_PRIV_CAN);
 }
 
 static inline bool can_cap_enabled(struct net_device *dev, u32 cap)
 {

index 48b79f7e6236..54f23681e0d4 100644
@@ -27,11 +27,11 @@ struct netns_can {
 	struct proc_dir_entry *bcmproc_dir;
 #endif
 
 	/* receive filters subscribed for 'all' CAN devices */
 	struct can_dev_rcv_lists *rx_alldev_list;
-	spinlock_t rcvlists_lock;
+	spinlock_t rcvlists_lock; /* protects the rcv_entries(_max) stats */
 	struct timer_list stattimer; /* timer for statistics update */
 	struct can_pkg_stats *pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats;
 
 	/* CAN GW per-net gateway jobs */

index d97f85fc3232..0f3051a61aab 100644
@@ -460,13 +460,14 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
 	if (!rcv)
 		return -ENOMEM;
 
-	spin_lock_bh(&net->can.rcvlists_lock);
-
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+
+	spin_lock_bh(&dev_rcv_lists->lock);
+
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	rcv->can_id = can_id;
 	rcv->mask = mask;
 	atomic_long_set(&rcv->matches, 0);

@@ -476,10 +477,13 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	rcv->sk = sk;
 
 	hlist_add_head_rcu(&rcv->list, rcv_list);
 	dev_rcv_lists->entries++;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	rcv_lists_stats->rcv_entries++;
 	rcv_lists_stats->rcv_entries_max = max(rcv_lists_stats->rcv_entries_max,
 					       rcv_lists_stats->rcv_entries);
 	spin_unlock_bh(&net->can.rcvlists_lock);
 

@@ -507,10 +511,18 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  * @func: callback function on filter match
  * @data: returned parameter for callback function
  *
  * Description:
  *  Removes subscription entry depending on given (subscription) values.
+ *  The receive lists of a CAN device are protected by a per-device lock,
+ *  so the subscription is found even when the device has been moved to
+ *  another network namespace after can_rx_register(). The caller has to
+ *  ensure that @dev stays valid, e.g. by holding a reference to @dev or
+ *  RTNL. @net should be the namespace that has been used for
+ *  can_rx_register(). It selects the 'any' CAN device lists for
+ *  @dev == NULL and is otherwise only used for the receive list
+ *  statistics, which become imprecise when another namespace is passed.
  */
 void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
 		       void *data)
 {

@@ -520,16 +532,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 
-	spin_lock_bh(&net->can.rcvlists_lock);
+	spin_lock_bh(&dev_rcv_lists->lock);
 
-	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/* Search the receiver list for the item to delete.  This should
 	 * exist, since no receiver may be unregistered that hasn't
 	 * been registered before.

@@ -545,30 +555,30 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * As this case may potentially happen when closing a socket while
 	 * the notifier for removing the CAN netdev is running we just print
 	 * a warning here.
 	 */
 	if (!rcv) {
+		spin_unlock_bh(&dev_rcv_lists->lock);
 		pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
 			DNAME(dev), can_id, mask);
-		goto out;
+		return;
 	}
 
 	hlist_del_rcu(&rcv->list);
 	dev_rcv_lists->entries--;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	if (rcv_lists_stats->rcv_entries > 0)
 		rcv_lists_stats->rcv_entries--;
-
- out:
 	spin_unlock_bh(&net->can.rcvlists_lock);
 
 	/* schedule the receiver item for deletion */
-	if (rcv) {
-		if (rcv->sk)
-			sock_hold(rcv->sk);
-		call_rcu(&rcv->rcu, can_rx_delete_receiver);
-	}
+	if (rcv->sk)
+		sock_hold(rcv->sk);
+	call_rcu(&rcv->rcu, can_rx_delete_receiver);
 }
 EXPORT_SYMBOL(can_rx_unregister);
 
 static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
 {

@@ -821,10 +831,11 @@ static int can_pernet_init(struct net *net)
 {
 	spin_lock_init(&net->can.rcvlists_lock);
 	net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list);
 	if (!net->can.rx_alldev_list)
 		goto out;
+	spin_lock_init(&net->can.rx_alldev_list->lock);
 	net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats);
 	if (!net->can.pkg_stats)
 		goto out_free_rx_alldev_list;
 	net->can.rcv_lists_stats = kzalloc_obj(*net->can.rcv_lists_stats);
 	if (!net->can.rcv_lists_stats)

2.43.0