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

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

[Andreas Claesson via B4 Relay]

From: Andreas Claesson <andreas@aclaesson.com>

Commit eb51c9f8cb4f ("HID: corsair: cancel worker before unregistering
LED to fix use-after-free") moved cancel_work_sync() in front of
led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions().

led_classdev_unregister() turns the LED off unless
LED_RETAIN_AT_SHUTDOWN is set. That calls k90_brightness_set(), which
unconditionally does schedule_work() on the same work item. The work is
therefore queued again after it was cancelled, and the structure that
embeds it is freed right afterwards:

  k90_cleanup_macro_functions()
    cancel_work_sync(&k90->record_led.work)
    led_classdev_unregister(&k90->record_led.cdev)
      led_set_brightness(LED_OFF)
        k90_brightness_set()
          schedule_work(&led->work)        <-- re-queued
    kfree(k90)                             <-- frees the queued work_struct

When the work later runs, the workqueue dereferences the freed
work_struct and k90_record_led_work() reads led->removed from freed
memory. A USB device that enumerates as a K90 and is then unplugged
triggers this reliably. With CONFIG_DEBUG_OBJECTS_WORK it shows up as:

  ODEBUG: free active (active state 0) object: ffff88803b8772c8 object type: work_struct hint: k90_record_led_work+0x0/0x1e0
  WARNING: lib/debugobjects.c:632 at debug_print_object+0xec/0x230
  Workqueue: usb_hub_wq hub_event
  Call Trace:
   debug_check_no_obj_freed+0x2e3/0x450 lib/debugobjects.c:1201
   kfree+0x13e/0x650 mm/slub.c:6923
   k90_cleanup_macro_functions drivers/hid/hid-corsair.c:547 [inline]
   corsair_remove+0xfd/0x260 drivers/hid/hid-corsair.c:602
   hid_device_remove+0xc5/0x1c0 drivers/hid/hid-core.c:2897
   ...
   usbhid_disconnect+0x9f/0xc0 drivers/hid/usbhid/hid-core.c:1486

(with ODEBUG enabled, work_fixup_free() cancels the work and hides the
use-after-free. Without it, the freed work item is executed.)

Keep the existing cancel_work_sync() before unregistering, which waits
for a worker that may already be past the 'removed' check, and cancel
the work once more after led_classdev_unregister() so the work queued
by the LED-off request is gone before the memory is freed. A re-queued
work that manages to run before the second cancel still sees
'removed == true' while the structure is alive and returns early.

Fixes: eb51c9f8cb4f ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
Cc: stable@vger.kernel.org
Signed-off-by: Andreas Claesson <andreas@aclaesson.com>
---
Found with syzkaller (USB emulation, KASAN + ODEBUG) on mainline
f66469d9905b. Both the record LED and the backlight paths reproduce
(hint: k90_record_led_work / k90_backlight_work); a minimised
3-call syzkaller reproducer is available on request. With this patch,
200 probe/removal cycles per path gave no ODEBUG or KASAN reports,
against ODEBUG on roughly half of removals without it.
---
 drivers/hid/hid-corsair.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..75c862f5f783 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -526,6 +526,8 @@ static void k90_cleanup_backlight(struct hid_device *dev)
 		drvdata->backlight->removed = true;
 		cancel_work_sync(&drvdata->backlight->work);
 		led_classdev_unregister(&drvdata->backlight->cdev);
+		/* unregister turns the LED off, which re-queues the work */
+		cancel_work_sync(&drvdata->backlight->work);
 		kfree(drvdata->backlight->cdev.name);
 		kfree(drvdata->backlight);
 	}
@@ -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);

---
base-commit: f66469d9905b39d2c260a15d7d6db450f4db67c1
change-id: 20261109-hid-corsair-k90-uaf-0c711197ce3e

Best regards,
--  
Andreas Claesson <andreas@aclaesson.com>

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

[sashiko-bot]

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