Re: [PATCH v5 07/12] leds: trigger: Enforce strict checks in led_trigger_is_hw_controlled()

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

Pre-existing issues:
- [High] Sysfs attributes are exposed to user-space before the `trigger_lock` rwsem is initialized, causing uninitialized lock access.
--

--- Patch [7]: [PATCH v5 07/12] leds: trigger: Enforce strict checks in led_trigger_is_hw_controlled() ---
commit ebab4e455acab29c44baf2a4a586beae42254bc5
Author: Rong Zhang <[email protected]>

leds: trigger: Enforce strict checks in led_trigger_is_hw_controlled()

With all existing triggers adopting the new interface, strict checks
could be enforced to make the semantics of hardware control triggers
clearer.

> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index de17a8bbb4d48..cb49a02a8b3cf 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -42,9 +42,16 @@ static bool __led_trigger_is_hw_controlled(struct led_classdev *led_cdev)
>  	if (!led_cdev->trigger)
>  		return false;
>  
> +	if (!led_cdev->hw_control_trigger ||
> +	    strcmp(led_cdev->hw_control_trigger, led_cdev->trigger->name))
> +		return false;

[Severity: High]
This is a pre-existing issue, but since this patch modifies the hardware
control paths, is it possible to access an uninitialized rw_semaphore here?

In drivers/leds/led-class.c, the device and sysfs attributes are created
and exposed to user-space before the trigger_lock rwsem is initialized:

led_classdev_register_ext() {
        ...
        led_cdev->dev = device_create_with_groups(&leds_class, parent, 0,
                                                  led_cdev, led_cdev->groups,
                                                  "%s", final_name);
        ...
        init_rwsem(&led_cdev->trigger_lock);
        ...
}

If user-space processes the uevent quickly enough and reads the brightness
attribute, brightness_show() will call led_trigger_is_hw_controlled() before
acquiring the led_access mutex:

brightness_show() {
        if (led_trigger_is_hw_controlled(led_cdev))
                return -ENODATA;

        mutex_lock(&led_cdev->led_access);
        ...
}

Since led_trigger_is_hw_controlled() eventually accesses trigger_lock, will
it attempt to acquire the uninitialized semaphore and cause a crash or
lockdep warning?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7