Received-SPF: pass (google.com: domain of linux-wireless+bounces-3615-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Subject: Re: [PATCH] wifi: wilc1000: prevent use-after-free on vif when
 cleaning up all interfaces
From: Kalle Valo <kvalo@kernel.org>
In-Reply-To: <20240212-wilc_rework_deinit-v1-1-9203ae56c27f@bootlin.com>
References: <20240212-wilc_rework_deinit-v1-1-9203ae56c27f@bootlin.com>
To: =?utf-8?q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>
Cc: linux-wireless@vger.kernel.org, Ajay Singh <ajay.kathat@microchip.com>,
 Claudiu Beznea <claudiu.beznea@tuxon.dev>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
 David Mosberger-Tang <davidm@egauge.net>, linux-kernel@vger.kernel.org,
 =?utf-8?q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>
User-Agent: pwcli/0.1.1-git (https://github.com/kvalo/pwcli/) Python/3.11.2
Message-ID: <170799534217.3764215.15607229596917179924.kvalo@kernel.org>
Date: Thu, 15 Feb 2024 11:09:03 +0000 (UTC)

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240212-wilc_rework_deinit-v1-1-9203ae56c27f@bootlin.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches