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

[Xuanqiang Luo <xuanqiang.luo@linux.dev>]

在 2026/8/28 06:13, Kaiwen Shi 写道:
> @@ -599,25 +603,38 @@ int mac802154_perform_association(struct ieee802154_sub_if_data *sdata,
>   		goto clear_assoc;
>   	}
>   
> -	if (local->assoc_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
> -		if (local->assoc_status == IEEE802154_PAN_AT_CAPACITY)
> +	/* The association is complete: clear the associating bit and snapshot
> +	 * the result under the same lock, so a second (e.g. malicious) ASSOC
> +	 * RESP can no longer pass the recheck below and overwrite
> +	 * assoc_status/assoc_addr before they are consumed.
> +	 */
> +	spin_lock(&local->assoc_lock);
> +	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);

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.

> +	resp_status = local->assoc_status;
> +	resp_short_addr = local->assoc_addr;
> +	spin_unlock(&local->assoc_lock);
> +
> +	if (resp_status != IEEE802154_ASSOCIATION_SUCCESSFUL) {
> +		if (resp_status == IEEE802154_PAN_AT_CAPACITY)
>   			ret = -ERANGE;
>   		else
>   			ret = -EPERM;
>   
>   		dev_warn(&sdata->dev->dev,
>   			 "Negative ASSOC RESP received from %8phC: %s\n", &ceaddr,
> -			 local->assoc_status == IEEE802154_PAN_AT_CAPACITY ?
> +			 resp_status == IEEE802154_PAN_AT_CAPACITY ?
>   			 "PAN at capacity" : "access denied");
> -		goto clear_assoc;
> +		return ret;
>   	}
>   
> -	ret = 0;
> -	*short_addr = local->assoc_addr;
> +	*short_addr = resp_short_addr;
> +
> +	return 0;
>   
>   clear_assoc:
> +	spin_lock(&local->assoc_lock);
>   	clear_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing);
> -	local->assoc_dev = NULL;
> +	spin_unlock(&local->assoc_lock);
>   
>   	return ret;
>   }
> @@ -639,19 +656,23 @@ int mac802154_process_association_resp(struct ieee802154_sub_if_data *sdata,
>   		     dest->mode != IEEE802154_EXTENDED_ADDRESSING))
>   		return -EINVAL;
>   
> -	if (unlikely(dest->extended_addr != wpan_dev->extended_addr ||
> -		     src->extended_addr != local->assoc_dev->extended_addr))
> +	spin_lock(&local->assoc_lock);
> +	if (unlikely(!test_bit(IEEE802154_IS_ASSOCIATING, &local->ongoing) ||
> +		     dest->extended_addr != wpan_dev->extended_addr ||
> +		     src->extended_addr != local->assoc_dev_extended_addr)) {
> +		spin_unlock(&local->assoc_lock);
>   		return -ENODEV;
> +	}
>   
>   	memcpy(&resp_pl, skb->data, sizeof(resp_pl));
>   	local->assoc_addr = resp_pl.short_addr;
>   	local->assoc_status = resp_pl.status;

here

> +	complete(&local->assoc_done);
> +	spin_unlock(&local->assoc_lock);
>   
>   	dev_dbg(&skb->dev->dev,
>   		"ASSOC RESP 0x%x received from %8phC, getting short address %04x\n",
> -		local->assoc_status, &deaddr, local->assoc_addr);
> -
> -	complete(&local->assoc_done);
> +		resp_pl.status, &deaddr, resp_pl.short_addr);
>   
>   	return 0;
>   }
>
> base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f

Thanks,
Xuanqiang