在 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 <[email protected]> Suggested-by: Xuanqiang Luo <[email protected]>
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 <[email protected]> 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/[email protected]/ https://lore.kernel.org/netdev/cann89i+o5b0xfcp5gltuopqhz2ppewk9zsr4995jsjcpm+3...@mail.gmail.com/ https://lore.kernel.org/netdev/caavpqudayx5zqn+eyl3q4yeu0ni2cqnodey--wb-2+yy650...@mail.gmail.com/ https://lore.kernel.org/netdev/[email protected]/ Thanks, Xuanqiang
