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

[Rong Zhang <i@rong.moe>]

Hi all,

On Fri, 2026-08-21 at 17:27 +0000, sashiko-bot@kernel.org wrote:
> 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.

Valid concern. This bug was introduced in commit b819dc7d8fb2 ("leds:
core: Report ENODATA for brightness of hardware controlled LED"), as the
check is not protected by led_access.

Since the series touches what was introduced by the said commit, I will
prepend a fix commit in the next revision by protecting it with
led_access.

Thanks,
Rong

> --
> 
> --- Patch [7]: [PATCH v5 07/12] leds: trigger: Enforce strict checks in led_trigger_is_hw_controlled() ---
> commit ebab4e455acab29c44baf2a4a586beae42254bc5
> Author: Rong Zhang <i@rong.moe>
> 
> 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?
> 
> [ ... ]