On 30/08/2026 at 07:05:51 +08, Kaiwen Shi <[email protected]> wrote:
> 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 <[email protected]> > Suggested-by: Xuanqiang Luo <[email protected]> > Fixes: fefd19807fe9 ("mac802154: Handle associating") > Cc: [email protected] > Signed-off-by: Kaiwen Shi <[email protected]> > --- Reviewed-by: Miquel Raynal <[email protected]> Thanks, Miquèl
