diff mbox series

wifi: wilc1000: prevent use-after-free on vif when cleaning up all interfaces

Message ID 20240212-wilc_rework_deinit-v1-1-9203ae56c27f@bootlin.com
State New
Headers show
Series wifi: wilc1000: prevent use-after-free on vif when cleaning up all interfaces | expand

Commit Message

Alexis Lothoré (eBPF Foundation) Feb. 12, 2024, 12:57 p.m. UTC 
wilc_netdev_cleanup currently triggers a KASAN warning, which can be
observed on interface registration error path, or simply by
removing the module/unbinding device from driver:

echo spi0.1 > /sys/bus/spi/drivers/wilc1000_spi/unbind

==================================================================
BUG: KASAN: slab-use-after-free in wilc_netdev_cleanup+0x508/0x5cc
Read of size 4 at addr c54d1ce8 by task sh/86

CPU: 0 PID: 86 Comm: sh Not tainted 6.8.0-rc1+ #117
Hardware name: Atmel SAMA5
 unwind_backtrace from show_stack+0x18/0x1c
 show_stack from dump_stack_lvl+0x34/0x58
 dump_stack_lvl from print_report+0x154/0x500
 print_report from kasan_report+0xac/0xd8
 kasan_report from wilc_netdev_cleanup+0x508/0x5cc
 wilc_netdev_cleanup from wilc_bus_remove+0xc8/0xec
 wilc_bus_remove from spi_remove+0x8c/0xac
 spi_remove from device_release_driver_internal+0x434/0x5f8
 device_release_driver_internal from unbind_store+0xbc/0x108
 unbind_store from kernfs_fop_write_iter+0x398/0x584
 kernfs_fop_write_iter from vfs_write+0x728/0xf88
 vfs_write from ksys_write+0x110/0x1e4
 ksys_write from ret_fast_syscall+0x0/0x1c

[...]

Allocated by task 1:
 kasan_save_track+0x30/0x5c
 __kasan_kmalloc+0x8c/0x94
 __kmalloc_node+0x1cc/0x3e4
 kvmalloc_node+0x48/0x180
 alloc_netdev_mqs+0x68/0x11dc
 alloc_etherdev_mqs+0x28/0x34
 wilc_netdev_ifc_init+0x34/0x8ec
 wilc_cfg80211_init+0x690/0x910
 wilc_bus_probe+0xe0/0x4a0
 spi_probe+0x158/0x1b0
 really_probe+0x270/0xdf4
 __driver_probe_device+0x1dc/0x580
 driver_probe_device+0x60/0x140
 __driver_attach+0x228/0x5d4
 bus_for_each_dev+0x13c/0x1a8
 bus_add_driver+0x2a0/0x608
 driver_register+0x24c/0x578
 do_one_initcall+0x180/0x310
 kernel_init_freeable+0x424/0x484
 kernel_init+0x20/0x148
 ret_from_fork+0x14/0x28

Freed by task 86:
 kasan_save_track+0x30/0x5c
 kasan_save_free_info+0x38/0x58
 __kasan_slab_free+0xe4/0x140
 kfree+0xb0/0x238
 device_release+0xc0/0x2a8
 kobject_put+0x1d4/0x46c
 netdev_run_todo+0x8fc/0x11d0
 wilc_netdev_cleanup+0x1e4/0x5cc
 wilc_bus_remove+0xc8/0xec
 spi_remove+0x8c/0xac
 device_release_driver_internal+0x434/0x5f8
 unbind_store+0xbc/0x108
 kernfs_fop_write_iter+0x398/0x584
 vfs_write+0x728/0xf88
 ksys_write+0x110/0x1e4
 ret_fast_syscall+0x0/0x1c
 [...]

David Mosberger-Tan initial investigation [1] showed that this
use-after-free is due to netdevice unregistration during vif list
traversal. When unregistering a net device, since the needs_free_netdev has
been set to true during registration, the netdevice object is also freed,
and as a consequence, the corresponding vif object too, since it is
attached to it as private netdevice data. The next occurrence of the loop
then tries to access freed vif pointer to the list to move forward in the
list.

Fix this use-after-free thanks to two mechanisms:
- navigate in the list with list_for_each_entry_safe, which allows to
  safely modify the list as we go through each element. For each element,
  remove it from the list with list_del_rcu
- make sure to wait for RCU grace period end after each vif removal to make
  sure it is safe to free the corresponding vif too (through
  unregister_netdev)

Since we are in a RCU "modifier" path (not a "reader" path), and because
such path is expected not to be concurrent to any other modifier (we are
using the vif_mutex lock), we do not need to use RCU list API, that's why
we can benefit from list_for_each_entry_safe.

[1] https://lore.kernel.org/linux-wireless/ab077dbe58b1ea5de0a3b2ca21f275a07af967d2.camel@egauge.net/

Fixes: 8399918f3056 ("staging: wilc1000: use RCU list to maintain vif interfaces list")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 drivers/net/wireless/microchip/wilc1000/netdev.c | 28 ++++++------------------
 1 file changed, 7 insertions(+), 21 deletions(-)


---
base-commit: d55b20fe9b6f0ec5552a3071ae63304978d70500
change-id: 20240209-wilc_rework_deinit-3c5347df9b17

Best regards,

Comments

Kalle Valo Feb. 15, 2024, 11:09 a.m. UTC | #1 
Alexis Lothoré <alexis.lothore@bootlin.com> wrote:

> wilc_netdev_cleanup currently triggers a KASAN warning, which can be
> observed on interface registration error path, or simply by
> removing the module/unbinding device from driver:
> 
> echo spi0.1 > /sys/bus/spi/drivers/wilc1000_spi/unbind
> 
> ==================================================================
> BUG: KASAN: slab-use-after-free in wilc_netdev_cleanup+0x508/0x5cc
> Read of size 4 at addr c54d1ce8 by task sh/86
> 
> CPU: 0 PID: 86 Comm: sh Not tainted 6.8.0-rc1+ #117
> Hardware name: Atmel SAMA5
>  unwind_backtrace from show_stack+0x18/0x1c
>  show_stack from dump_stack_lvl+0x34/0x58
>  dump_stack_lvl from print_report+0x154/0x500
>  print_report from kasan_report+0xac/0xd8
>  kasan_report from wilc_netdev_cleanup+0x508/0x5cc
>  wilc_netdev_cleanup from wilc_bus_remove+0xc8/0xec
>  wilc_bus_remove from spi_remove+0x8c/0xac
>  spi_remove from device_release_driver_internal+0x434/0x5f8
>  device_release_driver_internal from unbind_store+0xbc/0x108
>  unbind_store from kernfs_fop_write_iter+0x398/0x584
>  kernfs_fop_write_iter from vfs_write+0x728/0xf88
>  vfs_write from ksys_write+0x110/0x1e4
>  ksys_write from ret_fast_syscall+0x0/0x1c
> 
> [...]
> 
> Allocated by task 1:
>  kasan_save_track+0x30/0x5c
>  __kasan_kmalloc+0x8c/0x94
>  __kmalloc_node+0x1cc/0x3e4
>  kvmalloc_node+0x48/0x180
>  alloc_netdev_mqs+0x68/0x11dc
>  alloc_etherdev_mqs+0x28/0x34
>  wilc_netdev_ifc_init+0x34/0x8ec
>  wilc_cfg80211_init+0x690/0x910
>  wilc_bus_probe+0xe0/0x4a0
>  spi_probe+0x158/0x1b0
>  really_probe+0x270/0xdf4
>  __driver_probe_device+0x1dc/0x580
>  driver_probe_device+0x60/0x140
>  __driver_attach+0x228/0x5d4
>  bus_for_each_dev+0x13c/0x1a8
>  bus_add_driver+0x2a0/0x608
>  driver_register+0x24c/0x578
>  do_one_initcall+0x180/0x310
>  kernel_init_freeable+0x424/0x484
>  kernel_init+0x20/0x148
>  ret_from_fork+0x14/0x28
> 
> Freed by task 86:
>  kasan_save_track+0x30/0x5c
>  kasan_save_free_info+0x38/0x58
>  __kasan_slab_free+0xe4/0x140
>  kfree+0xb0/0x238
>  device_release+0xc0/0x2a8
>  kobject_put+0x1d4/0x46c
>  netdev_run_todo+0x8fc/0x11d0
>  wilc_netdev_cleanup+0x1e4/0x5cc
>  wilc_bus_remove+0xc8/0xec
>  spi_remove+0x8c/0xac
>  device_release_driver_internal+0x434/0x5f8
>  unbind_store+0xbc/0x108
>  kernfs_fop_write_iter+0x398/0x584
>  vfs_write+0x728/0xf88
>  ksys_write+0x110/0x1e4
>  ret_fast_syscall+0x0/0x1c
>  [...]
> 
> David Mosberger-Tan initial investigation [1] showed that this
> use-after-free is due to netdevice unregistration during vif list
> traversal. When unregistering a net device, since the needs_free_netdev has
> been set to true during registration, the netdevice object is also freed,
> and as a consequence, the corresponding vif object too, since it is
> attached to it as private netdevice data. The next occurrence of the loop
> then tries to access freed vif pointer to the list to move forward in the
> list.
> 
> Fix this use-after-free thanks to two mechanisms:
> - navigate in the list with list_for_each_entry_safe, which allows to
>   safely modify the list as we go through each element. For each element,
>   remove it from the list with list_del_rcu
> - make sure to wait for RCU grace period end after each vif removal to make
>   sure it is safe to free the corresponding vif too (through
>   unregister_netdev)
> 
> Since we are in a RCU "modifier" path (not a "reader" path), and because
> such path is expected not to be concurrent to any other modifier (we are
> using the vif_mutex lock), we do not need to use RCU list API, that's why
> we can benefit from list_for_each_entry_safe.
> 
> [1] https://lore.kernel.org/linux-wireless/ab077dbe58b1ea5de0a3b2ca21f275a07af967d2.camel@egauge.net/
> 
> Fixes: 8399918f3056 ("staging: wilc1000: use RCU list to maintain vif interfaces list")
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

Patch applied to wireless-next.git, thanks.

cb5942b77c05 wifi: wilc1000: prevent use-after-free on vif when cleaning up all interfaces
diff mbox series

Patch

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index ef22bf6bf86a..8bae0f2485be 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -890,8 +890,7 @@  static const struct net_device_ops wilc_netdev_ops = {
 
 void wilc_netdev_cleanup(struct wilc *wilc)
 {
-	struct wilc_vif *vif;
-	int srcu_idx, ifc_cnt = 0;
+	struct wilc_vif *vif, *vif_tmp;
 
 	if (!wilc)
 		return;
@@ -901,32 +900,19 @@  void wilc_netdev_cleanup(struct wilc *wilc)
 		wilc->firmware = NULL;
 	}
 
-	srcu_idx = srcu_read_lock(&wilc->srcu);
-	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
+	list_for_each_entry_safe(vif, vif_tmp, &wilc->vif_list, list) {
+		mutex_lock(&wilc->vif_mutex);
+		list_del_rcu(&vif->list);
+		wilc->vif_num--;
+		mutex_unlock(&wilc->vif_mutex);
+		synchronize_srcu(&wilc->srcu);
 		if (vif->ndev)
 			unregister_netdev(vif->ndev);
 	}
-	srcu_read_unlock(&wilc->srcu, srcu_idx);
 
 	wilc_wfi_deinit_mon_interface(wilc, false);
 	destroy_workqueue(wilc->hif_workqueue);
 
-	while (ifc_cnt < WILC_NUM_CONCURRENT_IFC) {
-		mutex_lock(&wilc->vif_mutex);
-		if (wilc->vif_num <= 0) {
-			mutex_unlock(&wilc->vif_mutex);
-			break;
-		}
-		vif = wilc_get_wl_to_vif(wilc);
-		if (!IS_ERR(vif))
-			list_del_rcu(&vif->list);
-
-		wilc->vif_num--;
-		mutex_unlock(&wilc->vif_mutex);
-		synchronize_srcu(&wilc->srcu);
-		ifc_cnt++;
-	}
-
 	wilc_wlan_cfg_deinit(wilc);
 	wlan_deinit_locks(wilc);
 	wiphy_unregister(wilc->wiphy);