Re: [PATCH v5 10/12] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight

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

Pre-existing issues:
- [High] The conversion of `last_hw_brightness` to an `atomic_t` fails to prevent a TOCTOU (Time-Of-Check to Time-Of-Use) race condition between ACPI notifications and sysfs brightness writes, violating core kernel locking rules.
--

--- Patch [10]: [PATCH v5 10/12] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight ---
commit b73007a6498db545468f511c09a496e09dfcac31
Author: Rong Zhang <[email protected]>

platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight

This commit decouples hardware brightness from LED classdev brightness for
the keyboard backlight. It also converts last_hw_brightness into an atomic_t
to help prevent potential race conditions.

> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
[ ... ]
> @@ -1637,32 +1676,37 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
>  	return ideapad_kbd_bl_brightness_get(priv);
>  }
>  
> -static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> +static int ideapad_kbd_bl_hw_brightness_set(struct ideapad_private *priv, int hw_brightness)
>  {
[ ... ]
>  	if (err)
>  		return err;
>  
> -	priv->kbd_bl.last_brightness = brightness;
> +	atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);
>  
>  	return 0;
>  }
[ ... ]
> @@ -1673,26 +1717,29 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
>  
>  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  {
> -	int brightness;
> +	int hw_brightness, brightness, last_hw_brightness;
>  
>  	if (!priv->kbd_bl.initialized)
>  		return;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
>  		return;
>  
> -	if (brightness == priv->kbd_bl.last_brightness)
> -		return;
> +	brightness = ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
> +	if (brightness < 0)
> +		return; /* Reject insane values early. */
>  
> -	priv->kbd_bl.last_brightness = brightness;
> +	last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);

[Severity: High]
This is a pre-existing issue, but does converting last_hw_brightness to an
atomic_t fully prevent a time-of-check to time-of-use race condition here?

Since the hardware read and the atomic_xchg() are separate steps, could
the following sequence occur?

CPU1 in ideapad_kbd_bl_notify():
    hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);

CPU2 in ideapad_kbd_bl_hw_brightness_set() via concurrent sysfs write:
    exec_kblc(priv->adev->handle, value);
    atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);

CPU1 resumes in ideapad_kbd_bl_notify():
    last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);

In this scenario, wouldn't CPU1 overwrite the software cache with a stale
hardware state, causing it to become permanently desynchronized from the
actual hardware?

Although follow-up patches in this series introduce a notif_mutex, does
the sysfs write path ever acquire this mutex to serialize these updates
properly?

> +	if (hw_brightness == last_hw_brightness)
> +		return;
>  
>  	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
>  }

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