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

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 [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
 drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

index c3b06b515c4f..25ff5dec221c 100644

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

 		aggr_conn->timer_scheduled = false;
 	}
 

Quoted 27 lines · 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 [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
 drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

index c3b06b515c4f..25ff5dec221c 100644

drivers/net/wireless/ath/ath6kl/txrx.c void aggr_reset_state(struct aggr_info_conn *aggr_conn) · line 1831

Quoted 5 lines

drivers/net/wireless/ath/ath6kl/txrx.c +1 -1

@@ -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.

Quoted 39 lines · Jeff Johnson

Quoted 27 lines · 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 [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
  drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

index c3b06b515c4f..25ff5dec221c 100644

drivers/net/wireless/ath/ath6kl/txrx.c void aggr_reset_state(struct aggr_info_conn *aggr_conn) · line 1831

Quoted 5 lines

drivers/net/wireless/ath/ath6kl/txrx.c +1 -1

@@ -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.

Quoted 3 lines

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.

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 [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
  drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

index c3b06b515c4f..25ff5dec221c 100644

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 [email protected]

Signed-off-by Daniel Hodges <[email protected]>

---
  drivers/net/wireless/ath/ath6kl/txrx.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/wireless/ath/ath6kl/txrx.c +0 -0

index c3b06b515c4f..25ff5dec221c 100644

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().

[...]