Re: [PATCH 1/4] HID: lg-g15: add LCD contrast LED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: lg-g15: out-of-bounds read in lg_g15_contrast_set() on
  unclamped brightness
- [Low] HID: lg-g15: unlocked read of contrast brightness in
  lg_g15_resume()
--

commit 8cb726418b3437c4e73ef257cf7f4986794e4a67
Author: Torstein Eide <torsteine+linux@gmail.com>

HID: lg-g15: add LCD contrast LED

This commit adds an LED class device to control the LCD contrast on
Logitech G15 and G15 v2 keyboards and restores the configured contrast on
resume.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 02ef3e2094b4c..50abe6c58b10b 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c

[ ... ]

> @@ -365,6 +375,57 @@ static int lg_g15_led_set(struct led_classdev *led_cdev,
>  	return ret;
>  }
>  
> +/*
> + * LCD contrast, report 2 field 32: {2, 32, 129, value}, value in
> + * {18, 22, 26} for low/medium/high. Distinct sub-format from the
> + * brightness/macro fields (value sits in byte 3, not byte 2, and byte 2 is
> + * a fixed marker rather than the field selector) and has no GET_REPORT
> + * readback, so brightness_get just returns the last value we set.
> + */
> +static int lg_g15_contrast_set(struct led_classdev *led_cdev,
> +			       enum led_brightness brightness)
> +{
> +	struct lg_g15_led *g15_led =
> +		container_of(led_cdev, struct lg_g15_led, cdev);
> +	struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> +	static const u8 contrast_values[] = { 18, 22, 26 };
> +	int ret;
> +
> +	if (led_cdev->flags & LED_UNREGISTERING)
> +		return 0;
> +
> +	mutex_lock(&g15->mutex);
> +
> +	g15->transfer_buf[0] = LG_G15_FEATURE_REPORT;
> +	g15->transfer_buf[1] = 32;
> +	g15->transfer_buf[2] = 129;
> +	g15->transfer_buf[3] = contrast_values[brightness];

[Severity: Medium]
Can this read past the end of contrast_values[] when brightness > 2?

When LED_SET_BRIGHTNESS or LED_BLINK_DISABLE is already set in
led_cdev->work_flags (for example, during back-to-back sysfs writes to
brightness), led_set_brightness() stores the unclamped brightness value in
led_cdev->delayed_set_value before led_set_brightness_nosleep() clamps it
to max_brightness:

drivers/leds/led-core.c:led_set_brightness() {
    ...
		if (test_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags) ||
		    test_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
			led_cdev->delayed_set_value = brightness;
			set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
			queue_work(led_cdev->wq, &led_cdev->set_brightness_work);
			return;
		}
    ...
}

set_brightness_delayed() then passes delayed_set_value directly to
brightness_set_blocking() without clamping, so a value >= 3 indexes out of
bounds in contrast_values[], sends that byte in the feature report, and
caches the out-of-bounds index in g15_led->brightness.

[ ... ]

> @@ -1389,12 +1465,33 @@ static void lg_g15_remove(struct hid_device *hdev)
>  	hid_hw_stop(hdev);
>  }
>  
> +/*
> + * The LCD does not retain its contrast across a USB reset or power loss;
> + * re-apply the last value userspace set, if any.
> + */
> +static int lg_g15_resume(struct hid_device *hdev)
> +{
> +	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +	struct lg_g15_led *contrast;
> +
> +	if (!g15)
> +		return 0;
> +
> +	contrast = &g15->leds[LG_G15_LCD_CONTRAST];
> +	if (contrast->cdev.dev && g15->lcd_contrast_set)
> +		lg_g15_contrast_set(&contrast->cdev, contrast->brightness);

[Severity: Low]
Can reading contrast->brightness without g15->mutex overwrite a concurrent
contrast update?

CPU0                              CPU1 (kworker)
lg_g15_resume()
  val = contrast->brightness
                                  lg_g15_contrast_set(..., new_val)
                                    mutex_lock(&g15->mutex)
                                    hid_hw_raw_request(...)
                                    contrast->brightness = new_val
                                    mutex_unlock(&g15->mutex)
  lg_g15_contrast_set(..., val)
    mutex_lock(&g15->mutex)
    hid_hw_raw_request(...)
    contrast->brightness = val    <-- overwrites new_val
    mutex_unlock(&g15->mutex)

Because lg_g15_resume() reads contrast->brightness and
g15->lcd_contrast_set before lg_g15_contrast_set() acquires g15->mutex, a
concurrent update on leds_wq can complete in between and then be clobbered
by the stale value on both the hardware and contrast->brightness.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094426.1432465-1-torsteine+linux@gmail.com?part=1