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