Re: [PATCH 0/2] driver core: platform: avoid use-after-free on device name

[Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 20, 2025 at 07:26:41PM +0100, Théo Lebrun wrote:
> On Thu Feb 20, 2025 at 5:19 PM CET, Greg Kroah-Hartman wrote:
> > 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@xxxxxxxxxxx/
> >> >> [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.
> 
> I never said the opposite, and I agree.
> However the mechanism exists and I was focused on not breaking it.
> 
> > 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.
> 
> I do get the argument that devices shouldn't change device names. I'll
> take the devil's advocate and give at least one argument FOR allowing
> changing names: prettier names, especially as device names leak into
> userspace through pseudo filesystems.

Then that same driver should have created a prettier name when it
created the device and sent it to the driver core :)

> If we agree that device names shouldn't be changed one a device is
> matched with a driver, then (1) we can remove the pdev->name field and
> (2) `dev_set_name()` should warn when used too late.
> 
> Turn the implicit explicit.
> 
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5a1f05198114..3532b068e32d 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3462,10 +3462,13 @@ static void device_remove_class_symlinks(struct device *dev)
>  int dev_set_name(struct device *dev, const char *fmt, ...)
>  {
>         va_list vargs;
>         int err;
> 
> +       if (dev_WARN_ONCE(dev, dev->driver, "device name is static once matched"))
> +               return -EPERM;

What?  No, this is a platform driver thing, not a driver core thing.
Let's just remove the name pointer in the platform driver structure and
then we can handle the rest from there.

> +
>         va_start(vargs, fmt);
>         err = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
>         va_end(vargs);
>         return err;
>  }
> 
> (Unsure about the exact error code to return.)
> 
> [...]
> 
> > 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.
> 
> The cover letter expands on the quest for those drivers:
> 
> On Tue Feb 18, 2025 at 12:00 PM CET, Théo Lebrun wrote:
> > Out of the 37 drivers that deal with platform devices and do a
> > dev_set_name() call, only one might be affected. That driver is
> > loongson-i2s-plat [0]. All other dev_set_name() calls are on children
> > devices created on the spot. The issue was found on downstream kernels
> > and we don't have what it takes to test loongson-i2s-plat.

out-of-tree drivers don't matter to us :)


> [...]
> >
> >    ⟩ # Finding potential trouble-makers:
> >    ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name
> >
> [...]
> > [0]: https://elixir.bootlin.com/linux/v6.13.2/source/sound/soc/loongson/loongson_i2s_plat.c#L155
> 
> [...]
> 
> > 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.
> 
> That used to exist! I cannot see how it could be a good idea to
> reintroduce the distinction though.
> 
> commit eca3930163ba8884060ce9d9ff5ef0d9b7c7b00f
> Author: Grant Likely <grant.likely@xxxxxxxxxxxx>
> Date:   Tue Jun 8 07:48:21 2010 -0600
> 
>     of: Merge of_platform_bus_type with platform_bus_type

True, that was nice, but we shouldn't let one force bugs in the other :)

Anyway try removing the name pointer and let's see what falls out.

thanks,

greg k-h