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

[PATCH net v3] 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 recorded result is exposed to the same window. assoc_status and
assoc_addr are written by the handler but read by the association path
while the associating bit is still set, so a second response for the same
request - a malicious one, for instance - can replace them between those
reads and leave the caller with an incoherent status and address pair.

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. Clear the bit and snapshot the result under
the same lock as soon as wait_for_completion_killable_timeout() returns,
before the result is consumed, so a later response cannot pass the
recheck and replace it. Cleanup clears the bit under the lock too.
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>
---
v3:
 - clear IEEE802154_IS_ASSOCIATING and snapshot assoc_status/assoc_addr
   under assoc_lock right after wait_for_completion returns, so a second
   (e.g. malicious) response can no longer pass the recheck and overwrite
   them while perform_association() consumes the result (Miquel).

v2:
 - replace assoc_dev with the cached coordinator extended address, as
   suggested by Miquel;
 - use a plain spinlock instead of spin_lock_bh(), since both users run
   in process context;
 - protect the cached address and the association-state transitions
   with the same lock;
 - recheck the association state in the response handler and signal the
   completion before releasing the lock;
 - add Cc: stable@vger.kernel.org.

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

Link v1:
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         | 47 ++++++++++++++++++++++++++----------
 3 files changed, 37 insertions(+), 14 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..87a7867dbb72 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -536,7 +536,9 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 	struct ieee802154_association_req_frame frame = {};
 	struct ieee802154_local *local = sdata->local;
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+	__le16 resp_short_addr;
 	struct sk_buff *skb;
+	u8 resp_status;
 	int ret;
 
 	frame.mhr.fc.type = IEEE802154_FC_TYPE_MAC_CMD;
@@ -578,9 +580,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) {
@@ -599,25 +603,38 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 		goto clear_assoc;
 	}
 
-	if (local->assoc_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
-		if (local->assoc_status == IEEE802154_PAN_AT_CAPACITY)
+	/* The association is complete: clear the associating bit and snapshot
+	 * the result under the same lock, so a second (e.g. malicious) ASSOC
+	 * RESP can no longer pass the recheck below and overwrite
+	 * assoc_status/assoc_addr before they are consumed.
+	 */
+	spin_lock(&local->assoc_lock);
+	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
+	resp_status = local->assoc_status;
+	resp_short_addr = local->assoc_addr;
+	spin_unlock(&local->assoc_lock);
+
+	if (resp_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
+		if (resp_status == IEEE802154_PAN_AT_CAPACITY)
 			ret = -ERANGE;
 		else
 			ret = -EPERM;
 
 		dev_warn(&sdata->dev->dev,
 			 "Negative ASSOC RESP received from %8phC: %s\n", &ceaddr,
-			 local->assoc_status == IEEE802154_PAN_AT_CAPACITY ?
+			 resp_status == IEEE802154_PAN_AT_CAPACITY ?
 			 "PAN at capacity" : "access denied");
-		goto clear_assoc;
+		return ret;
 	}
 
-	ret = 0;
-	*short_addr = local->assoc_addr;
+	*short_addr = resp_short_addr;
+
+	return 0;
 
 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 +656,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 v3] mac802154: fix data race and NULL deref on local->assoc_dev

[Xuanqiang Luo]

在 2026/8/28 06:13, Kaiwen Shi 写道:
> @@ -599,25 +603,38 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
>   		goto clear_assoc;
>   	}
>   
> -	if (local->assoc_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
> -		if (local->assoc_status == IEEE802154_PAN_AT_CAPACITY)
> +	/* The association is complete: clear the associating bit and snapshot
> +	 * the result under the same lock, so a second (e.g. malicious) ASSOC
> +	 * RESP can no longer pass the recheck below and overwrite
> +	 * assoc_status/assoc_addr before they are consumed.
> +	 */
> +	spin_lock(&local->assoc_lock);
> +	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);

I think this clear_bit() should be moved to
mac802154_process_association_resp(), after saving the first valid
response and before calling complete().

Clearing it only after wait_for_completion() returns may still leave a
window, since the woken waiter may not acquire assoc_lock before the
next work item runs. Clearing it earlier in the response handler ensures
that subsequent responses fail the in-lock
IEEE802154_IS_ASSOCIATING check and cannot overwrite the saved result.

> +	resp_status = local->assoc_status;
> +	resp_short_addr = local->assoc_addr;
> +	spin_unlock(&local->assoc_lock);
> +
> +	if (resp_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
> +		if (resp_status == IEEE802154_PAN_AT_CAPACITY)
>   			ret = -ERANGE;
>   		else
>   			ret = -EPERM;
>   
>   		dev_warn(&sdata->dev->dev,
>   			 "Negative ASSOC RESP received from %8phC: %s\n", &ceaddr,
> -			 local->assoc_status == IEEE802154_PAN_AT_CAPACITY ?
> +			 resp_status == IEEE802154_PAN_AT_CAPACITY ?
>   			 "PAN at capacity" : "access denied");
> -		goto clear_assoc;
> +		return ret;
>   	}
>   
> -	ret = 0;
> -	*short_addr = local->assoc_addr;
> +	*short_addr = resp_short_addr;
> +
> +	return 0;
>   
>   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 +656,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;

here

> +	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

Thanks,
Xuanqiang

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

[Kaiwen Shi]

Hi Xuanqiang,

> I think this clear_bit() should be moved to
> mac802154_process_association_resp(), after saving the first valid
> response and before calling complete().
>
> Clearing it only after wait_for_completion() returns may still leave a
> window, since the woken waiter may not acquire assoc_lock before the
> next work item runs. Clearing it earlier in the response handler ensures
> that subsequent responses fail the in-lock
> IEEE802154_IS_ASSOCIATING check and cannot overwrite the saved result.

Good catch, you are right. complete() is issued while the handler still
holds assoc_lock, so between the wake-up and the waiter taking that lock
another response can get in, pass the recheck and replace
assoc_status/assoc_addr before perform_association() has consumed them.

I reproduced it on a debug kernel: on v3 the bit is still set when the
wait returns, later responses keep being accepted after that point, and
the association ends up with an address from one of them instead of the
first one. With the clear_bit() moved, none is accepted.

So v4 does what you suggested: clear_bit() right after the result is
stored and before complete(), both under assoc_lock. The timeout and error
paths still clear it under the lock. The success and negative paths do not
need to any more, because a wait that returns success now means the
handler has already cleared the bit.

One thing I got wrong earlier, while I am at it. In my v2 reply I said
wpan_dev->association_lock is not held on either of the two paths
involved. That was wrong. nl802154_associate() takes it around
rdev_associate(), so it is held for the whole of
mac802154_perform_association(), the wait for the response included. That
rules out reusing it here: the handler would only get that mutex once the
association has already timed out. The v4 commit message states this
correctly.

A v4 follows.

Thanks,
Kaiwen