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

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

[Daniel Hodges]

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);
 		aggr_conn->timer_scheduled = false;
 	}
 
-- 
2.52.0

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

[Jeff Johnson]

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.

>  		aggr_conn->timer_scheduled = false;
>  	}
>  

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

[Jeff Johnson]

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.

Gotta love our new AI-driven workflows...

/jeff

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

[Vasanthakumar Thiagarajan]

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

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

[Vasanthakumar Thiagarajan]

On 2/7/2026 12:22 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);
>   		aggr_conn->timer_scheduled = false;
>   	}
>   

Reviewed-by: Vasanthakumar Thiagarajan <vasanthakumar.thiagarajan@oss.qualcomm.com>

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

[Rameshkumar Sundaram]

On 2/7/2026 12:22 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);
>   		aggr_conn->timer_scheduled = false;
>   	}
>   

I am not familiar with ath6kl either, but while looking through the code,

aggr_reset_state() still calls timer_delete_sync() only when
aggr_conn->timer_scheduled is true. However aggr_timeout() clears
timer_scheduled near the beginning of the callback, before it walks
aggr_conn->rx_tid[] and updates aggr_conn->stat[].

So aggr_reset_state() can observe timer_scheduled == false while the
timer callback is still running, skip timer_delete_sync(), and then 
delete the TID state / allow aggr_conn to be freed. That still leaves 
the UAF window open.

I think timer_delete_sync() should be called unconditionally after the
aggr_conn NULL check, and timer_scheduled can then be cleared afterwards.


--
Ramesh

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

[Jeff Johnson]

On Fri, 06 Feb 2026 13:52:07 -0500, 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().
> 
> [...]

Applied, thanks!

[1/1] wifi: ath6kl: fix use-after-free in aggr_reset_state()
      commit: ba7debb4dd6427386862220e8335a53a4bfc235d

Best regards,
-- 
Jeff Johnson <jeff.johnson@oss.qualcomm.com>