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

[Lee Jones <lee@kernel.org>]

On Thu, 09 Jul 2026, sashiko-bot@kernel.org wrote:

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

I need to see that you at least considered this.

> 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

-- 
Lee Jones