Re: [PATCH] wifi: ath6kl: fix use-after-free in aggr_reset_state()

[Vasanthakumar Thiagarajan <vasanthakumar.thiagarajan@oss.qualcomm.com>]

On 6/18/2026 7:00 AM, Jeff Johnson wrote:
> On 6/17/2026 6:26 PM, Jeff Johnson wrote:
>> On 2/6/2026 10:52 AM, Daniel Hodges wrote:
>>> The aggr_reset_state() function uses timer_delete() (non-synchronous)
>>> for the aggregation timer before proceeding to delete TID state and
>>> before the structure is freed by callers like aggr_module_destroy().
>>>
>>> If the timer callback (aggr_timeout) is executing when aggr_reset_state()
>>> is called, the callback will continue to access aggr_conn fields like
>>> rx_tid[] and stat[] which may be freed immediately after by
>>> kfree(aggr_info->aggr_conn) in aggr_module_destroy().
>>>
>>> Additionally, the timer callback can re-arm itself via mod_timer() while
>>> aggr_reset_state() is running, creating a more complex race condition.
>>>
>>> Use timer_delete_sync() instead to ensure any running timer callback
>>> has completed before returning.
>>>
>>> Fixes: bdcd81707973 ("Add ath6kl cleaned up driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Daniel Hodges <git@danielhodges.dev>
>>> ---
>>>   drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
>>> index c3b06b515c4f..25ff5dec221c 100644
>>> --- a/drivers/net/wireless/ath/ath6kl/txrx.c
>>> +++ b/drivers/net/wireless/ath/ath6kl/txrx.c
>>> @@ -1828,7 +1828,7 @@ void aggr_reset_state(struct aggr_info_conn *aggr_conn)
>>>   		return;
>>>   
>>>   	if (aggr_conn->timer_scheduled) {
>>> -		timer_delete(&aggr_conn->timer);
>>> +		timer_delete_sync(&aggr_conn->timer);
>>
>> My review agent claims this still doesn't fix the UAF since aggr_timeout() can
>> call mod_timer() to rearm itself and hence the timer can fire again.
>> Instead it suggests timer_shutdown_sync() should be used since that prevents
>> any rearm from taking effect.
>>
>> But I'm not familiar with this driver so I don't know if there are reasons to
>> not use timer_shutdown_sync(), i.e. if the timer will be reused again then
>> timer_setup() will need to be called again.
> 
> Interesting enough, another iteration of the same agent says:
> **The fix is correct.** `timer_delete_sync()` loops until the timer is both
> not-running and not-pending — it handles the re-arm case because after the
> callback calls `mod_timer()`, the sync loop picks that up and cancels it.

Yeah, my understanding is also that timer_delete_sync() will ensure that the
timer will not be pending (including the one armed from the timer callback itself)
or running when it returns with the caller ensuring the timer is not rearmed concurrently.
With that, I think this code change is fine and no need to use timer_shutdown_sync().

Vasanth