diff mbox series

wifi: ath12k: fix use-after-free in ath12k_dp_cc_cleanup()

Message ID 20240926020931.3424376-1-quic_ramess@quicinc.com (mailing list archive)
State Superseded
Delegated to: Jeff Johnson
Headers show
Series wifi: ath12k: fix use-after-free in ath12k_dp_cc_cleanup() | expand

Commit Message

Rameshkumar Sundaram Sept. 26, 2024, 2:09 a.m. UTC 
During ath12k module removal, in ath12k_core_deinit(),
ath12k_mac_destroy() un-registers ah->hw from mac80211 and frees
the ah->hw as well as all the ar's in it. After this
ath12k_core_soc_destroy()-> ath12k_dp_free()-> ath12k_dp_cc_cleanup()
tries to access one of the freed ar's from pending skb.

This is because during mac destroy, driver failed to flush few
data packets, which were accessed later in ath12k_dp_cc_cleanup()
and freed, but using ar from the packet led to this use-after-free.

BUG: KASAN: use-after-free in ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
Write of size 4 at addr ffff888150bd3514 by task modprobe/8926
CPU: 0 UID: 0 PID: 8926 Comm: modprobe Not tainted
6.11.0-rc2-wt-ath+ #1746
Hardware name: Intel(R) Client Systems NUC8i7HVK/NUC8i7HVB, BIOS 
HNKBLi70.86A.0067.2021.0528.1339 05/28/2021

Call Trace:
  <TASK>
  dump_stack_lvl+0x7d/0xe0
  print_address_description.constprop.0+0x33/0x3a0
  print_report+0xb5/0x260
  ? kasan_addr_to_slab+0x24/0x80
  kasan_report+0xd8/0x110
  ? ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
  ? ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
  kasan_check_range+0xf3/0x1a0
  __kasan_check_write+0x14/0x20
  ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
  ath12k_dp_free+0x178/0x420 [ath12k]
  ath12k_core_stop+0x176/0x200 [ath12k]
  ath12k_core_deinit+0x13f/0x210 [ath12k]
  ath12k_pci_remove+0xad/0x1c0 [ath12k]
  pci_device_remove+0x9b/0x1b0
  device_remove+0xbf/0x150
  device_release_driver_internal+0x3c3/0x580
  ? __kasan_check_read+0x11/0x20
  driver_detach+0xc4/0x190
  bus_remove_driver+0x130/0x2a0
  driver_unregister+0x68/0x90
  pci_unregister_driver+0x24/0x240
  ? find_module_all+0x13e/0x1e0
  ath12k_pci_exit+0x10/0x20 [ath12k]
  __do_sys_delete_module+0x32c/0x580
  ? module_flags+0x2f0/0x2f0
  ? kmem_cache_free+0xf0/0x410
  ? __fput+0x56f/0xab0
  ? __fput+0x56f/0xab0
  ? debug_smp_processor_id+0x17/0x20
  __x64_sys_delete_module+0x4f/0x70
  x64_sys_call+0x522/0x9f0
  do_syscall_64+0x64/0x130
  entry_SYSCALL_64_after_hwframe+0x4b/0x53
RIP: 0033:0x7f8182c6ac8b

Commit 24de1b7b231c ("wifi: ath12k: fix flush failure in recovery
scenarios") added the change to decrement the pending packets count
in case of recovery which make sense as ah->hw as well all
ar's in it are intact during recovery, but during core deinit there
is no use in decrementing packets count or waking up the empty waitq
as the module is going to be removed also ar from pending skbs
can't be used and the packets should just be released back.

To fix this, add additional vif check before trying to use the
ar from skb->cb, because during core_deinit mac80211 will delete
all the vifs and ath12k will remove these vif's reference
(ath12k_mac_vif_unref()) from pending packets.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00214-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Fixes: 24de1b7b231c ("wifi: ath12k: fix flush failure in recovery scenarios")
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/dp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)


base-commit: d35bb26e150d7fb7434959fad9fcaeaac99906e6

Comments

Jeff Johnson Sept. 30, 2024, 9:05 p.m. UTC | #1 
On 9/25/2024 7:09 PM, Rameshkumar Sundaram wrote:
> During ath12k module removal, in ath12k_core_deinit(),
> ath12k_mac_destroy() un-registers ah->hw from mac80211 and frees
> the ah->hw as well as all the ar's in it. After this
> ath12k_core_soc_destroy()-> ath12k_dp_free()-> ath12k_dp_cc_cleanup()
> tries to access one of the freed ar's from pending skb.
> 
> This is because during mac destroy, driver failed to flush few
> data packets, which were accessed later in ath12k_dp_cc_cleanup()
> and freed, but using ar from the packet led to this use-after-free.
> 
> BUG: KASAN: use-after-free in ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
> Write of size 4 at addr ffff888150bd3514 by task modprobe/8926
> CPU: 0 UID: 0 PID: 8926 Comm: modprobe Not tainted
> 6.11.0-rc2-wt-ath+ #1746
> Hardware name: Intel(R) Client Systems NUC8i7HVK/NUC8i7HVB, BIOS 
> HNKBLi70.86A.0067.2021.0528.1339 05/28/2021
> 
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x7d/0xe0
>   print_address_description.constprop.0+0x33/0x3a0
>   print_report+0xb5/0x260
>   ? kasan_addr_to_slab+0x24/0x80
>   kasan_report+0xd8/0x110
>   ? ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
>   ? ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
>   kasan_check_range+0xf3/0x1a0
>   __kasan_check_write+0x14/0x20
>   ath12k_dp_cc_cleanup.part.0+0x5e2/0xd40 [ath12k]
>   ath12k_dp_free+0x178/0x420 [ath12k]
>   ath12k_core_stop+0x176/0x200 [ath12k]
>   ath12k_core_deinit+0x13f/0x210 [ath12k]
>   ath12k_pci_remove+0xad/0x1c0 [ath12k]
>   pci_device_remove+0x9b/0x1b0
>   device_remove+0xbf/0x150
>   device_release_driver_internal+0x3c3/0x580
>   ? __kasan_check_read+0x11/0x20
>   driver_detach+0xc4/0x190
>   bus_remove_driver+0x130/0x2a0
>   driver_unregister+0x68/0x90
>   pci_unregister_driver+0x24/0x240
>   ? find_module_all+0x13e/0x1e0
>   ath12k_pci_exit+0x10/0x20 [ath12k]
>   __do_sys_delete_module+0x32c/0x580
>   ? module_flags+0x2f0/0x2f0
>   ? kmem_cache_free+0xf0/0x410
>   ? __fput+0x56f/0xab0
>   ? __fput+0x56f/0xab0
>   ? debug_smp_processor_id+0x17/0x20
>   __x64_sys_delete_module+0x4f/0x70
>   x64_sys_call+0x522/0x9f0
>   do_syscall_64+0x64/0x130
>   entry_SYSCALL_64_after_hwframe+0x4b/0x53
> RIP: 0033:0x7f8182c6ac8b
> 
> Commit 24de1b7b231c ("wifi: ath12k: fix flush failure in recovery
> scenarios") added the change to decrement the pending packets count
> in case of recovery which make sense as ah->hw as well all
> ar's in it are intact during recovery, but during core deinit there
> is no use in decrementing packets count or waking up the empty waitq
> as the module is going to be removed also ar from pending skbs
> can't be used and the packets should just be released back.
> 
> To fix this, add additional vif check before trying to use the
> ar from skb->cb, because during core_deinit mac80211 will delete
> all the vifs and ath12k will remove these vif's reference
> (ath12k_mac_vif_unref()) from pending packets.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00214-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Fixes: 24de1b7b231c ("wifi: ath12k: fix flush failure in recovery scenarios")
> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/dp.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
> index 61aa78d8bd8c..09363ca15701 100644
> --- a/drivers/net/wireless/ath/ath12k/dp.c
> +++ b/drivers/net/wireless/ath/ath12k/dp.c
> @@ -1203,9 +1203,14 @@ static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
>  				continue;
>  
>  			skb_cb = ATH12K_SKB_CB(skb);
> -			ar = skb_cb->ar;
> -			if (atomic_dec_and_test(&ar->dp.num_tx_pending))
> -				wake_up(&ar->dp.tx_empty_waitq);
> +			/* skb_cb's ar is valid only if the skb is associated with
> +			 * a valid vif.
> +			 */
> +			if (skb_cb->vif) {
> +				ar = skb_cb->ar;
> +				if (atomic_dec_and_test(&ar->dp.num_tx_pending))
> +					wake_up(&ar->dp.tx_empty_waitq);
> +			}
>  
>  			dma_unmap_single(ab->dev, ATH12K_SKB_CB(skb)->paddr,
>  					 skb->len, DMA_TO_DEVICE);
> 
> base-commit: d35bb26e150d7fb7434959fad9fcaeaac99906e6

dropping this since there will be a v2
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/dp.c b/drivers/net/wireless/ath/ath12k/dp.c
index 61aa78d8bd8c..09363ca15701 100644
--- a/drivers/net/wireless/ath/ath12k/dp.c
+++ b/drivers/net/wireless/ath/ath12k/dp.c
@@ -1203,9 +1203,14 @@  static void ath12k_dp_cc_cleanup(struct ath12k_base *ab)
 				continue;
 
 			skb_cb = ATH12K_SKB_CB(skb);
-			ar = skb_cb->ar;
-			if (atomic_dec_and_test(&ar->dp.num_tx_pending))
-				wake_up(&ar->dp.tx_empty_waitq);
+			/* skb_cb's ar is valid only if the skb is associated with
+			 * a valid vif.
+			 */
+			if (skb_cb->vif) {
+				ar = skb_cb->ar;
+				if (atomic_dec_and_test(&ar->dp.num_tx_pending))
+					wake_up(&ar->dp.tx_empty_waitq);
+			}
 
 			dma_unmap_single(ab->dev, ATH12K_SKB_CB(skb)->paddr,
 					 skb->len, DMA_TO_DEVICE);