On Wed, 2024-01-24 at 10:31 -0700, David Mosberger-Tang wrote:
> Alexis,
>
> On Wed, 2024-01-24 at 10:01 +0100, Alexis Lothoré wrote:
> > ==================================================================
> > BUG: KASAN: slab-use-after-free in wilc_netdev_cleanup+0x294/0x2c0
> > Read of size 4 at addr c3c91ce8 by task swapper/1
>
> OK, I think I see what's going on: it's the list traversal. Here is what
> wilc_netdev_cleanup() does:
>
> list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
> if (vif->ndev)
> unregister_netdev(vif->ndev);
> }
>
> The problem is that "vif" is the private part of the netdev, so when the netdev
> is freed, the vif structure is no longer valid and list_for_each_entry_rcu()
> ends up dereferencing a dangling pointer.
>
> Ajay or Alexis, could you propose a fix for this - this is not something I'm
> familiar with.
Actually, after staring at the code a little longer, is there anything wrong
with delaying the unregister_netdev() call until after the vif has been removed
from the vif-list? Something along the lines of the below.
--david
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c
b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 0bf6aef4661e..e78e7d971243 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -884,7 +884,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;
+ int ifc_cnt = 0;
if (!wilc)
return;
@@ -894,13 +894,6 @@ 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) {
- 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);
@@ -918,6 +911,10 @@ void wilc_netdev_cleanup(struct wilc *wilc)
mutex_unlock(&wilc->vif_mutex);
synchronize_srcu(&wilc->srcu);
ifc_cnt++;
+
+ if (vif->ndev)
+ /* vif gets freed as part of this call: */
+ unregister_netdev(vif->ndev);
}
wilc_wlan_cfg_deinit(wilc);
