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

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 <[email protected]>
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/[email protected]?part=1