| Message ID | 20221124151349.2386077-1-zhangxiaoxu5@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [wireless] wifi: wilc1000: Fix UAF in wilc_netdev_cleanup() when iterator the RCU list | expand |
<Ajay.Kathat@microchip.com> writes: > On 24/11/22 20:43, Zhang Xiaoxu wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > There is a UAF read when remove the wilc1000_spi module: > > BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] > Read of size 8 at addr ffff888116846900 by task rmmod/386 > > CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 > Call Trace: > dump_stack_lvl+0x68/0x85 > print_report+0x16c/0x4a3 > kasan_report+0x95/0x190 > wilc_netdev_cleanup.cold+0xc4/0xe0 > wilc_bus_remove+0x52/0x60 > spi_remove+0x46/0x60 > device_remove+0x73/0xc0 > device_release_driver_internal+0x12d/0x210 > driver_detach+0x84/0x100 > bus_remove_driver+0x90/0x120 > driver_unregister+0x4f/0x80 > __x64_sys_delete_module+0x2fc/0x440 > do_syscall_64+0x38/0x90 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > Since set 'needs_free_netdev=true' when initialize the net device, the > net device will be freed when unregister, then use the freed 'vif' to > find the next will UAF read. > > Did you test this behaviour on the real device. I am seeing a kernel > crash when the module is unloaded after the connection with an AP. As > I see, "vif_list" is used to maintain the interface list, so even when > one interface is removed, another element is fetched from the > "vif_list", not using the freed "vif" Ajay, please don't use HTML as our lists drop those.
On 2022/11/26 0:17, Ajay.Kathat@microchip.com wrote: > > On 24/11/22 20:43, Zhang Xiaoxu wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> There is a UAF read when remove the wilc1000_spi module: >> >> BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] >> Read of size 8 at addr ffff888116846900 by task rmmod/386 >> >> CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 >> Call Trace: >> dump_stack_lvl+0x68/0x85 >> print_report+0x16c/0x4a3 >> kasan_report+0x95/0x190 >> wilc_netdev_cleanup.cold+0xc4/0xe0 >> wilc_bus_remove+0x52/0x60 >> spi_remove+0x46/0x60 >> device_remove+0x73/0xc0 >> device_release_driver_internal+0x12d/0x210 >> driver_detach+0x84/0x100 >> bus_remove_driver+0x90/0x120 >> driver_unregister+0x4f/0x80 >> __x64_sys_delete_module+0x2fc/0x440 >> do_syscall_64+0x38/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Since set 'needs_free_netdev=true' when initialize the net device, the >> net device will be freed when unregister, then use the freed 'vif' to >> find the next will UAF read. > > > Did you test this behaviour on the real device. I am seeing a kernel crash when the module is unloaded after the connection with an AP. Thanks Ajay, I have no real device, what kind of crash about your scenario? > As I see, "vif_list" is used to maintain the interface list, so even when one interface is removed, another element is fetched from the "vif_list", not using the freed "vif" For example if the "vif_list" has device A and device B, just like: A->next = &B B->prev = &A When iterator on the vif_list, 1st: Got A and unregister A, A will be freed since needs_free_netdev=true 2nd: Try get B from A->next, A already freed in the first step, UAF occurred. rcu list no implement the interface like "list_for_each_entry_safe". > > Regards, > Ajay >
"zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com> writes: > On 2022/11/26 0:17, Ajay.Kathat@microchip.com wrote: >> >> On 24/11/22 20:43, Zhang Xiaoxu wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> There is a UAF read when remove the wilc1000_spi module: >>> >>> BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] >>> Read of size 8 at addr ffff888116846900 by task rmmod/386 >>> >>> CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 >>> Call Trace: >>> dump_stack_lvl+0x68/0x85 >>> print_report+0x16c/0x4a3 >>> kasan_report+0x95/0x190 >>> wilc_netdev_cleanup.cold+0xc4/0xe0 >>> wilc_bus_remove+0x52/0x60 >>> spi_remove+0x46/0x60 >>> device_remove+0x73/0xc0 >>> device_release_driver_internal+0x12d/0x210 >>> driver_detach+0x84/0x100 >>> bus_remove_driver+0x90/0x120 >>> driver_unregister+0x4f/0x80 >>> __x64_sys_delete_module+0x2fc/0x440 >>> do_syscall_64+0x38/0x90 >>> entry_SYSCALL_64_after_hwframe+0x63/0xcd >>> >>> Since set 'needs_free_netdev=true' when initialize the net device, the >>> net device will be freed when unregister, then use the freed 'vif' to >>> find the next will UAF read. >> >> >> Did you test this behaviour on the real device. I am seeing a kernel >> crash when the module is unloaded after the connection with an AP. > > Thanks Ajay, I have no real device, what kind of crash about your > scenario? If you don't have a real device to test on, please state that clearly in the commit log. For example, "Compile tested only" or something like that. We get way too much untested patches where there's no indication that they have had no testing. I'm really concerned about this trend, I'm even considering should I just start dropping these kind of untested cleanup patches?
On 2022/11/28 19:14, Kalle Valo wrote: > "zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com> writes: > >> On 2022/11/26 0:17, Ajay.Kathat@microchip.com wrote: >>> >>> On 24/11/22 20:43, Zhang Xiaoxu wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> There is a UAF read when remove the wilc1000_spi module: >>>> >>>> BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] >>>> Read of size 8 at addr ffff888116846900 by task rmmod/386 >>>> >>>> CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 >>>> Call Trace: >>>> dump_stack_lvl+0x68/0x85 >>>> print_report+0x16c/0x4a3 >>>> kasan_report+0x95/0x190 >>>> wilc_netdev_cleanup.cold+0xc4/0xe0 >>>> wilc_bus_remove+0x52/0x60 >>>> spi_remove+0x46/0x60 >>>> device_remove+0x73/0xc0 >>>> device_release_driver_internal+0x12d/0x210 >>>> driver_detach+0x84/0x100 >>>> bus_remove_driver+0x90/0x120 >>>> driver_unregister+0x4f/0x80 >>>> __x64_sys_delete_module+0x2fc/0x440 >>>> do_syscall_64+0x38/0x90 >>>> entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>> >>>> Since set 'needs_free_netdev=true' when initialize the net device, the >>>> net device will be freed when unregister, then use the freed 'vif' to >>>> find the next will UAF read. >>> >>> >>> Did you test this behaviour on the real device. I am seeing a kernel >>> crash when the module is unloaded after the connection with an AP. >> >> Thanks Ajay, I have no real device, what kind of crash about your >> scenario? > > If you don't have a real device to test on, please state that clearly in > the commit log. For example, "Compile tested only" or something like > that. Thanks Kalle, I found this problem with a bpf mock device, and test this patch use the same way. > > We get way too much untested patches where there's no indication that > they have had no testing. I'm really concerned about this trend, I'm > even considering should I just start dropping these kind of untested > cleanup patches? >
"zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com> writes: > On 2022/11/28 19:14, Kalle Valo wrote: >> "zhangxiaoxu (A)" <zhangxiaoxu5@huawei.com> writes: >> >>> On 2022/11/26 0:17, Ajay.Kathat@microchip.com wrote: >>>> >>>> On 24/11/22 20:43, Zhang Xiaoxu wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> There is a UAF read when remove the wilc1000_spi module: >>>>> >>>>> BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] >>>>> Read of size 8 at addr ffff888116846900 by task rmmod/386 >>>>> >>>>> CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 >>>>> Call Trace: >>>>> dump_stack_lvl+0x68/0x85 >>>>> print_report+0x16c/0x4a3 >>>>> kasan_report+0x95/0x190 >>>>> wilc_netdev_cleanup.cold+0xc4/0xe0 >>>>> wilc_bus_remove+0x52/0x60 >>>>> spi_remove+0x46/0x60 >>>>> device_remove+0x73/0xc0 >>>>> device_release_driver_internal+0x12d/0x210 >>>>> driver_detach+0x84/0x100 >>>>> bus_remove_driver+0x90/0x120 >>>>> driver_unregister+0x4f/0x80 >>>>> __x64_sys_delete_module+0x2fc/0x440 >>>>> do_syscall_64+0x38/0x90 >>>>> entry_SYSCALL_64_after_hwframe+0x63/0xcd >>>>> >>>>> Since set 'needs_free_netdev=true' when initialize the net device, the >>>>> net device will be freed when unregister, then use the freed 'vif' to >>>>> find the next will UAF read. >>>> >>>> >>>> Did you test this behaviour on the real device. I am seeing a kernel >>>> crash when the module is unloaded after the connection with an AP. >>> >>> Thanks Ajay, I have no real device, what kind of crash about your >>> scenario? >> >> If you don't have a real device to test on, please state that clearly in >> the commit log. For example, "Compile tested only" or something like >> that. > > Thanks Kalle, > > I found this problem with a bpf mock device, and test this patch use > the same way. That's very good info, please always include that information to the commit log as well. That way there's a better understanding how this patch is found.
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c index 9b319a455b96..d6374bfd7650 100644 --- a/drivers/net/wireless/microchip/wilc1000/netdev.c +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c @@ -879,6 +879,7 @@ void wilc_netdev_cleanup(struct wilc *wilc) { struct wilc_vif *vif; int srcu_idx, ifc_cnt = 0; + LIST_HEAD(list_kill); if (!wilc) return; @@ -888,16 +889,14 @@ void wilc_netdev_cleanup(struct wilc *wilc) wilc->firmware = NULL; } + rtnl_lock(); srcu_idx = srcu_read_lock(&wilc->srcu); list_for_each_entry_rcu(vif, &wilc->vif_list, list) { if (vif->ndev) - unregister_netdev(vif->ndev); + unregister_netdevice_queue(vif->ndev, &list_kill); } 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) { @@ -914,6 +913,12 @@ void wilc_netdev_cleanup(struct wilc *wilc) ifc_cnt++; } + unregister_netdevice_many(&list_kill); + rtnl_unlock(); + + wilc_wfi_deinit_mon_interface(wilc, false); + destroy_workqueue(wilc->hif_workqueue); + wilc_wlan_cfg_deinit(wilc); wlan_deinit_locks(wilc); wiphy_unregister(wilc->wiphy);
There is a UAF read when remove the wilc1000_spi module: BUG: KASAN: use-after-free in wilc_netdev_cleanup.cold+0xc4/0xe0 [wilc1000] Read of size 8 at addr ffff888116846900 by task rmmod/386 CPU: 2 PID: 386 Comm: rmmod Tainted: G N 6.1.0-rc6+ #8 Call Trace: dump_stack_lvl+0x68/0x85 print_report+0x16c/0x4a3 kasan_report+0x95/0x190 wilc_netdev_cleanup.cold+0xc4/0xe0 wilc_bus_remove+0x52/0x60 spi_remove+0x46/0x60 device_remove+0x73/0xc0 device_release_driver_internal+0x12d/0x210 driver_detach+0x84/0x100 bus_remove_driver+0x90/0x120 driver_unregister+0x4f/0x80 __x64_sys_delete_module+0x2fc/0x440 do_syscall_64+0x38/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd Since set 'needs_free_netdev=true' when initialize the net device, the net device will be freed when unregister, then use the freed 'vif' to find the next will UAF read. Let's unregister the net device after removed from wilc list to avoid the UAF. Fixes: 8399918f3056 ("staging: wilc1000: use RCU list to maintain vif interfaces list") Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> --- drivers/net/wireless/microchip/wilc1000/netdev.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)