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

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

[Théo Lebrun]

The use-after-free bug appears when:
 - A platform device is created from OF, by of_device_add();
 - The same device's name is changed afterwards using dev_set_name(),
   by its probe for example.

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.

Note: loongson-i2s-plat maintainers are CCed.

   ⟩ # Finding potential trouble-makers:
   ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name

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.

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].

[0]: https://elixir.bootlin.com/linux/v6.13.2/source/sound/soc/loongson/loongson_i2s_plat.c#L155
[1]: https://elixir.bootlin.com/linux/v6.13.2/source/drivers/base/platform.c#L581
[2]: https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/drm_drv.c#L679
[3]: https://elixir.bootlin.com/linux/v6.13.2/source/include/linux/i2c.h#L343
[4]: https://elixir.bootlin.com/linux/v6.13.2/source/include/linux/pci.h#L2150

This can be reproduced using Buildroot's qemu_aarch64_virt_defconfig
with CONFIG_KASAN=y and a dev_set_name() inside the probe of:
drivers/pci/controller/pci-host-common.c

The below splat appears at boot. It happens whenever something tries to
access pdev->name; one big consumer of this field is platform_match()
that fallbacks to name matching.

   ==================================================================
   BUG: KASAN: slab-use-after-free in strcmp+0x2c/0x78
   Read of size 1 at addr ffffff80c0300160 by task swapper/0/1

   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.6.32 #1
   Hardware name: linux,dummy-virt (DT)
   Call trace:
    dump_backtrace+0x90/0xe8
    show_stack+0x18/0x24
    dump_stack_lvl+0x48/0x60
    print_report+0xf8/0x5d8
    kasan_report+0x90/0xcc
    __asan_load1+0x60/0x6c
    strcmp+0x2c/0x78
    platform_match+0xd0/0x140
    __driver_attach+0x44/0x240
    bus_for_each_dev+0xe4/0x160
    driver_attach+0x34/0x44
    bus_add_driver+0x134/0x270
    driver_register+0xa4/0x1e4
    __platform_driver_register+0x44/0x54
    ged_driver_init+0x1c/0x28
    do_one_initcall+0xdc/0x260
    kernel_init_freeable+0x314/0x448
    kernel_init+0x2c/0x1e0
    ret_from_fork+0x10/0x20

   Allocated by task 1:
    kasan_save_stack+0x3c/0x64
    kasan_set_track+0x2c/0x40
    kasan_save_alloc_info+0x24/0x34
    __kasan_kmalloc+0xb8/0xbc
    __kmalloc_node_track_caller+0x64/0xa4
    kvasprintf+0xcc/0x16c
    kvasprintf_const+0xe8/0x180
    kobject_set_name_vargs+0x54/0xd4
    dev_set_name+0xa8/0xe4
    of_device_make_bus_id+0x298/0x2b0
    of_device_alloc+0x1ec/0x204
    of_platform_device_create_pdata+0x60/0x168
    of_platform_bus_create+0x20c/0x4a0
    of_platform_populate+0x50/0x10c
    of_platform_default_populate_init+0xe0/0x100
    do_one_initcall+0xdc/0x260
    kernel_init_freeable+0x314/0x448
    kernel_init+0x2c/0x1e0
    ret_from_fork+0x10/0x20

   Freed by task 1:
    kasan_save_stack+0x3c/0x64
    kasan_set_track+0x2c/0x40
    kasan_save_free_info+0x38/0x60
    __kasan_slab_free+0xe4/0x150
    __kmem_cache_free+0x134/0x26c
    kfree+0x54/0x6c
    kfree_const+0x34/0x40
    kobject_set_name_vargs+0xa8/0xd4
    dev_set_name+0xa8/0xe4
    pci_host_common_probe+0x9c/0x294
    platform_probe+0x90/0x100
    really_probe+0x100/0x3cc
    __driver_probe_device+0xb8/0x18c
    driver_probe_device+0x108/0x1d8
    __driver_attach+0xc8/0x240
    bus_for_each_dev+0xe4/0x160
    driver_attach+0x34/0x44
    bus_add_driver+0x134/0x270
    driver_register+0xa4/0x1e4
    __platform_driver_register+0x44/0x54
    gen_pci_driver_init+0x1c/0x28
    do_one_initcall+0xdc/0x260
    kernel_init_freeable+0x314/0x448
    kernel_init+0x2c/0x1e0
    ret_from_fork+0x10/0x20

   The buggy address belongs to the object at ffffff80c0300160
    which belongs to the cache kmalloc-16 of size 16
   The buggy address is located 0 bytes inside of
    freed 16-byte region [ffffff80c0300160, ffffff80c0300170)

   The buggy address belongs to the physical page:
   page:0000000099fe29a0 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x100300
   flags: 0x8000000000000800(slab|zone=2)
   page_type: 0xffffffff()
   raw: 8000000000000800 ffffff80c00013c0 dead000000000122 0000000000000000
   raw: 0000000000000000 0000000080800080 00000001ffffffff 0000000000000000
   page dumped because: kasan: bad access detected

   Memory state around the buggy address:
    ffffff80c0300000: fa fb fc fc fa fb fc fc 00 07 fc fc 00 07 fc fc
    ffffff80c0300080: 00 07 fc fc 00 02 fc fc 00 02 fc fc 00 02 fc fc
   >ffffff80c0300100: 00 06 fc fc 00 06 fc fc 00 06 fc fc fa fb fc fc
                                                          ^
    ffffff80c0300180: 00 00 fc fc 00 00 fc fc 00 06 fc fc 00 06 fc fc
    ffffff80c0300200: 00 06 fc fc 00 06 fc fc 00 06 fc fc 00 06 fc fc
   ==================================================================

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (2):
      driver core: platform: turn pdev->id_auto into pdev->flags
      driver core: platform: avoid use-after-free on pdev->name

 drivers/base/platform.c         |  8 +++++---
 drivers/of/platform.c           | 12 +++++++++++-
 include/linux/platform_device.h |  4 +++-
 3 files changed, 19 insertions(+), 5 deletions(-)
---
base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
change-id: 20250217-pdev-uaf-1a779a98d81b

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH 1/2] driver core: platform: turn pdev->id_auto into pdev->flags

[Théo Lebrun]

struct platform_device->id_auto is the only boolean stored inside the
structure. Remove it and add an u8 flags field. The goal is to allow
more flags (without using more memory).

Cc: <stable@vger.kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/base/platform.c         | 6 +++---
 include/linux/platform_device.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6f2a33722c5203ac196a6e36e153648d0fe6c6d4..e2284482c7ba7c12fe2ab3c715e7d1daa3f65021 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -682,7 +682,7 @@ int platform_device_add(struct platform_device *pdev)
 		if (ret < 0)
 			return ret;
 		pdev->id = ret;
-		pdev->id_auto = true;
+		pdev->flags |= PLATFORM_DEVICE_FLAG_ID_AUTO;
 		dev_set_name(dev, "%s.%d.auto", pdev->name, pdev->id);
 		break;
 	}
@@ -720,7 +720,7 @@ int platform_device_add(struct platform_device *pdev)
 	return 0;
 
  failed:
-	if (pdev->id_auto) {
+	if (pdev->flags & PLATFORM_DEVICE_FLAG_ID_AUTO) {
 		ida_free(&platform_devid_ida, pdev->id);
 		pdev->id = PLATFORM_DEVID_AUTO;
 	}
@@ -750,7 +750,7 @@ void platform_device_del(struct platform_device *pdev)
 	if (!IS_ERR_OR_NULL(pdev)) {
 		device_del(&pdev->dev);
 
-		if (pdev->id_auto) {
+		if (pdev->flags & PLATFORM_DEVICE_FLAG_ID_AUTO) {
 			ida_free(&platform_devid_ida, pdev->id);
 			pdev->id = PLATFORM_DEVID_AUTO;
 		}
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 074754c23d330c9a099e20eecfeb6cbd5025e04f..d842b21ba3791f974fa62f52bd160ef5820261c1 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -23,7 +23,8 @@ struct platform_device_id;
 struct platform_device {
 	const char	*name;
 	int		id;
-	bool		id_auto;
+	u8		flags;
+#define PLATFORM_DEVICE_FLAG_ID_AUTO	BIT(0)
 	struct device	dev;
 	u64		platform_dma_mask;
 	struct device_dma_parameters dma_parms;

-- 
2.48.1

[PATCH 2/2] driver core: platform: avoid use-after-free on pdev->name

[Théo Lebrun]

The issue is with this:

	int of_device_add(struct platform_device *ofdev)
	{
		// ...
		ofdev->name = dev_name(&ofdev->dev);
		// ...
	}

We store the current device name pointer. If the device name changes
through a `dev_set_name(dev, "foo")` call:
 - old device name is freed: kfree(dev->name);
 - new device name is allocated: kmalloc(...);
 - notice pdev->name is still the old device name, ie a freed pointer.

OF is at fault here, taking the pointer to the device name in
of_device_add().

The new PLATFORM_DEVICE_FLAG_FREE_NAME flag tells platform devices if
they own their pdev->name pointer and if it requires a kfree() call.

Considerations:

 - The generic case in platform_device_register_full() is not faulty
   because it allocates memory for storing the name adjacent to the
   `struct platform_device` alloc; see platform_device_alloc():

	struct platform_object *pa;
	pa = kzalloc(sizeof(*pa) + strlen(name) + 1, GFP_KERNEL);

   We cannot rely on this codepath in all cases because OF wants to
   change the name after the platform device creation.

 - kfree_const() cannot solve the issue: either we allocated pdev->name
   separately or it is part of the platform_object allocation.
   pdev->name is never coming from read-only data.

 - 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.

 - In of_device_add(), we make sure to kstrdup() the new name before
   freeing the old one; if alloc fails, we leave the device as-is.

Fixes: eca3930163ba ("of: Merge of_platform_bus_type with platform_bus_type")
Cc: <stable@vger.kernel.org>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/base/platform.c         |  2 ++
 drivers/of/platform.c           | 12 +++++++++++-
 include/linux/platform_device.h |  1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e2284482c7ba7c12fe2ab3c715e7d1daa3f65021..3548714d6ba408abc6c7ab0f3e7496c6e27ba060 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -563,6 +563,8 @@ static void platform_device_release(struct device *dev)
 	kfree(pa->pdev.mfd_cell);
 	kfree(pa->pdev.resource);
 	kfree(pa->pdev.driver_override);
+	if (pa->pdev.flags & PLATFORM_DEVICE_FLAG_FREE_NAME)
+		kfree(pa->pdev.name);
 	kfree(pa);
 }
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c6d8afb284e88061eb6fb0ba02e429cec702664c..ef6f341fd9b77a9e0ed6969c3f322b9bc91d0e8d 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -44,11 +44,21 @@ EXPORT_SYMBOL(of_find_device_by_node);
 
 int of_device_add(struct platform_device *ofdev)
 {
+	char *new_name;
+
 	BUG_ON(ofdev->dev.of_node == NULL);
 
+	new_name = kstrdup(dev_name(&ofdev->dev), GFP_KERNEL);
+	if (!new_name)
+		return -ENOMEM;
+
+	if (ofdev->flags & PLATFORM_DEVICE_FLAG_FREE_NAME)
+		kfree(ofdev->name);
+
 	/* name and id have to be set so that the platform bus doesn't get
 	 * confused on matching */
-	ofdev->name = dev_name(&ofdev->dev);
+	ofdev->name = new_name;
+	ofdev->flags |= PLATFORM_DEVICE_FLAG_FREE_NAME;
 	ofdev->id = PLATFORM_DEVID_NONE;
 
 	/*
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d842b21ba3791f974fa62f52bd160ef5820261c1..203016afc3899ffa05f38b9d4ce3bfc02d5b75ef 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -25,6 +25,7 @@ struct platform_device {
 	int		id;
 	u8		flags;
 #define PLATFORM_DEVICE_FLAG_ID_AUTO	BIT(0)
+#define PLATFORM_DEVICE_FLAG_FREE_NAME	BIT(1)
 	struct device	dev;
 	u64		platform_dma_mask;
 	struct device_dma_parameters dma_parms;

-- 
2.48.1

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

[Greg Kroah-Hartman]

On Tue, Feb 18, 2025 at 12:00:11PM +0100, Théo Lebrun wrote:
> The use-after-free bug appears when:
>  - A platform device is created from OF, by of_device_add();
>  - The same device's name is changed afterwards using dev_set_name(),
>    by its probe for example.
> 
> 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.
> 
> Note: loongson-i2s-plat maintainers are CCed.
> 
>    ⟩ # Finding potential trouble-makers:
>    ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name
> 
> 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.

thanks,

greg k-h

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

[Théo Lebrun]

Hello Greg,

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 use-after-free bug appears when:
>>  - A platform device is created from OF, by of_device_add();
>>  - The same device's name is changed afterwards using dev_set_name(),
>>    by its probe for example.
>> 
>> 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.
>> 
>> Note: loongson-i2s-plat maintainers are CCed.
>> 
>>    ⟩ # Finding potential trouble-makers:
>>    ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name
>> 
>> 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]:

> 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

Regards,

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

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

[Greg Kroah-Hartman]

On Thu, Feb 20, 2025 at 02:31:29PM +0100, Théo Lebrun wrote:
> Hello Greg,
> 
> 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 use-after-free bug appears when:
> >>  - A platform device is created from OF, by of_device_add();
> >>  - The same device's name is changed afterwards using dev_set_name(),
> >>    by its probe for example.
> >> 
> >> 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.
> >> 
> >> Note: loongson-i2s-plat maintainers are CCed.
> >> 
> >>    ⟩ # Finding potential trouble-makers:
> >>    ⟩ git grep -l 'struct platform_device' | xargs grep -l dev_set_name
> >> 
> >> 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.

> > 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.

thanks,

greg k-h

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

[Théo Lebrun]

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.
All I can tell you is a good reason to have one, as quoted below.

>> > 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.

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.

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

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.

Regards,

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

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

[Greg Kroah-Hartman]

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

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

[Théo Lebrun]

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@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.

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@secretlab.ca>
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

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

[Greg Kroah-Hartman]

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@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.
> 
> 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@secretlab.ca>
> 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

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

[Thomas Petazzoni]

On Thu, 20 Feb 2025 19:26:41 +0100
Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> 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@secretlab.ca>
> Date:   Tue Jun 8 07:48:21 2010 -0600
> 
>     of: Merge of_platform_bus_type with platform_bus_type

I don't really see how an of_platform bus would make sense. OF is not a
bus at all, it's a way of providing HW description to an operating
system.

What would IMO make a lot more sense is mmio_bus, for Memory-Mapped I/O
peripherals. mmio_device can be described through OF, through old-style
board.c, possibly through ACPI, or other means.

But in my eyes, the current platform bus is exactly this: the bus for
MMIO devices. It would have be clearer to name it mmio_bus, and that
would have probably prevented abuses of the platform bus for things
that aren't memory-mapped peripherals.

But clearly any bus that has "OF" in its name is wrong, as OF cannot be
a bus. Keep in mind that OF allows to describe not only MMIO devices,
but also I2C devices, SPI devices, MMC/SDIO devices, PCI devices, USB
devices, etc. OF is a description of the HW, not a bus.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com