Received-SPF: pass (google.com: domain of linux-wireless+bounces-2455-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <eb20373c68936f032c322174fbbd3353167d898a.camel@egauge.net>
Subject: Re: [PATCH] wifi: wilc1000: validate chip id during bus probe
From: David Mosberger-Tang <davidm@egauge.net>
Date: Wed, 24 Jan 2024 18:45:27 +0000 (UTC)
In-Reply-To: <ab077dbe58b1ea5de0a3b2ca21f275a07af967d2.camel@egauge.net>
References: <20240122211315.1444880-2-davidm@egauge.net>
	 <20240122220350.1449413-1-davidm@egauge.net>
	 <751bf8e4-c81c-495b-9166-9f91f9c4b2d5@bootlin.com>
	 <b8e8a3f82fe240506e82322a10be7b4e9f218eca.camel@egauge.net>
	 <0d77d857-35ce-43bc-aaf3-2b46c01a44ec@bootlin.com>
	 <ab077dbe58b1ea5de0a3b2ca21f275a07af967d2.camel@egauge.net>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0
To: Alexis =?iso-8859-1?q?Lothor=E9?= <alexis.lothore@bootlin.com>,
	linux-wireless@vger.kernel.org
Cc: Ajay.Kathat@microchip.com, kvalo@kernel.org

On Wed, 2024-01-24 at 10:31 -0700, David Mosberger-Tang wrote:
> Alexis,
>=20
> On Wed, 2024-01-24 at 10:01 +0100, Alexis Lothor=E9 wrote:
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > BUG: KASAN: slab-use-after-free in wilc_netdev_cleanup+0x294/0x2c0
> > Read of size 4 at addr c3c91ce8 by task swapper/1
>=20
> OK, I think I see what's going on: it's the list traversal.  Here is what
> wilc_netdev_cleanup() does:
>=20
> 	list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
> 		if (vif->ndev)
> 			unregister_netdev(vif->ndev);
> 	}
>=20
> 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_rc=
u()
> ends up dereferencing a dangling pointer.
>=20
> 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 wron=
g
with delaying the unregister_netdev() call until after the vif has been rem=
oved
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 =3D =
{
 void wilc_netdev_cleanup(struct wilc *wilc)
 {
 	struct wilc_vif *vif;
-	int srcu_idx, ifc_cnt =3D 0;
+	int ifc_cnt =3D 0;
=20
 	if (!wilc)
 		return;
@@ -894,13 +894,6 @@ void wilc_netdev_cleanup(struct wilc *wilc)
 		wilc->firmware =3D NULL;
 	}
=20
-	srcu_idx =3D 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);
=20
@@ -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);
 	}
=20
 	wilc_wlan_cfg_deinit(wilc);