Re: [PATCH 4/5] wifi: ath12k: avoid IDR mutation during pending mgmt TX cleanup

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 9/28/2026 1:26 AM, jiale yao wrote:
> At 2026-09-28 13:49:54, "Rameshkumar Sundaram" <rameshkumar.sundaram@oss.qualcomm.com> wrote:
>> On 9/26/2026 9:18 PM, Jiale Yao wrote:
>>> ath12k_mac_tx_mgmt_pending_free() is passed as an idr_for_each()
>>> callback and removes the current entry from txmgmt_idr. This can
>>> invalidate the radix-tree iterator retained by idr_for_each().
>>>
>>> Both callers destroy the IDR immediately after the walk, so removing
>>> each entry in the callback is unnecessary. Split skb release from IDR
>>> removal and let the callback release the supplied skb without updating
>>> the IDR. The following idr_destroy() tears down the IDR itself.
>>>
>>> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
>>> Signed-off-by: Jiale Yao <yaojiale02@163.com>
>>> ---
>>>   drivers/net/wireless/ath/ath12k/mac.c | 25 +++++++++++++++----------
>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>>> index 99bf5cf79d10..7695b21149d1 100644
>>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>>> @@ -9166,18 +9166,11 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
>>>   		wake_up(&ar->txmgmt_empty_waitq);
>>>   }
>>>   
>>> -static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>> +static void ath12k_mac_tx_mgmt_free_skb(struct ath12k *ar,
>>> +					struct sk_buff *msdu)
>>>   {
>>> -	struct sk_buff *msdu;
>>>   	struct ieee80211_tx_info *info;
>>>   
>>> -	spin_lock_bh(&ar->txmgmt_idr_lock);
>>> -	msdu = idr_remove(&ar->txmgmt_idr, buf_id);
>>> -	spin_unlock_bh(&ar->txmgmt_idr_lock);
>>> -
>>> -	if (!msdu)
>>> -		return;
>>> -
>>>   	dma_unmap_single(ar->ab->dev, ATH12K_SKB_CB(msdu)->paddr, msdu->len,
>>>   			 DMA_TO_DEVICE);
>>>   
>>> @@ -9187,11 +9180,23 @@ static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>>   	ath12k_mgmt_over_wmi_tx_drop(ar, msdu);
>>>   }
>>>   
>>> +static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>> +{
>>> +	struct sk_buff *msdu;
>>> +
>>> +	spin_lock_bh(&ar->txmgmt_idr_lock);
>>> +	msdu = idr_remove(&ar->txmgmt_idr, buf_id);
>>> +	spin_unlock_bh(&ar->txmgmt_idr_lock);
>>> +
>>> +	if (msdu)
>>> +		ath12k_mac_tx_mgmt_free_skb(ar, msdu);
>>> +}
>>> +
>>>   int ath12k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx)
>>>   {
>>>   	struct ath12k *ar = ctx;
>>>   
>>> -	ath12k_mac_tx_mgmt_free(ar, buf_id);
>>> +	ath12k_mac_tx_mgmt_free_skb(ar, skb);
>>
>> this now keeps the freed skbs in the idr for the whole time 
>> idr_for_each() runs as opposed to previous implementation which removed 
>> the idr entry as well which looked safe.
> 
> Thanks for the review.
> 
> The original issue is the same as the one fixed by commit c38b1e19485a
> ("firmware: arm_scmi: Avoid IDR updates while cleaning channels").
> ath12k_mac_tx_mgmt_pending_free() is called from idr_for_each(), and
> ath12k_mac_tx_mgmt_free() removes the current entry from that same IDR
> before the iterator advances. This can invalidate the iterator state.
> 
> You are right that my patch leaves freed skb pointers in the IDR until
> idr_destroy(), which is undesirable.
> 
> I will send a v2 using idr_for_each_entry(), which performs a fresh
> idr_get_next() lookup for each iteration. The entry will then be removed
> from the IDR before its skb is freed, avoiding both the invalid iterator
> state and stale skb pointers.
Thanks, I just came here to comment that my AI agent spotted the same thing:

The patch introduces a real UAF window that did not exist in the original
code.

The window:

In the new callback (ath12k_mac_tx_mgmt_pending_free /
ath11k_mac_tx_mgmt_pending_free), ath12k_mac_tx_mgmt_free_skb() frees the skb
but leaves the IDR entry pointing to it. idr_destroy() only runs after the
entire idr_for_each() walk completes. So between the callback freeing skb N
and idr_destroy() removing entry N, the IDR still holds a dangling pointer.

The concurrent path is wmi_process_mgmt_tx_comp() in wmi.c:6436, which does
exactly this sequence:

spin_lock_bh(&ar->txmgmt_idr_lock);
msdu = idr_find(&ar->txmgmt_idr, desc_id);   // returns freed skb pointer
...
idr_remove(&ar->txmgmt_idr, desc_id);
spin_unlock_bh(&ar->txmgmt_idr_lock);

skb_cb = ATH12K_SKB_CB(msdu);                // UAF: msdu already freed
dma_unmap_single(..., skb_cb->paddr, ...);   // UAF

Why the original code was safe:

The old ath12k_mac_tx_mgmt_free() callback called idr_remove() under
txmgmt_idr_lock before freeing the skb. So wmi_process_mgmt_tx_comp would
either win the race (remove and free the skb itself, leaving the IDR entry
absent when the cleanup callback ran) or lose it (find the entry already
removed, print the "invalid msdu_id" warning, and bail). In neither case could
it get a pointer to an already-freed skb.