Re: [PATCH net] mac802154: fix data race and NULL deref on local->assoc_dev
From: Miquel Raynal <miquel.raynal@bootlin.com>
Date: 2026-08-25 13:15:29
Also in:
lkml
Hi Kaiwen, On 25/08/2026 at 01:59:38 +08, Kaiwen Shi [off-list ref] 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. [...]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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