[PATCH net v2] mac802154: fix data race and NULL deref on local->assoc_dev

[PATCH net v2] mac802154: fix data race and NULL deref on local->assoc_dev

[Kaiwen Shi]

local->assoc_dev is shared between the association path and the
association-response worker without common synchronization.

mac802154_perform_association() stores the coordinator pointer and waits
for a response. Its timeout and error paths clear the pointer and return
to mac802154_associate(), which may then free the coordinator object.
Meanwhile, mac802154_rx_mac_cmd_worker() may observe the associating bit
and enter mac802154_process_association_resp(), which dereferences
assoc_dev.

The worker's bit test and the handler's pointer dereference are not
atomic with respect to cleanup. Cleanup can clear assoc_dev between them,
causing a NULL dereference, or free the coordinator while the response
handler still uses the pointer.

The response handler only needs the coordinator extended address.
Replace assoc_dev with a cached address, removing the pointer lifetime
dependency. Protect the cached address and the associating bit with a
dedicated spinlock. A READ_ONCE()/WRITE_ONCE() pair would not guarantee
an atomic __le64 access on all 32-bit architectures.

Reset the completion, publish the cached address, and set the associating
bit while holding the lock. Cleanup clears the bit under the same lock.
The response handler takes the lock, rechecks the bit, validates the
cached address, records the response, and completes the waiter before
unlocking. Thus cleanup cannot pass the handler between its state check
and completion, and the cached 64-bit value cannot tear.

Both users run in process context, so a plain spinlock is sufficient.
The lock is not held while waiting for the completion.

Suggested-by: Miquel Raynal <miquel.raynal@bootlin.com>
Fixes: fefd19807fe9 ("mac802154: Handle associating")
Cc: stable@vger.kernel.org
Signed-off-by: Kaiwen Shi <skwkevin@mail.ustc.edu.cn>
---
v2:
  - cache the coordinator extended address instead of retaining the
    assoc_dev pointer, as suggested by Miquel;
  - use a plain spinlock to serialize the cached address and associating
    bit, including 64-bit address accesses on 32-bit architectures;
  - reset the completion under the same lock, recheck the associating bit
    in the response handler, and complete before releasing the lock;
  - use the response payload in the debug message;
  - add Cc: stable@vger.kernel.org.

Link: https://lore.kernel.org/r/20260824175938.11143-1-skwkevin@mail.ustc.edu.cn

 net/mac802154/ieee802154_i.h |  3 ++-
 net/mac802154/main.c         |  1 +
 net/mac802154/scan.c         | 21 ++++++++++++++-------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 8f2bff268392..8f92ac83f5f9 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -76,7 +76,8 @@ struct ieee802154_local {
 	struct work_struct rx_mac_cmd_work;
 
 	/* Association */
-	struct ieee802154_pan_device *assoc_dev;
+	spinlock_t assoc_lock; /* protects association address and active bit */
+	__le64 assoc_dev_extended_addr;
 	struct completion assoc_done;
 	__le16 assoc_addr;
 	u8 assoc_status;
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index ea1efef3572a..63e89bd586e3 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -104,6 +104,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
 	INIT_WORK(&local->rx_mac_cmd_work, mac802154_rx_mac_cmd_worker);
 
 	init_completion(&local->assoc_done);
+	spin_lock_init(&local->assoc_lock);
 
 	/* init supported flags with 802.15.4 default ranges */
 	phy->supported.max_minbe = 8;
diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index 005338f89b75..0ae11e448ccb 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -578,9 +578,11 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 		return ret;
 	}
 
-	local->assoc_dev = coord;
+	spin_lock(&local->assoc_lock);
 	reinit_completion(&local->assoc_done);
+	local->assoc_dev_extended_addr = coord->extended_addr;
 	set_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
+	spin_unlock(&local->assoc_lock);
 
 	ret = ieee802154_mlme_tx_one_locked(local, sdata, skb);
 	if (ret) {
@@ -616,8 +618,9 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 	*short_addr = local->assoc_addr;
 
 clear_assoc:
+	spin_lock(&local->assoc_lock);
 	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
-	local->assoc_dev = NULL;
+	spin_unlock(&local->assoc_lock);
 
 	return ret;
 }
@@ -639,19 +642,23 @@ int mac802154_process_association_resp(struct ieee802154_sub_if_data *sdata,
 		     dest->mode != IEEE802154_EXTENDED_ADDRESSING))
 		return -EINVAL;
 
-	if (unlikely(dest->extended_addr != wpan_dev->extended_addr ||
-		     src->extended_addr != local->assoc_dev->extended_addr))
+	spin_lock(&local->assoc_lock);
+	if (unlikely(!test_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing) ||
+		     dest->extended_addr != wpan_dev->extended_addr ||
+		     src->extended_addr != local->assoc_dev_extended_addr)) {
+		spin_unlock(&local->assoc_lock);
 		return -ENODEV;
+	}
 
 	memcpy(&resp_pl, skb->data, sizeof(resp_pl));
 	local->assoc_addr = resp_pl.short_addr;
 	local->assoc_status = resp_pl.status;
+	complete(&local->assoc_done);
+	spin_unlock(&local->assoc_lock);
 
 	dev_dbg(&skb->dev->dev,
 		"ASSOC RESP 0x%x received from %8phC, getting short address %04x\n",
-		local->assoc_status, &deaddr, local->assoc_addr);
-
-	complete(&local->assoc_done);
+		resp_pl.status, &deaddr, resp_pl.short_addr);
 
 	return 0;
 }

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.34.1

Re: [PATCH net v2] mac802154: fix data race and NULL deref on local->assoc_dev

[Miquel Raynal]

Hi Kaiwen,

>   - cache the coordinator extended address instead of retaining the
>     assoc_dev pointer, as suggested by Miquel;
>   - use a plain spinlock to serialize the cached address and associating
>     bit, including 64-bit address accesses on 32-bit architectures;

Can you justify the choice of a spinlock vs. mutex here? This is an open
question, not a request for changes.

>   - reset the completion under the same lock, recheck the associating bit
>     in the response handler, and complete before releasing the lock;
>   - use the response payload in the debug message;
>   - add Cc: stable@vger.kernel.org.
>

...

> --- a/net/mac802154/scan.c
> +++ b/net/mac802154/scan.c
> @@ -578,9 +578,11 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
>  		return ret;
>  	}
>  
> -	local->assoc_dev = coord;
> +	spin_lock(&local->assoc_lock);
>  	reinit_completion(&local->assoc_done);
> +	local->assoc_dev_extended_addr = coord->extended_addr;
>  	set_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
> +	spin_unlock(&local->assoc_lock);
>  
>  	ret = ieee802154_mlme_tx_one_locked(local, sdata, skb);
>  	if (ret) {

Shouldn't we also make sure that accessing assoc_status/assoc_addr is
serialized? Typically, I believe the IS_ASSOCIATING bit should be
cleared earlier in mac802154_perform_association(), just after the
wait_for_completion call returns. This way, in case we get two responses
for the same request (maybe a malicious one), it will prevent the
possibility to get incoherent assoc_status and assoc_address.

Thanks,
Miquèl

Re: [PATCH net v2] mac802154: fix data race and NULL deref on local->assoc_dev

[Kaiwen Shi]

Hi Miquèl,

> Can you justify the choice of a spinlock vs. mutex here? This is an open
> question, not a request for changes.

Both users run in process context (the response handler runs from a
workqueue), so either lock is legal. I used a spinlock because the
critical sections are tiny and never sleep: caching a __le64 address and
flipping the associating bit, or validating a frame, storing a short
address plus a status byte, and completing the waiter. The lock is never
held across wait_for_completion_killable_timeout(), so a sleeping lock
would add nothing.

I also looked at reusing wpan_dev->association_lock rather than adding a
lock. It is not held on either of the two paths involved here
(mac802154_perform_association() and mac802154_process_association_resp()),
and pan.c has several lockdep_assert_held() on it, so widening its scope
to the response path looked like a bigger change than this fix should
carry. Happy to revisit if you would rather see one lock covering both.

> Shouldn't we also make sure that accessing assoc_status/assoc_addr is
> serialized? Typically, I believe the IS_ASSOCIATING bit should be
> cleared earlier in mac802154_perform_association(), just after the
> wait_for_completion call returns ...

You are right. The success path reads assoc_status/assoc_addr after the
wait returns but before clearing the bit, so a second (e.g. malicious)
response accepted in that window can still overwrite them mid-read.

With a debug delay inserted between the wait returning and the result
being read, and a peer that keeps answering the same request with an
increasing short address, the association path reads 0x08f0 right after
the wait and 0x0903 after the delay; 38 further responses were accepted
in between. v3 clears the associating bit and snapshots both fields under
assoc_lock right after the wait returns, and in the same test no response
is accepted in that window and both reads return the same address.

This is also what the scan path already does: mac802154_scan_cleanup_locked()
clears IEEE802154_IS_SCANNING first, "to prevent any further use of the
scan request", and only then replaces and frees the request.

A v3 follows.

Thanks,
Kaiwen