[PATCH] leds: class: disable sysfs before unregistering LED devices

[PATCH] leds: class: disable sysfs before unregistering LED devices

[David Lee]

Closing a uleds device removes the LED class device and then frees the
struct uleds_device that contains it. The LED class device is also exposed
through sysfs, where attributes such as trigger, brightness, delay_on, and
delay_off can remain open across unregister.

That creates a lifetime race. led_classdev_unregister() tears down the LED
trigger, software blink timer, brightness state, and trigger-specific sysfs
groups, but it does not first block LED sysfs writers. An already-open sysfs
attribute can therefore enter the LED trigger or timer paths while unregister
is tearing down the same embedded struct led_classdev.

For uleds this can become a use-after-free: after unregister returns,
uleds_release() frees the containing struct uleds_device, while stale sysfs
or timer paths may still access led_classdev fields embedded in that freed
object. KASAN reports use-after-free reads and writes in __run_timers(),
led_timer_function(), and uleds_brightness_set().

Fix this by entering the existing LED sysfs exclusion protocol at the start
of unregister. Take led_cdev->led_access, set LED_SYSFS_DISABLE, perform the
trigger, timer, brightness, and trigger-sysfs teardown, then release the
mutex. A racing sysfs writer either completes before unregister starts or
sees the disabled state and returns -EBUSY instead of touching an object that
the driver may free.

Fixes: e381322b0190 ("leds: Introduce userspace LED class driver")
Cc: stable@vger.kernel.org
Signed-off-by: David Lee <david.lee@trailofbits.com>
Assisted-by: Codex:gpt-5.5
---
Trail of Bits has a minimal PoC that triggers this crash on a custom
kernel build, which can be shared further if needed.

drivers/leds/led-class.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index a51b0ed53886..a697d6740011 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -616,6 +616,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	if (IS_ERR_OR_NULL(led_cdev->dev))
 		return;
 
+	mutex_lock(&led_cdev->led_access);
+	led_sysfs_disable(led_cdev);
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	down_write(&led_cdev->trigger_lock);
 	if (led_cdev->trigger)
@@ -636,6 +639,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
 		led_remove_brightness_hw_changed(led_cdev);
 
+	mutex_unlock(&led_cdev->led_access);
+
 	device_unregister(led_cdev->dev);
 
 	down_write(&leds_list_lock);

Re: [PATCH] leds: class: disable sysfs before unregistering LED devices

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Reusing `LED_SYSFS_DISABLE` for unregistration teardown conflicts with V4L2 flash, which can clear the flag during unregistration and bypass the patch's protection.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing the `led_sysfs_is_disabled()` check in `multi_intensity_store()` bypasses the patch's newly introduced teardown protection, allowing the use-after-free to still occur for multicolor LEDs.
- [High] This problem wasn't introduced by this patch, but concurrent `led_trigger_register` can attach a trigger to an unregistering LED device whose embedded `dev` structure has already been freed, leading to a Use-After-Free.
- [High] This problem wasn't introduced by this patch, but LED triggers have a Use-After-Free race condition where their sysfs attributes can execute concurrently with or after trigger deactivation.
--

--- Patch [1]: [PATCH] leds: class: disable sysfs before unregistering LED devices ---
commit a26d864d321effb464ac1db24d6210a72a4d07d3
Author: David Lee <david.lee@trailofbits.com>
Subject: leds: class: disable sysfs before unregistering LED devices

This commit attempts to fix a lifetime race during LED unregistration
by locking led_access and setting LED_SYSFS_DISABLE early in
led_classdev_unregister(). This blocks sysfs writers from accessing the
embedded struct led_classdev while it is being torn down.

> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 6a8ea94834fa3..a6c2468c0da1e 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -435,6 +435,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>  	if (IS_ERR_OR_NULL(led_cdev->dev))
>  		return;
>  
> +	mutex_lock(&led_cdev->led_access);
> +	led_sysfs_disable(led_cdev);
> +

[Severity: High]
Does overloading LED_SYSFS_DISABLE for unregistration conflict with V4L2
flash devices?

If a user closes the V4L2 subdevice file concurrently during driver removal,
v4l2_flash_close() actively clears this flag:

drivers/media/v4l2-core/v4l2-flash-led-class.c:v4l2_flash_close() {
    ...
    led_sysfs_enable(led_cdev);
    mutex_unlock(&led_cdev->led_access);
}

Could this allow a blocked sysfs writer to acquire led_access after
unregister unlocks it, incorrectly see sysfs as enabled, and schedule work
on an unregistering or freed device?

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a
led_sysfs_is_disabled() check in multi_intensity_store() bypass this newly
added teardown protection?

If a concurrent sysfs write to multi_intensity blocks on led_access, and
acquires it after unregister unlocks it:

drivers/leds/led-class-multicolor.c:multi_intensity_store() {
    ...
    mutex_lock(&led_cdev->led_access);

    for (i = 0; i < mcled_cdev->num_colors; i++) {
    ...
}

Because it immediately proceeds without checking if sysfs is disabled,
could it call led_set_brightness() and schedule new work after unregister
has already flushed work queues, leading to a use-after-free?

>  #ifdef CONFIG_LEDS_TRIGGERS
>  	down_write(&led_cdev->trigger_lock);
>  	if (led_cdev->trigger)
>  		led_trigger_set(led_cdev, NULL);
>  	up_write(&led_cdev->trigger_lock);
>  #endif

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
race condition when clearing the trigger here?

In led_trigger_set(), the deactivate callback is executed before the sysfs
groups are removed:

drivers/leds/led-triggers.c:led_trigger_set() {
    ...
    if (led_cdev->trigger->deactivate)
        led_cdev->trigger->deactivate(led_cdev);
    device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
    ...
}

Since sysfs operations for triggers (like transient_activate_store()) do not
acquire led_access, could a sysfs write that is preempted resume after
deactivate() frees the private data, modifying freed memory before
device_remove_groups() can prevent it?

[ ... ]

> @@ -455,6 +458,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>  	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
>  		led_remove_brightness_hw_changed(led_cdev);
>  
> +	mutex_unlock(&led_cdev->led_access);
> +
>  	device_unregister(led_cdev->dev);
>  
>  	down_write(&leds_list_lock);
>  	list_del(&led_cdev->node);
>  	up_write(&leds_list_lock);

[Severity: High]
This isn't a bug introduced by this patch, but does unregistering the
device before removing the LED from the global leds_list leave a dangling
pointer?

If device_unregister() drops the final reference and frees the device
structure, could a concurrent led_trigger_register() iterating over
leds_list match this unregistering LED?

drivers/leds/led-triggers.c:led_trigger_set() {
    ...
    ret = device_add_groups(led_cdev->dev, trig->groups);
    if (ret) {
    ...
}

Could this lead to led_trigger_set() dereferencing the already-freed
led_cdev->dev when attempting to attach the trigger's sysfs groups?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709101210.104975-1-david.lee@trailofbits.com?part=1