Message-ID: <2025022019-enigmatic-mace-60ca@gregkh>
Date: Thu, 20 Feb 2025 17:19:26 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, Rob Herring <robh@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Grant Likely <grant.likely@...retlab.ca>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, Binbin Zhou <zhoubinbin@...ngson.cn>,
	linux-sound@...r.kernel.org,
	Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
	Grégory Clement <gregory.clement@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Tawfik Bayouk <tawfik.bayouk@...ileye.com>, stable@...r.kernel.org
Subject: Re: [PATCH 0/2] driver core: platform: avoid use-after-free on
 device name

On Thu, Feb 20, 2025 at 04:46:59PM +0100, Théo Lebrun wrote:
> On Thu Feb 20, 2025 at 3:06 PM CET, Greg Kroah-Hartman wrote:
> > On Thu, Feb 20, 2025 at 02:31:29PM +0100, Théo Lebrun wrote:
> >> On Thu Feb 20, 2025 at 1:41 PM CET, Greg Kroah-Hartman wrote:
> >> > On Tue, Feb 18, 2025 at 12:00:11PM +0100, Théo Lebrun wrote:
> >> >> The solution proposed is to add a flag to platform_device that tells if
> >> >> it is responsible for freeing its name. We can then duplicate the
> >> >> device name inside of_device_add() instead of copying the pointer.
> >> >
> >> > Ick.
> >> >
> >> >> What is done elsewhere?
> >> >>  - Platform bus code does a copy of the argument name that is stored
> >> >>    alongside the struct platform_device; see platform_device_alloc()[1].
> >> >>  - Other busses duplicate the device name; either through a dynamic
> >> >>    allocation [2] or through an array embedded inside devices [3].
> >> >>  - Some busses don't have a separate name; when they want a name they
> >> >>    take it from the device [4].
> >> >
> >> > Really ick.
> >> >
> >> > Let's do the right thing here and just get rid of the name pointer
> >> > entirely in struct platform_device please.  Isn't that the correct
> >> > thing that way the driver core logic will work properly for all of this.
> >> 
> >> I would agree, if it wasn't for this consideration that is found in the
> >> commit message [0]:
> >
> > What, that the of code is broken?  Then it should be fixed, why does it
> > need a pointer to a name at all anyway?  It shouldn't be needed there
> > either.
> 
> I cannot guess why it originally has a separate pdev->name field.

Many people got this wrong when we designed busses, it's not unique.
But we should learn from our mistakes where we can :)

> >> > It is important to duplicate! pdev->name must not change to make sure
> >> > the platform_match() return value is stable over time. If we updated
> >> > pdev->name alongside dev->name, once a device probes and changes its
> >> > name then the platform_match() return value would change.
> >> 
> >> I'd be fine sending a V2 that removes the field *and the fallback* [1],
> >> but I don't have the full scope in mind to know what would become broken.
> >> 
> >> [0]: https://lore.kernel.org/lkml/20250218-pdev-uaf-v1-2-5ea1a0d3aba0@bootlin.com/
> >> [1]: https://elixir.bootlin.com/linux/v6.13.3/source/drivers/base/platform.c#L1357
> >
> > The fallback will not need to be removed, properly point to the name of
> > the device and it should work correctly.
> 
> No, it will not work correctly, as the above quote indicates.

I don't know which quote, sorry.

> Let's assume we remove the field, this situation would be broken:
>  - OF allocates platform devices and gives them names.
>  - A device matches with a driver, which gets probed.
>  - During the probe, driver does a dev_set_name().
>  - Afterwards, the upcoming platform_match() against other drivers are
>    called with another device name.
> 
> We should be safe as there are guardraids to not probe twice a device,
> see __driver_probe_device() that checks dev->driver is NULL. But it
> isn't a situation we should be in.

The fragility of attempting to match a driver to a device purely by a
name is a very week part of using platform devices.

Why would a driver change the device name?  It's been given to the
driver to "bind to" not to change its name.  That shouldn't be ok, fix
those drivers.

> Another broken situation:
>  - OF allocates platform devices and gives them names.
>  - A device matches with a driver, which gets probed based on its name.
>  - During the probe, driver does a dev_set_name().

Again, don't do that.  That's the breaking part.

>  - Module is removed.
>  - Module is re-added, the (driver, device) pair don't end up matching
>    again because the device name changed.

Sure, that was a bug in the driver.  It shouldn't be changing the name,
the name is set/owned by the bus, not the driver.

Do we have examples today of platform drivers that like to rename
devices?  I did a quick search and couldn't find any in-tree, but I
might have missed some.

Again, the bus controls the name when the device is created, changing it
after the fact is generally not a good idea.

> I might be missing other edge-cases.
> 
> Conclusion: we need a constant name for platform devices as we want the
> return value of platform_match() to stay stable across time.

No, let's just not rename devices in platform drivers.

Or if this really is an issue, let's fix OF to not use the platform bus
and have it's own bus for stuff like this.

thanks,

greg k-h

Powered by blists - more mailing lists