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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Kaiwen,

>   - cache the coordinator extended address instead of retaining the
>     assoc_dev pointer, as suggested by Miquel;
>   - use a plain spinlock to serialize the cached address and associating
>     bit, including 64-bit address accesses on 32-bit architectures;

Can you justify the choice of a spinlock vs. mutex here? This is an open
question, not a request for changes.

>   - reset the completion under the same lock, recheck the associating bit
>     in the response handler, and complete before releasing the lock;
>   - use the response payload in the debug message;
>   - add Cc: stable@vger.kernel.org.
>

...

> --- a/net/mac802154/scan.c
> +++ b/net/mac802154/scan.c
> @@ -578,9 +578,11 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
>  		return ret;
>  	}
>  
> -	local->assoc_dev = coord;
> +	spin_lock(&local->assoc_lock);
>  	reinit_completion(&local->assoc_done);
> +	local->assoc_dev_extended_addr = coord->extended_addr;
>  	set_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
> +	spin_unlock(&local->assoc_lock);
>  
>  	ret = ieee802154_mlme_tx_one_locked(local, sdata, skb);
>  	if (ret) {

Shouldn't we also make sure that accessing assoc_status/assoc_addr is
serialized? Typically, I believe the IS_ASSOCIATING bit should be
cleared earlier in mac802154_perform_association(), just after the
wait_for_completion call returns. This way, in case we get two responses
for the same request (maybe a malicious one), it will prevent the
possibility to get incoherent assoc_status and assoc_address.

Thanks,
Miquèl