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

[Théo Lebrun <theo.lebrun@xxxxxxxxxxx>]

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.

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;
+
        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.
[...]
>
>    ⟩ # 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

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com