[PATCH net] mac802154: flush rx_mac_cmd_list before freeing sdata

[PATCH net] mac802154: flush rx_mac_cmd_list before freeing sdata

[Ibrahim Hashimov]

mac802154_rx_mac_cmd_worker() (net/mac802154/rx.c) is queued on
local->mac_wq every time a MAC-command frame (assoc req/resp, disassoc
notify, beacon req) is received on any interface of a given phy. Each
queued struct cfg802154_mac_pkt stashes a *raw* pointer to the
receiving interface's ieee802154_sub_if_data (sdata) in
ieee802154_subif_frame():

	mac_pkt->sdata = sdata;
	list_add_tail(&mac_pkt->node, &sdata->local->rx_mac_cmd_list);
	queue_work(sdata->local->mac_wq, &sdata->local->rx_mac_cmd_work);

and the worker later dereferences it with no liveness check at all,
e.g. for IEEE802154_CMD_ASSOCIATION_REQ:

	if (mac_pkt->sdata->wpan_dev.iftype != NL802154_IFTYPE_COORD)

Neither teardown path drains this queue before the sdata it points to
is freed:

 * ieee802154_if_remove() (net/mac802154/iface.c), reached from the
   nl802154 NL802154_CMD_DEL_INTERFACE handler, does list_del_rcu() +
   synchronize_rcu() + unregister_netdevice(sdata->dev) -- which frees
   sdata via priv_destructor/needs_free_netdev -- without touching
   local->mac_wq or local->rx_mac_cmd_list at all.

 * ieee802154_unregister_hw() (net/mac802154/main.c) only flushes
   local->workqueue (the DATA-path queue) before calling
   ieee802154_remove_interfaces(), which frees every sdata on the
   phy; local->mac_wq is drained only via destroy_workqueue() much
   later, after the interfaces (and their sdata) are already gone.

Either way, if mac802154_rx_mac_cmd_worker() is already queued (or
races back in from a frame received just before teardown), it runs
after the free and dereferences freed memory -- confirmed under
KASAN: flooding a victim NODE interface with MAC_CMD frames from a
MONITOR interface on a sibling phy, then deleting the victim via
NL802154_CMD_DEL_INTERFACE, reliably produces:

  BUG: KASAN: use-after-free in mac802154_rx_mac_cmd_worker+0x463/0x630 [mac802154]
  Read of size 4 at addr ffff888004b22a18 by task kworker/u8:1/31
  Workqueue: phy0-mac-cmds mac802154_rx_mac_cmd_worker [mac802154]
  Freed by task 498: ... (the DEL_INTERFACE task, ieee802154_if_remove)

Verified on the same v6.19 KASAN stand with this patch applied: the
identical MONITOR-flood-then-DEL_INTERFACE reproducer no longer trips
the use-after-free report in mac802154_rx_mac_cmd_worker().

Fix it the same way mac802154_flush_queued_beacons() (net/mac802154/scan.c)
already flushes local->rx_beacon_list on scan cleanup, plus a
cancel_work_sync() step: rx_beacon_work's worker never dereferences
sdata, so the existing sibling doesn't need it, but rx_mac_cmd_work's
does. Add mac802154_flush_queued_mac_cmds(local, sdata):

 - cancel_work_sync(&local->rx_mac_cmd_work) waits out a run already
   in flight (still safe -- nothing has been freed yet) and blocks any
   new run from starting while we hold the RTNL;
 - every rx_mac_cmd_list entry whose ->sdata matches (or every entry,
   when called with sdata == NULL for full-teardown) is then dropped,
   so no future run of the worker can see it;
 - if entries belonging to *other*, still-live interfaces on the same
   local remain, the work is re-queued so they still get processed.

Call it from both teardown paths:

 - ieee802154_if_remove(), before unregister_netdevice(sdata->dev),
   filtered to the sdata being removed (other interfaces on the same
   phy may have legitimate entries in flight);
 - ieee802154_unregister_hw(), before ieee802154_remove_interfaces(),
   with sdata == NULL since every interface on the local is going
   away and local->workqueue's flush_workqueue() does not cover
   local->mac_wq.

This mirrors how mac80211 drains per-interface work (e.g. the
analogous per-sdata work items cancelled from ieee80211_do_stop()
before an interface is torn down) and the existing mac802154 scan.c
list-flush idiom, applied to the one rx_mac_cmd_list consumer that
actually dereferences the freed interface.

Fixes: d021d218f6d9 ("mac802154: Handle received BEACON_REQ")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
---
 net/mac802154/ieee802154_i.h |  2 ++
 net/mac802154/iface.c        | 10 +++++++++
 net/mac802154/main.c         | 11 ++++++++++
 net/mac802154/rx.c           | 42 ++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 8f2bff268392..e3c5c8d5b5d0 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -300,6 +300,8 @@ static inline bool mac802154_is_beaconing(struct ieee802154_local *local)
 }
 
 void mac802154_rx_mac_cmd_worker(struct work_struct *work);
+void mac802154_flush_queued_mac_cmds(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,
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 000be60d9580..a5aa213e25a2 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -694,6 +694,16 @@ void ieee802154_if_remove(struct ieee802154_sub_if_data *sdata)
 	mutex_unlock(&sdata->local->iflist_mtx);
 
 	synchronize_rcu();
+
+	/*
+	 * Drop any rx_mac_cmd_list entry still pointing at this sdata
+	 * before it is freed below: mac802154_rx_mac_cmd_worker() runs
+	 * asynchronously on local->mac_wq and derefs mac_pkt->sdata with
+	 * no liveness check of its own (see mac802154_flush_queued_mac_cmds()
+	 * for details).
+	 */
+	mac802154_flush_queued_mac_cmds(sdata->local, sdata);
+
 	unregister_netdevice(sdata->dev);
 }
 
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index ea1efef3572a..2f8c57e78db1 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -277,6 +277,17 @@ void ieee802154_unregister_hw(struct ieee802154_hw *hw)
 	tasklet_kill(&local->tasklet);
 	flush_workqueue(local->workqueue);
 
+	/*
+	 * tasklet_kill() above stops any further frame reaching
+	 * ieee802154_subif_frame(), but mac802154_rx_mac_cmd_worker() may
+	 * still be queued/running on local->mac_wq and derefs the sdata of
+	 * every interface ieee802154_remove_interfaces() is about to free
+	 * below. flush_workqueue(local->workqueue) does not cover it --
+	 * that is the DATA workqueue, not local->mac_wq -- so drain it
+	 * explicitly first.
+	 */
+	mac802154_flush_queued_mac_cmds(local, NULL);
+
 	rtnl_lock();
 
 	ieee802154_remove_interfaces(local);
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index cd8f2a11920d..0b167f76cb23 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -128,6 +128,48 @@ out:
 	kfree(mac_pkt);
 }
 
+/**
+ * mac802154_flush_queued_mac_cmds - drop pending rx_mac_cmd_list work
+ * @local: the mac802154 device the queue belongs to
+ * @sdata: interface being torn down, or %NULL to flush unconditionally
+ *
+ * Every queued &struct cfg802154_mac_pkt stashes a raw pointer to the
+ * interface it was received on (see ieee802154_subif_frame() below) which
+ * mac802154_rx_mac_cmd_worker() dereferences without ever checking whether
+ * that interface is still alive. Callers must invoke this before freeing
+ * @sdata -- or every interface on @local, when @sdata is %NULL -- so the
+ * worker can never run against freed memory:
+ *
+ *  - cancel_work_sync() waits out a run already in flight. That is still
+ *    safe to let finish because nothing has been freed yet, and it blocks
+ *    any new run from starting for as long as we hold the RTNL.
+ *  - every list entry pointing at @sdata (all of them, if @sdata is NULL)
+ *    is then dropped so no future run of the worker can see it.
+ *
+ * Mirrors mac802154_flush_queued_beacons() in scan.c, which does not need
+ * the cancel_work_sync() step because its worker never dereferences sdata.
+ */
+void mac802154_flush_queued_mac_cmds(struct ieee802154_local *local,
+				     struct ieee802154_sub_if_data *sdata)
+{
+	struct cfg802154_mac_pkt *mac_pkt, *tmp;
+
+	cancel_work_sync(&local->rx_mac_cmd_work);
+
+	list_for_each_entry_safe(mac_pkt, tmp, &local->rx_mac_cmd_list, node) {
+		if (sdata && mac_pkt->sdata != sdata)
+			continue;
+
+		list_del(&mac_pkt->node);
+		kfree_skb(mac_pkt->skb);
+		kfree(mac_pkt);
+	}
+
+	/* Other interfaces on @local may still have entries pending. */
+	if (!list_empty(&local->rx_mac_cmd_list))
+		queue_work(local->mac_wq, &local->rx_mac_cmd_work);
+}
+
 static int
 ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		       struct sk_buff *skb, const struct ieee802154_hdr *hdr)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH net] mac802154: flush rx_mac_cmd_list before freeing sdata

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/
---
mac802154: flush rx_mac_cmd_list before freeing sdata

This patch adds mac802154_flush_queued_mac_cmds() and calls it from
ieee802154_if_remove() and ieee802154_unregister_hw() to drain
local->rx_mac_cmd_list before sdata is freed, aiming to close a KASAN
use-after-free in mac802154_rx_mac_cmd_worker().

> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index b823720630e7..59a045cccfe9 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -694,6 +694,16 @@ void ieee802154_if_remove(struct ieee802154_sub_if_data *sdata)
>  	mutex_unlock(&sdata->local->iflist_mtx);
>  
>  	synchronize_rcu();
> +
> +	/*
> +	 * Drop any rx_mac_cmd_list entry still pointing at this sdata
> +	 * before it is freed below: mac802154_rx_mac_cmd_worker() runs
> +	 * asynchronously on local->mac_wq and derefs mac_pkt->sdata with
> +	 * no liveness check of its own (see mac802154_flush_queued_mac_cmds()
> +	 * for details).
> +	 */
> +	mac802154_flush_queued_mac_cmds(sdata->local, sdata);
> +
>  	unregister_netdevice(sdata->dev);
>  }

[High]
When ieee802154_if_remove() runs to delete one interface, does anything
stop RX softirq on sibling interfaces of the same phy from concurrently
enqueueing into local->rx_mac_cmd_list while the flush is walking it?

The producer path is ieee802154_subif_frame(), reached from the RX
tasklet under rcu_read_lock only, and it unconditionally does:

    mac_pkt->sdata = sdata;
    list_add_tail(&mac_pkt->node, &sdata->local->rx_mac_cmd_list);
    queue_work(sdata->local->mac_wq, &sdata->local->rx_mac_cmd_work);

with no lock on rx_mac_cmd_list. list_del_rcu() + synchronize_rcu()
above only quiesces the sdata being removed; other sdata on the same
local remain in local->interfaces and can keep receiving MAC-command
frames on other CPUs throughout the flush.

> diff --git a/net/mac802154/main.c b/net/mac802154/main.c
> index ea1efef3572a..2f8c57e78db1 100644
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -277,6 +277,17 @@ void ieee802154_unregister_hw(struct ieee802154_hw *hw)
>  	tasklet_kill(&local->tasklet);
>  	flush_workqueue(local->workqueue);
>  
> +	/*
> +	 * tasklet_kill() above stops any further frame reaching
> +	 * ieee802154_subif_frame(), but mac802154_rx_mac_cmd_worker() may
> +	 * still be queued/running on local->mac_wq and derefs the sdata of
> +	 * every interface ieee802154_remove_interfaces() is about to free
> +	 * below. flush_workqueue(local->workqueue) does not cover it --
> +	 * that is the DATA workqueue, not local->mac_wq -- so drain it
> +	 * explicitly first.
> +	 */
> +	mac802154_flush_queued_mac_cmds(local, NULL);
> +
>  	rtnl_lock();
>  
>  	ieee802154_remove_interfaces(local);

[High]
Does tasklet_kill() actually stop any further frame from reaching
ieee802154_subif_frame() here?

tasklet_kill() clears TASKLET_STATE_SCHED and waits for a running
instance to finish, but it does not prevent a subsequent
tasklet_schedule() from a driver's ieee802154_rx_irqsafe() path. If a
driver interrupt fires between mac802154_flush_queued_mac_cmds(local,
NULL) and destroy_workqueue(local->mac_wq) further down, a new
mac_pkt would be added to rx_mac_cmd_list, the worker would be queued
on local->mac_wq, and it would then run against sdatas already freed
by ieee802154_remove_interfaces().

Note this call site also runs before rtnl_lock() a few lines below,
which is relevant to the kernel-doc comment quoted further down.

> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index cd8f2a11920d..0b167f76cb23 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -128,6 +128,48 @@ void mac802154_rx_mac_cmd_worker(struct work_struct *work)
>  	kfree(mac_pkt);
>  }
>  
> +/**
> + * mac802154_flush_queued_mac_cmds - drop pending rx_mac_cmd_list work
> + * @local: the mac802154 device the queue belongs to
> + * @sdata: interface being torn down, or %NULL to flush unconditionally
> + *
> + * Every queued &struct cfg802154_mac_pkt stashes a raw pointer to the
> + * interface it was received on (see ieee802154_subif_frame() below) which
> + * mac802154_rx_mac_cmd_worker() dereferences without ever checking whether
> + * that interface is still alive. Callers must invoke this before freeing
> + * @sdata -- or every interface on @local, when @sdata is %NULL -- so the
> + * worker can never run against freed memory:
> + *
> + *  - cancel_work_sync() waits out a run already in flight. That is still
> + *    safe to let finish because nothing has been freed yet, and it blocks
> + *    any new run from starting for as long as we hold the RTNL.

[Low]
Is this description of cancel_work_sync() accurate?

cancel_work_sync() only guarantees the work is not pending or executing
by the time it returns, and does not itself block future queue_work()
calls. RTNL is not held at the ieee802154_unregister_hw() call site
(the flush is invoked before rtnl_lock() there), and at the
ieee802154_if_remove() site RTNL is a plain mutex that does not disable
BH and does not serialize with the RX softirq path
ieee802154_tasklet_handler() -> ieee802154_rx() ->
__ieee802154_rx_handle_packet() -> ieee802154_subif_frame(), which is
what enqueues into rx_mac_cmd_list and re-arms rx_mac_cmd_work via
queue_work(local->mac_wq, ...).

Could this comment be reworded to describe an invariant the code
actually provides?

> + *  - every list entry pointing at @sdata (all of them, if @sdata is NULL)
> + *    is then dropped so no future run of the worker can see it.
> + *
> + * Mirrors mac802154_flush_queued_beacons() in scan.c, which does not need
> + * the cancel_work_sync() step because its worker never dereferences sdata.
> + */
> +void mac802154_flush_queued_mac_cmds(struct ieee802154_local *local,
> +				     struct ieee802154_sub_if_data *sdata)
> +{
> +	struct cfg802154_mac_pkt *mac_pkt, *tmp;
> +
> +	cancel_work_sync(&local->rx_mac_cmd_work);
> +
> +	list_for_each_entry_safe(mac_pkt, tmp, &local->rx_mac_cmd_list, node) {
> +		if (sdata && mac_pkt->sdata != sdata)
> +			continue;
> +
> +		list_del(&mac_pkt->node);
> +		kfree_skb(mac_pkt->skb);
> +		kfree(mac_pkt);
> +	}
> +
> +	/* Other interfaces on @local may still have entries pending. */
> +	if (!list_empty(&local->rx_mac_cmd_list))
> +		queue_work(local->mac_wq, &local->rx_mac_cmd_work);
> +}

[High]
Given that ieee802154_subif_frame() can still be entered from softirq on
sibling interfaces after cancel_work_sync() returns, what protects
rx_mac_cmd_list during this iteration?

The producer does:

    list_add_tail(&mac_pkt->node, &sdata->local->rx_mac_cmd_list);
    queue_work(sdata->local->mac_wq, &sdata->local->rx_mac_cmd_work);

with no lock. cancel_work_sync() does not block a subsequent
queue_work(), so mac802154_rx_mac_cmd_worker() can start on another CPU
while this function is still iterating. The worker does
list_first_entry_or_null() + list_del() on the same list, and the
iterator here pre-loads tmp = list_next_entry(pos) each round.

Under that shape, could a concurrent list_del() from the worker on the
head entry (a) target the same entry the flush is about to delete
(double kfree() of mac_pkt), (b) free the entry that tmp points at
(use-after-free inside the flush loop), or (c) race with a
list_add_tail() from the producer softirq and leave neighbour pointers
inconsistent?

The trailing:

    if (!list_empty(&local->rx_mac_cmd_list))
        queue_work(local->mac_wq, &local->rx_mac_cmd_work);

also reads list state and re-arms the worker without any lock, which is
a TOCTOU against a softirq list_add_tail() on the same list.

Would either a spin_lock_bh() around every rx_mac_cmd_list mutation
plus the queue_work() (matching the softirq producer), or preventing
new enqueues for the sdata being torn down in ieee802154_subif_frame()
before the flush runs, close this window?

Also, in the multi-interface case ieee802154_if_remove() only quiesces
the sdata being removed with synchronize_rcu(); does this fix actually
address the scenario the commit message describes, where MAC_CMD
traffic is arriving on a sibling interface of the same phy during
NL802154_CMD_DEL_INTERFACE?