`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 <[email protected]>
---
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
