Re: [PATCH] HID: corsair: cancel LED work again after unregistering the LED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] HID: corsair: use-after-free in k90_init_macro_functions()
  error path [1]
--

commit 3a201ad222c1beccee0e1120afae987d661371db
Author: Andreas Claesson <andreas@aclaesson.com>

HID: corsair: cancel LED work again after unregistering the LED

This commit adds a second cancel_work_sync() call after
led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions() to cancel work re-queued when
led_classdev_unregister() turns the LED off.

> diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
> index 278c6efb565df..75c862f5f783f 100644
> --- a/drivers/hid/hid-corsair.c
> +++ b/drivers/hid/hid-corsair.c

[ ... ]

> @@ -542,6 +544,8 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
>  		k90->record_led.removed = true;
>  		cancel_work_sync(&k90->record_led.work);
>  		led_classdev_unregister(&k90->record_led.cdev);
> +		/* unregister turns the LED off, which re-queues the work */
> +		cancel_work_sync(&k90->record_led.work);
>  		kfree(k90->record_led.cdev.name);
>  
>  		kfree(k90);

[Severity: Medium]
This problem wasn't introduced by this patch, but can the fail_sysfs error
path in k90_init_macro_functions() still cause a use-after-free on
led->cdev.dev [1]?

drivers/hid/hid-corsair.c:k90_init_macro_functions() {
    ...
fail_sysfs:
	k90->record_led.removed = true;
	led_classdev_unregister(&k90->record_led.cdev);
	cancel_work_sync(&k90->record_led.work);
    ...
}

When sysfs_create_group() fails after led_classdev_register() has exposed
the record LED, fail_sysfs calls led_classdev_unregister() without calling
cancel_work_sync() first:

CPU0                              CPU1
k90_init_macro_functions()
  led_classdev_register(...)
                                  k90_brightness_set()
                                    schedule_work(&led->work)
                                  k90_record_led_work()
                                    if (led->removed)
  sysfs_create_group(...)
  k90->record_led.removed = true;
  led_classdev_unregister(...)
                                    led->cdev.dev->parent
                                      <-- use-after-free
  cancel_work_sync(...)

Calling cancel_work_sync(&k90->record_led.work) before
led_classdev_unregister() in fail_sysfs as well would match
k90_cleanup_macro_functions().

[1] https://sashiko.dev/#/bug/linux-2f786541-d064-428e-a1ab-d203d4112002

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-hid-corsair-k90-uaf-v1-1-0621c2c5c451@aclaesson.com?part=1