Re: [PATCH] HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] HID: surface-hid: teardown race in surface_hid_device_destroy() causes UAF
- [Medium] HID: surface-hid: missing LEDS_CLASS dependency causes link failure
- [Medium] HID: surface-hid: unsynchronized kbd_bl_cur access in surface_kbd_bl_work_fn()
- [Medium] HID: surface-hid: surface_kbd_bl_work_fn() cycles brightness on read error
- [Medium] HID: surface-hid: missing keyboard backlight restoration on resume
--

commit 895dc904dfd7f995cd5b914b2d6be010d9b86f34
Author: Ramón Jesús Miranda <ramonjesusmiranda@gmail.com>

HID: surface-hid: support keyboard backlight on Surface Pro Keyboard (045E:09AE)

This commit adds keyboard backlight LED class support to surface-hid for the
Microsoft Surface Pro Keyboard (045E:09AE), mapping brightness levels 0..3 to
HID feature and output reports 0xd5 and 0xd6 and handling hardware brightness
cycle events on input report 0xd5.

> diff --git a/drivers/hid/surface-hid/surface_hid_core.c b/drivers/hid/surface-hid/surface_hid_core.c
> index 6690c24f28f0c..afee61f9af442 100644
> --- a/drivers/hid/surface-hid/surface_hid_core.c
> +++ b/drivers/hid/surface-hid/surface_hid_core.c
> @@ -184,6 +184,98 @@ static const struct hid_ll_driver surface_hid_ll_driver = {
>  };
>  
>  
> +/* -- Keyboard backlight (Surface Pro Flex / Type Cover 045E:09AE). --------- */
> +
> +static u8 surface_kbd_bl_raw_to_idx(struct surface_hid_device *shid, u8 raw)
> +{
> +	u8 i;
> +
> +	for (i = 0; i < 4; i++) {
> +		if (shid->kbd_bl_levels[i] >= raw)
> +			return i;
> +	}
> +	return 3;
> +}
> +
> +static enum led_brightness surface_kbd_bl_get(struct led_classdev *cdev)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(cdev, struct surface_hid_device, kbd_bl_led);
> +	u8 buf[2] = { 0xd6, 0 };
> +
> +	if (surface_hid_is_hot_removed(shid))
> +		return shid->kbd_bl_cur;
> +
> +	if (shid->ops.get_feature_report(shid, 0xd6, buf, sizeof(buf)) < 0)
> +		return shid->kbd_bl_cur;
> +
> +	shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, buf[1]);
> +	return shid->kbd_bl_cur;
> +}
> +
> +static int surface_kbd_bl_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(cdev, struct surface_hid_device, kbd_bl_led);
> +	u8 idx = clamp_t(u8, value, 0, 3);
> +	u8 buf[2] = { 0xd5, shid->kbd_bl_levels[idx] };
> +	int ret;
> +
> +	if (surface_hid_is_hot_removed(shid))
> +		return -ENODEV;
> +
> +	ret = shid->ops.output_report(shid, 0xd5, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	shid->kbd_bl_cur = idx;
> +	return 0;
> +}
> +
> +static void surface_kbd_bl_work_fn(struct work_struct *work)
> +{
> +	struct surface_hid_device *shid =
> +		container_of(work, struct surface_hid_device, kbd_bl_work);
> +	u8 prev = shid->kbd_bl_cur;
> +	u8 hw_idx = surface_kbd_bl_get(&shid->kbd_bl_led);
> +
> +	if (hw_idx == prev) {
> +		u8 next = (prev + 1) % 4;
> +
> +		surface_kbd_bl_set(&shid->kbd_bl_led, next);
> +	}
> +
> +	led_classdev_notify_brightness_hw_changed(&shid->kbd_bl_led, shid->kbd_bl_cur);
> +}

[Severity: Medium]
Can concurrent access to shid->kbd_bl_cur cause surface_kbd_bl_work_fn() to
advance the brightness twice on a single F5 press?

shid->kbd_bl_cur is read and updated without synchronization across
surface_kbd_bl_get(), surface_kbd_bl_set(), and surface_kbd_bl_work_fn().

If a userspace daemon reads the sysfs brightness attribute (calling
brightness_show() -> led_update_brightness() -> surface_kbd_bl_get()) after
the embedded controller increments the brightness on an F5 press, but before
surface_kbd_bl_work_fn() reads shid->kbd_bl_cur, surface_kbd_bl_get() will
update shid->kbd_bl_cur to the new hardware level first.

When surface_kbd_bl_work_fn() subsequently runs, prev already holds the new
hardware level, so hw_idx == prev evaluates to true and
surface_kbd_bl_work_fn() calls surface_kbd_bl_set() to step the brightness a
second time to (prev + 1) % 4. Concurrent sysfs writes via
set_brightness_delayed() -> surface_kbd_bl_set() also update shid->kbd_bl_cur
without serialization against surface_kbd_bl_work_fn().

[Severity: Medium]
What happens in surface_kbd_bl_work_fn() if reading feature report 0xd6 fails
or the keyboard is hot-removed?

In surface_kbd_bl_get(), both surface_hid_is_hot_removed(shid) and a negative
return from shid->ops.get_feature_report(shid, 0xd6, buf, sizeof(buf)) return
the cached shid->kbd_bl_cur.

Because surface_kbd_bl_work_fn() sets prev = shid->kbd_bl_cur and then
compares hw_idx == prev, any read failure or hot-removal makes hw_idx equal
prev, causing surface_kbd_bl_work_fn() to treat the failed read as an
unchanged hardware brightness, send output report 0xd5 via
surface_kbd_bl_set(), and unconditionally notify userspace via
led_classdev_notify_brightness_hw_changed().

> +
> +static void surface_hid_kbd_bl_init(struct surface_hid_device *shid)
> +{
> +	u8 feat[5] = { 0xd5, 0 };
> +	u8 cur[2] = { 0xd6, 0 };
> +
> +	if (shid->hid->vendor != 0x045e || shid->hid->product != 0x09ae)
> +		return;
> +
> +	if (shid->ops.get_feature_report(shid, 0xd5, feat, sizeof(feat)) < 0 ||
> +	    feat[4] == 0)
> +		return;
> +
> +	memcpy(shid->kbd_bl_levels, &feat[1], 4);
> +	if (shid->ops.get_feature_report(shid, 0xd6, cur, sizeof(cur)) >= 0)
> +		shid->kbd_bl_cur = surface_kbd_bl_raw_to_idx(shid, cur[1]);
> +
> +	INIT_WORK(&shid->kbd_bl_work, surface_kbd_bl_work_fn);
> +	shid->kbd_bl_led.name = "surface::kbd_backlight";
> +	shid->kbd_bl_led.max_brightness = 3;
> +	shid->kbd_bl_led.flags = LED_BRIGHT_HW_CHANGED;
> +	shid->kbd_bl_led.brightness_set_blocking = surface_kbd_bl_set;
> +	shid->kbd_bl_led.brightness_get = surface_kbd_bl_get;
> +	shid->kbd_bl_led.brightness = shid->kbd_bl_cur;

[Severity: Medium]
The commit message states that the driver will "restore the saved brightness
level on resume", and also mentions hooking raw_event:

    Also hook raw_event to read back Feature Report 0xd6 and notify userspace
    via led_classdev_notify_brightness_hw_changed() when Input Report 0xd5
    arrives from the F5 key, and restore the saved brightness level on resume.

However, shid->kbd_bl_led.flags only sets LED_BRIGHT_HW_CHANGED without
LED_CORE_SUSPENDRESUME, and neither surface_hid_resume() nor
surface_hid_restore() in drivers/hid/surface-hid/surface_hid_core.c writes
shid->kbd_bl_cur back to the device.

If the keyboard resets its backlight to 0 across suspend/resume while
shid->kbd_bl_cur stays at its pre-suspend index (for example 1), won't the
backlight remain off after resume and cause the next F5 press to step the
hardware from 0 to 1, match hw_idx == prev in surface_kbd_bl_work_fn(), and
jump to 2? Also, should device-specific HID report handling for 045E:09AE live
in a HID device driver rather than the low-level transport driver?

> +
> +	if (led_classdev_register(shid->dev, &shid->kbd_bl_led) == 0)
> +		shid->has_kbd_bl = true;
> +}

[Severity: Medium]
Will this fail to link when CONFIG_SURFACE_HID_CORE is enabled without
CONFIG_LEDS_CLASS (or when CONFIG_SURFACE_HID_CORE=y and
CONFIG_LEDS_CLASS=m)?

surface_hid_kbd_bl_init() calls led_classdev_register() (which expands to
led_classdev_register_ext()), surface_hid_device_destroy() calls
led_classdev_unregister(), and surface_kbd_bl_work_fn() calls
led_classdev_notify_brightness_hw_changed().

In include/linux/leds.h, led_classdev_register_ext() and
led_classdev_unregister() do not have inline stubs when CONFIG_LEDS_CLASS is
disabled, and SURFACE_HID_CORE (as well as SURFACE_HID and SURFACE_KBD) in
drivers/hid/surface-hid/Kconfig has no dependency on or select of LEDS_CLASS
or NEW_LEDS.

[ ... ]

> @@ -218,15 +310,23 @@ int surface_hid_device_add(struct surface_hid_device *shid)
>  	shid->hid->ll_driver = &surface_hid_ll_driver;
>  
>  	status = hid_add_device(shid->hid);
> -	if (status)
> +	if (status) {
>  		hid_destroy_device(shid->hid);
> +		return status;
> +	}
>  
> -	return status;
> +	surface_hid_kbd_bl_init(shid);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(surface_hid_device_add);
>  
>  void surface_hid_device_destroy(struct surface_hid_device *shid)
>  {
> +	if (shid->has_kbd_bl) {
> +		cancel_work_sync(&shid->kbd_bl_work);
> +		led_classdev_unregister(&shid->kbd_bl_led);
> +		shid->has_kbd_bl = false;
> +	}
>  	hid_destroy_device(shid->hid);
>  }

[Severity: Critical]
Can ssam_hid_event_fn() re-schedule shid->kbd_bl_work after
cancel_work_sync() in surface_hid_device_destroy(), causing a use-after-free
of shid and shid->kbd_bl_led?

In surface_hid_device_destroy(), cancel_work_sync(&shid->kbd_bl_work) is
called while shid->has_kbd_bl is still true and before
hid_destroy_device(shid->hid) calls surface_hid_stop() ->
__ssam_notifier_unregister() to stop ssam_hid_event_fn():

drivers/hid/surface-hid/surface_hid.c:ssam_hid_event_fn() {
    ...
	if (shid->has_kbd_bl && event->length >= 2 &&
	    event->data[0] == 0xd5 && (event->data[1] & 0x01))
		schedule_work(&shid->kbd_bl_work);
    ...
}

While led_classdev_unregister(&shid->kbd_bl_led) is sleeping (or before
hid_destroy_device() unregisters the SSAM event notifier), an incoming 0xd5
event in ssam_hid_event_fn() can still observe shid->has_kbd_bl == true (or
have already passed that check before shid->has_kbd_bl = false) and call
schedule_work(&shid->kbd_bl_work).

Once re-queued after cancel_work_sync(), surface_kbd_bl_work_fn() can run
after led_classdev_unregister() has destroyed shid->kbd_bl_led and after
surface_hid_remove() returns and devres frees shid.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023173213.6219-1-ramonjesusmiranda@gmail.com?part=1