mac802154: fix use-after-free of sdata via queued RX frames

The RX softirq producer ieee802154_subif_frame() queues received beacon and MAC-command frames onto local->rx_beacon_list / rx_mac_cmd_list and schedules a process-context worker, storing a raw mac_pkt->sdata (and skb->dev == sdata->dev) with neither a reference nor any locking:

- the lists have no lock: the softirq producer list_add_tail()s while the mac_wq worker list_del()s, so sibling interfaces on the same phy corrupt the list;

- the workers dereference the interface after it may have been freed. mac802154_rx_mac_cmd_worker() touches mac_pkt->sdata directly, and mac802154_rx_beacon_worker() -> mac802154_process_beacon() dereferences skb->dev (== sdata->dev). Removing an interface frees its sdata (netdev_priv) while a queued frame still points at it, so a later worker run is a use-after-free.

Reproduced under KASAN by flooding a victim interface with MAC command frames and removing it (the beacon path is the same class via skb->dev):

  BUG: KASAN: slab-use-after-free in mac802154_rx_mac_cmd_worker+0x463/0x630 [mac802154]
  Read of size 4 at addr ffff888002f9ea18 by task kworker/u8:1/31
  Workqueue: phy0-mac-cmds mac802154_rx_mac_cmd_worker [mac802154]
  Call Trace:
   mac802154_rx_mac_cmd_worker+0x463/0x630 [mac802154]
   process_one_work+0x611/0xe80
   worker_thread+0x52e/0xdc0
   kthread+0x30c/0x630
   ret_from_fork+0x2fd/0x3e0

Fix both lists together:

- add local->rx_lock and take it around every list access: the softirq producer (plain spin_lock, softirq context) and the workers and flush (spin_lock_bh, process context);

- pin the interface for the lifetime of a queued frame with netdev_hold()/netdev_put(), so the worker can safely dereference sdata / skb->dev even while the interface is being removed;

- dequeue under the lock at the head and loop-drain the whole list in the workers (they previously processed one frame per run and relied on a later enqueue to drain the rest);

- drop not-yet-started frames of an interface before it is unregistered, from ieee802154_if_remove() (after the RCU grace period) and from the ieee802154_remove_interfaces() loop -- the latter is the whole-phy teardown path, which does not go through ieee802154_if_remove().

An in-flight worker that already dequeued a frame keeps its own netdev reference; unregister_netdevice() then waits it out in netdev_run_todo(), which runs at rtnl_unlock() (rtnl released) and after the interface has been closed, so it does not pin rtnl. A worker blocked in an association TX only delays that one interface's unregister (the usual "waiting for %s to become free"), it does not hold rtnl. netdev_hold() is used for this reason instead of a cancel_work_sync() under rtnl, which would block on the worker's unbounded MLME TX wait via ieee802154_sync_queue().

The mac-command worker additionally skips processing for a stopped interface (ieee802154_sdata_running()), avoiding a needless association response during teardown.

---
v3:
- Cover rx_beacon_list too: its worker dereferences the interface via skb->dev in mac802154_process_beacon(), so it has the same UAF. v2 wrongly stated the beacon worker never touches the interface (raised by Miquel Raynal and by automated race analysis on the list).
- Use per-frame netdev_hold() instead of drain + cancel_work_sync(). The cancel_work_sync() ran under rtnl in ieee802154_if_remove() and could pin rtnl for the length of an in-flight association TX (ieee802154_sync_queue(), unbounded). The netdev reference is instead waited on in netdev_run_todo(), after rtnl is dropped (raised by Jakub Kicinski and by automated race analysis).
- Fix whole-phy teardown: v2 drained in ieee802154_unregister_hw() before the interfaces were stopped, so a frame could still be queued after the drain and run against freed sdata. Drain per-interface in the ieee802154_remove_interfaces() loop instead.
- Loop-drain the workers (also fixes a pre-existing one-frame-per-run stranding); required so queued netdev references cannot be left pending.
- Include the KASAN splat.

v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

 include/net/cfg802154.h      |   1 +
 net/mac802154/ieee802154_i.h |   8 +++
 net/mac802154/iface.c        |   6 ++
 net/mac802154/main.c         |   1 +
 net/mac802154/rx.c           | 120 ++++++++++++++++++++++++++++-------
 net/mac802154/scan.c         |  10 +--
 6 files changed, 117 insertions(+), 29 deletions(-)

index 76d2cd2e2b30..2e960441ea49 100644
@@ -376,6 +376,7 @@ struct cfg802154_mac_pkt {
 	struct list_head node;
 	struct sk_buff *skb;
 	struct ieee802154_sub_if_data *sdata;
+	netdevice_tracker dev_tracker;
 	u8 page;
 	u8 channel;
 };

index 8f2bff268392..279ff4e9b11e 100644
@@ -74,6 +74,10 @@ struct ieee802154_local {
 	struct work_struct rx_beacon_work;
 	struct list_head rx_mac_cmd_list;
 	struct work_struct rx_mac_cmd_work;
+	/* Serializes rx_beacon_list and rx_mac_cmd_list against the RX
+	 * softirq producer, the mac_wq workers and the teardown flush.
+	 */
+	spinlock_t rx_lock;
 
 	/* Association */
 	struct ieee802154_pan_device *assoc_dev;

@@ -300,6 +304,10 @@ static inline bool mac802154_is_beaconing(struct ieee802154_local *local)
 }
 
 void mac802154_rx_mac_cmd_worker(struct work_struct *work);
+void mac802154_flush_list(struct list_head *list,
+			  struct ieee802154_sub_if_data *sdata);
+void mac802154_flush_queued_pkts(struct ieee802154_local *local,
+				 struct ieee802154_sub_if_data *sdata);
 
 int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 				  struct ieee802154_pan_device *coord,

index b823720630e7..31353795fa24 100644
@@ -694,6 +694,7 @@ void ieee802154_if_remove(struct ieee802154_sub_if_data *sdata)
 	mutex_unlock(&sdata->local->iflist_mtx);
 
 	synchronize_rcu();
+	mac802154_flush_queued_pkts(sdata->local, sdata);
 	unregister_netdevice(sdata->dev);
 }
 

@@ -705,6 +706,11 @@ void ieee802154_remove_interfaces(struct ieee802154_local *local)
 	list_for_each_entry_safe(sdata, tmp, &local->interfaces, list) {
 		list_del_rcu(&sdata->list);
 
+		/* Best-effort: a frame the RX softirq queues for this sdata
+		 * after the flush still pins the netdev, so the
+		 * unregister_netdevice() below waits it out.
+		 */
+		mac802154_flush_queued_pkts(local, sdata);
 		unregister_netdevice(sdata->dev);
 	}
 	mutex_unlock(&local->iflist_mtx);

index ea1efef3572a..386c086c79d1 100644
@@ -91,6 +91,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
 	INIT_LIST_HEAD(&local->interfaces);
 	INIT_LIST_HEAD(&local->rx_beacon_list);
 	INIT_LIST_HEAD(&local->rx_mac_cmd_list);
+	spin_lock_init(&local->rx_lock);
 	mutex_init(&local->iflist_mtx);
 
 	tasklet_setup(&local->tasklet, ieee802154_tasklet_handler);

index 300d4584533e..245e32970c05 100644
@@ -104,13 +104,9 @@ static unsigned int mac802154_scan_get_channel_time(u8 duration_order,
 
 static void mac802154_flush_queued_beacons(struct ieee802154_local *local)
 {
-	struct cfg802154_mac_pkt *mac_pkt, *tmp;
-
-	list_for_each_entry_safe(mac_pkt, tmp, &local->rx_beacon_list, node) {
-		list_del(&mac_pkt->node);
-		kfree_skb(mac_pkt->skb);
-		kfree(mac_pkt);
-	}
+	spin_lock_bh(&local->rx_lock);
+	mac802154_flush_list(&local->rx_beacon_list, NULL);
+	spin_unlock_bh(&local->rx_lock);
 }
 
 static void

2.50.1 (Apple Git-155)

Hi Ibrahim,

This whole change makes things clearly less and less readable, but I don't easily see any possible simplification.

If you are willing to fix another possible leak, you can check Sashiko's feedback agout skb_get() which is likely leaked since down the stack we clone it and not consume it.

Thanks,
Miquèl

Hello Ibrahim Hashimov.

Applied to wpan/wpan.git, thanks!

[1/1] mac802154: fix use-after-free of sdata via queued RX frames

      https://git.kernel.org/wpan/wpan/c/2f37fba846c9

regards,
Stefan Schmidt