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

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

[Kaiwen Shi]

`local->assoc_dev` is accessed from three places with no common lock
and, on the read side, no NULL or lifetime guarantee:

  - mac802154_perform_association() stores the coordinator pointer in
    it, then blocks in wait_for_completion_killable_timeout() for up
    to 10 s;

  - its clear_assoc label sets it back to NULL;

  - mac802154_process_association_resp(), which runs from the
    rx_mac_cmd workqueue, dereferences
    `local->assoc_dev->extended_addr` with neither lock nor NULL
    check.

The dereference races with both write sites:

  1. NULL dereference: clear_assoc() stores NULL while the RESP
     handler is between loading and dereferencing the pointer; the
     subsequent dereference of `->extended_addr` on a NULL pointer
     faults.

  2. use-after-free: mac802154_associate() frees the freshly
     allocated `parent` in its free_parent path after
     perform_association() returns an error. If the RESP handler
     still holds the now-dangling pointer, it reads freed memory.

No existing lock serializes these accesses.  The
wpan_dev->association_lock mutex only guards the coordinator-side
parent/child list (mac802154_process_association_req() and
friends), which is a different data structure, and it is not held
while this device is associating.  The writer blocks in
wait_for_completion_killable_timeout() for up to 10 s after storing
assoc_dev, so the reader on the rx_mac_cmd workqueue races with both
the NULL store in clear_assoc and the kfree() of the leftover
ieee802154_pan_device in mac802154_associate()'s error path.

Add a dedicated assoc_dev_lock and hold it around all three
accesses; the RESP handler additionally validates the pointer before
dereferencing it.  This is a per-field lock so it cannot contend
with the completion logic.

Fixes: fefd19807fe9 ("mac802154: Handle associating")
Signed-off-by: Kaiwen Shi <skwkevin@mail.ustc.edu.cn>

---
Best regards,
Kaiwen Shi

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

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 8f2bff268392..921712ee2a37 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -77,6 +77,7 @@ struct ieee802154_local {
 
 	/* Association */
 	struct ieee802154_pan_device *assoc_dev;
+	spinlock_t assoc_dev_lock; /* protects assoc_dev */
 	struct completion assoc_done;
 	__le16 assoc_addr;
 	u8 assoc_status;
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index ea1efef3572a..e59f46100203 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_dev_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 65089826ff59..5ef23a4507ea 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -574,7 +574,9 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 		return ret;
 	}
 
+	spin_lock_bh(&local->assoc_dev_lock);
 	local->assoc_dev = coord;
+	spin_unlock_bh(&local->assoc_dev_lock);
 	reinit_completion(&local->assoc_done);
 	set_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
 
@@ -613,7 +615,9 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
 
 clear_assoc:
 	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
+	spin_lock_bh(&local->assoc_dev_lock);
 	local->assoc_dev = NULL;
+	spin_unlock_bh(&local->assoc_dev_lock);
 
 	return ret;
 }
@@ -626,6 +630,7 @@ int mac802154_process_association_resp(struct ieee802154_sub_if_data *sdata,
 	u64 deaddr = swab64((__force u64)dest->extended_addr);
 	struct ieee802154_local *local = sdata->local;
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+	struct ieee802154_pan_device *assoc_dev;
 	struct ieee802154_assoc_resp_pl resp_pl = {};
 
 	if (skb->len != sizeof(resp_pl))
@@ -635,9 +640,15 @@ 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_bh(&local->assoc_dev_lock);
+	assoc_dev = local->assoc_dev;
+	if (unlikely(!assoc_dev ||
+		     dest->extended_addr != wpan_dev->extended_addr ||
+		     src->extended_addr != assoc_dev->extended_addr)) {
+		spin_unlock_bh(&local->assoc_dev_lock);
 		return -ENODEV;
+	}
+	spin_unlock_bh(&local->assoc_dev_lock);
 
 	memcpy(&resp_pl, skb->data, sizeof(resp_pl));
 	local->assoc_addr = resp_pl.short_addr;
-- 
2.47.0

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

[Miquel Raynal]

Hi Kaiwen,

On 25/08/2026 at 01:59:38 +08, Kaiwen Shi <skwkevin@mail.ustc.edu.cn> wrote:

> `local->assoc_dev` is accessed from three places with no common lock
> and, on the read side, no NULL or lifetime guarantee:
>
>   - mac802154_perform_association() stores the coordinator pointer in
>     it, then blocks in wait_for_completion_killable_timeout() for up
>     to 10 s;
>
>   - its clear_assoc label sets it back to NULL;
>
>   - mac802154_process_association_resp(), which runs from the
>     rx_mac_cmd workqueue, dereferences
>     `local->assoc_dev->extended_addr` with neither lock nor NULL
>     check.
>
> The dereference races with both write sites:
>
>   1. NULL dereference: clear_assoc() stores NULL while the RESP
>      handler is between loading and dereferencing the pointer; the
>      subsequent dereference of `->extended_addr` on a NULL pointer
>      faults.
>
>   2. use-after-free: mac802154_associate() frees the freshly
>      allocated `parent` in its free_parent path after
>      perform_association() returns an error. If the RESP handler
>      still holds the now-dangling pointer, it reads freed memory.
>
> No existing lock serializes these accesses.  The
> wpan_dev->association_lock mutex only guards the coordinator-side
> parent/child list (mac802154_process_association_req() and
> friends), which is a different data structure, and it is not held
> while this device is associating.  The writer blocks in
> wait_for_completion_killable_timeout() for up to 10 s after storing
> assoc_dev, so the reader on the rx_mac_cmd workqueue races with both
> the NULL store in clear_assoc and the kfree() of the leftover
> ieee802154_pan_device in mac802154_associate()'s error path.
>
> Add a dedicated assoc_dev_lock and hold it around all three
> accesses; the RESP handler additionally validates the pointer before
> dereferencing it.  This is a per-field lock so it cannot contend
> with the completion logic.
>
> Fixes: fefd19807fe9 ("mac802154: Handle associating")

This likely deserves a `Cc: stable*` tag.

[...]

> @@ -613,7 +615,9 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
>  
>  clear_assoc:
>  	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
> +	spin_lock_bh(&local->assoc_dev_lock);
>  	local->assoc_dev = NULL;
> +	spin_unlock_bh(&local->assoc_dev_lock);

Is _bh really useful here? Isn't the dereferencing always happening in
process context?

> @@ -626,6 +630,7 @@ int mac802154_process_association_resp(struct ieee802154_sub_if_data *sdata,
>  	u64 deaddr = swab64((__force u64)dest->extended_addr);
>  	struct ieee802154_local *local = sdata->local;
>  	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
> +	struct ieee802154_pan_device *assoc_dev;

The below block can be much simpler: just cache the extended addr of the
assoc device for the time of the check.

>  	struct ieee802154_assoc_resp_pl resp_pl = {};
>  
>  	if (skb->len != sizeof(resp_pl))
> @@ -635,9 +640,15 @@ 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_bh(&local->assoc_dev_lock);
> +	assoc_dev = local->assoc_dev;
> +	if (unlikely(!assoc_dev ||
> +		     dest->extended_addr != wpan_dev->extended_addr ||
> +		     src->extended_addr != assoc_dev->extended_addr)) {
> +		spin_unlock_bh(&local->assoc_dev_lock);
>  		return -ENODEV;
> +	}
> +	spin_unlock_bh(&local->assoc_dev_lock);

Actually, even simpler than using a lock, since the only information we
use from assoc_dev is the extended address, why not just caching the
extended address value directly in the local structure?

Thanks,
Miquèl