Received-SPF: pass (google.com: domain of linux-wireless+bounces-2505-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <df243322ee109149c29a69d7c7a952dcc034ed51.camel@egauge.net>
Subject: Re: [PATCH] wifi: wilc1000: validate chip id during bus probe
From: David Mosberger-Tang <davidm@egauge.net>
Date: Thu, 25 Jan 2024 19:17:57 +0000 (UTC)
In-Reply-To: <6fa6c238-9938-4e14-9b99-95759b659147@bootlin.com>
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>
	 <eb20373c68936f032c322174fbbd3353167d898a.camel@egauge.net>
	 <c5118672-7e67-4601-a570-52c8e212aeda@microchip.com>
	 <6fa6c238-9938-4e14-9b99-95759b659147@bootlin.com>
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>,
	Ajay.Kathat@microchip.com
Cc: kvalo@kernel.org, linux-wireless@vger.kernel.org

On Thu, 2024-01-25 at 12:04 +0100, Alexis Lothor=E9 wrote:
> On 1/25/24 07:23, Ajay.Kathat@microchip.com wrote:
> > On 1/24/24 11:45, David Mosberger-Tang wrote:
> >=20
> > > >=20
> > > > OK, I think I see what's going on: it's the list traversal.  Here i=
s 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 whe=
n the netdev
> > > > is freed, the vif structure is no longer valid and list_for_each_en=
try_rcu()
> > > > ends up dereferencing a dangling pointer.
>=20
> Your diagnostic sounds relevant :)

Yeah, it's definitely what's going on.  And it's not just the list traversa=
l:
afterwards, wilc_netdev_cleanup() continues to access the vif structure whi=
le
removing them from the vif-list.

I think the original idea of calling unregister_netdev() is probably the ri=
ght
one as, like you said, you want to remove the device from being visible to =
the
user before tearing down anything else.  If I understood the problem correc=
tly,
the use-after-free caused by this line in wilc_netdev_ifc_init():

	ndev->needs_free_netdev =3D true;

This causes unregister_netdev() to implicitly call free_netdev().  Without =
that
code, I think you could call unregister_netdev() early on (as it is right n=
ow)
and when done with using the vifs, call free_netdev() while avoiding any
dangling references.

In any case, this is definitely not my area of expertise.  I don't fully
understand the motivition behind needs_free_netdev, even after reading
https://docs.kernel.org/networking/netdevices.html - I suspect the use of t=
hat
flag has evolved over the years and the docs may not be entirely up-to-date
anymore.  One driver I looked at (wireless/ath/wil6210/netdev.c) sets
needs_free_netdev only for secondary vifs (i.e., all but the first vif).

Hopefully someone else on this list can figure out what the right solution =
is
here.

Thanks,

  --david