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

[Kaiwen Shi]

Hi Xuanqiang,

> I think this clear_bit() should be moved to
> mac802154_process_association_resp(), after saving the first valid
> response and before calling complete().
>
> Clearing it only after wait_for_completion() returns may still leave a
> window, since the woken waiter may not acquire assoc_lock before the
> next work item runs. Clearing it earlier in the response handler ensures
> that subsequent responses fail the in-lock
> IEEE802154_IS_ASSOCIATING check and cannot overwrite the saved result.

Good catch, you are right. complete() is issued while the handler still
holds assoc_lock, so between the wake-up and the waiter taking that lock
another response can get in, pass the recheck and replace
assoc_status/assoc_addr before perform_association() has consumed them.

I reproduced it on a debug kernel: on v3 the bit is still set when the
wait returns, later responses keep being accepted after that point, and
the association ends up with an address from one of them instead of the
first one. With the clear_bit() moved, none is accepted.

So v4 does what you suggested: clear_bit() right after the result is
stored and before complete(), both under assoc_lock. The timeout and error
paths still clear it under the lock. The success and negative paths do not
need to any more, because a wait that returns success now means the
handler has already cleared the bit.

One thing I got wrong earlier, while I am at it. In my v2 reply I said
wpan_dev->association_lock is not held on either of the two paths
involved. That was wrong. nl802154_associate() takes it around
rdev_associate(), so it is held for the whole of
mac802154_perform_association(), the wait for the response included. That
rules out reusing it here: the handler would only get that mutex once the
association has already timed out. The v4 commit message states this
correctly.

A v4 follows.

Thanks,
Kaiwen