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

[luoxuanqiang]

在 2026/8/30 07:05, 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.
>
> wpan_dev->association_lock cannot be reused here: nl802154_associate()
> holds it across rdev_associate(), hence for the whole of
> mac802154_perform_association() including the wait for the response.
> A response handler taking that lock would only get it once the
> association has already given up.
>
> Reset the completion, publish the cached address, and set the associating
> bit while holding the lock. The response handler takes the lock, rechecks
> the bit and the cached address, records the response, clears the bit, and
> only then completes the waiter. Thus cleanup cannot pass the handler
> between its state check and completion, and the cached 64-bit value
> cannot tear.
>
> The handler clears the bit before completing, not the woken waiter:
> otherwise complete() is issued under the lock and a second (e.g.
> malicious) response can reacquire it before the waiter and replace the
> result. So a wait that returns success implies the bit is already clear,
> and the success and negative-response paths return directly. The
> transmit-error and timeout paths still clear it under assoc_lock, which
> serializes any racing response against the cleanup while the call returns
> the error it already selected. Both paths snapshot assoc_status and
> assoc_addr under the same lock.
>
> 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@xxxxxxxxxxx>
> Suggested-by: Xuanqiang Luo <xuanqiang.luo@xxxxxxxxx>

Thanks for the update.

I don't think my review warrants a Suggested-by tag here. I only pointed
out a small issue, which did not alter your original approach.

Other than that, the current version looks good to me. Please drop my
Suggested-by tag and feel free to add:

Reviewed-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>

Here are some related discussions about when a Suggested-by tag is
appropriate, for reference:
https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
https://lore.kernel.org/netdev/20260803190044.274355ea@xxxxxxxxxx/
https://lore.kernel.org/netdev/CANn89i+O5b0xfcP5GLtUopqHz2ppewK9zSr4995jsjCPM+3AFQ@xxxxxxxxxxxxxx/
https://lore.kernel.org/netdev/CAAVpQUDaYX5ZQN+EYL3q4yeu0Ni2cqNODEY--Wb-2+yY650Mbw@xxxxxxxxxxxxxx/
https://lore.kernel.org/netdev/20251010065515.GA3115768@xxxxxxxxxxxxxxxx/

Thanks,
Xuanqiang