[PATCH] HID: corsair: fix use-after-free by reordering remove sequence

[PATCH] HID: corsair: fix use-after-free by reordering remove sequence

[Chen Changcheng]

On device removal corsair_remove() currently does:
    k90_cleanup_macro_functions()     ─┐
        kfree(k90)                     │  k90 freed, drvdata->k90
                                       │  is now a dangling pointer
    k90_cleanup_backlight()            │
    hid_hw_stop(dev)                  ─┘  HID I/O finally stopped
The problem is that kfree happens before hid_hw_stop.  Between them,
corsair_event() can still be invoked on another CPU:

    CPU 0 (remove path)                  CPU 1 (USB interrupt)
    ──────────────────                   ────────────────────
    kfree(k90)
                                          corsair_event()
    ↓                                      if (!drvdata->k90)
    ↓                                        return 0;   ← non-NULL!
    ↓                                      drvdata->k90->record_led
    ↓                                        .brightness = x;
    ↓                                      ^^^^^^^^^^^^^^^^^^^^^^^^
    ↓                                      UAF write into freed slab
    hid_hw_stop(dev)
      hid_disconnect()
        clear claimed flags
      usbhid_stop()
        kill URBs

The NULL check in corsair_event() is ineffective because
k90_cleanup_macro_functions() never clears drvdata->k90 after kfree.

Fix by reordering so hid_hw_stop() runs first, eliminating the window:
    hid_hw_stop(dev)                  ─┐  HID I/O stopped first,
    k90_cleanup_macro_functions()      │  no more events can arrive
    k90_cleanup_backlight()           ─┘

Set drvdata->k90 = NULL after kfree() as a defensive measure, matching
the existing pattern in k90_init_macro_functions()'s error path.

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 21cd8b12a757..ac12877a6db7 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -545,6 +545,7 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
+		drvdata->k90 = NULL;
 	}
 }
 
@@ -596,10 +597,10 @@ static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id)
 
 static void corsair_remove(struct hid_device *dev)
 {
+	hid_hw_stop(dev);
+
 	k90_cleanup_macro_functions(dev);
 	k90_cleanup_backlight(dev);
-
-	hid_hw_stop(dev);
 }
 
 static int corsair_event(struct hid_device *dev, struct hid_field *field,
-- 
2.25.1

[PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal

[Chen Changcheng]

This series fixes two use-after-free bugs in the Corsair HID driver
found during code review.  The previous single-patch submission only
addressed the first issue; the second was identified by Sashiko AI
review.

v2:
  - Split into 2 patches: event vs remove UAF, worker vs cleanup UAF
  - Rewrote commit messages in plain ASCII
  - Added drvdata->k90 = NULL as defensive measure (patch 1)
  - Fixed LED worker ordering in both cleanup functions (patch 2)

Chen Changcheng (2):
  HID: corsair: fix use-after-free by reordering remove sequence
  HID: corsair: cancel worker before unregistering LED to fix
    use-after-free

 drivers/hid/hid-corsair.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence

[Chen Changcheng]

The corsair_remove() function currently frees the k90 driver data before
calling hid_hw_stop().  Since hid_hw_stop() stops HID I/O, the event
callback corsair_event() can still be invoked between the kfree() and
hid_hw_stop(), and will dereference the freed drvdata->k90 pointer to
write record_led.brightness.

Reorder the remove sequence so that hid_hw_stop() is called first.
Once hid_hw_stop() completes, the HID device is disconnected and no
URBs are active, so corsair_event() cannot fire anymore.  The driver
data is freed only afterwards.

Additionally, set drvdata->k90 to NULL after kfree() as a defensive
measure, matching the existing pattern in the error path of
k90_init_macro_functions().

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 21cd8b12a757..ac12877a6db7 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -545,6 +545,7 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
+		drvdata->k90 = NULL;
 	}
 }
 
@@ -596,10 +597,10 @@ static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id)
 
 static void corsair_remove(struct hid_device *dev)
 {
+	hid_hw_stop(dev);
+
 	k90_cleanup_macro_functions(dev);
 	k90_cleanup_backlight(dev);
-
-	hid_hw_stop(dev);
 }
 
 static int corsair_event(struct hid_device *dev, struct hid_field *field,
-- 
2.25.1

[PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free

[Chen Changcheng]

The cleanup functions k90_cleanup_backlight() and
k90_cleanup_macro_functions() call led_classdev_unregister() before
cancel_work_sync():

    led_classdev_unregister()     <-- may free led->cdev.dev
    cancel_work_sync()            <-- wait for worker

If the LED worker (k90_backlight_work / k90_record_led_work) is
already running on another CPU, the following race can occur:

    CPU 1 (worker)               CPU 2 (remove)
    ---------------------         --------------------
    if (led->removed) -> false
    (passed the guard, about to read led->cdev.dev)
    * preempted
                                   removed = true
                                   led_classdev_unregister()
                                     -> led->cdev.dev freed
                                   cancel_work_sync()
                                     -> waits for worker
    * resumes
    dev = led->cdev.dev->parent   <-- UAF!

Fix by swapping the order so that the worker is cancelled first:

    cancel_work_sync()            <-- wait for worker first
    led_classdev_unregister()     <-- then safe to unregister

The removed flag is set before cancel_work_sync() so that if
led_classdev_unregister() internally triggers another brightness
update (which re-schedules the work), the worker will see the flag
and return immediately.

Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
---
 drivers/hid/hid-corsair.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index ac12877a6db7..278c6efb565d 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -524,8 +524,8 @@ static void k90_cleanup_backlight(struct hid_device *dev)
 
 	if (drvdata->backlight) {
 		drvdata->backlight->removed = true;
-		led_classdev_unregister(&drvdata->backlight->cdev);
 		cancel_work_sync(&drvdata->backlight->work);
+		led_classdev_unregister(&drvdata->backlight->cdev);
 		kfree(drvdata->backlight->cdev.name);
 		kfree(drvdata->backlight);
 	}
@@ -540,8 +540,8 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
 		sysfs_remove_group(&dev->dev.kobj, &k90_attr_group);
 
 		k90->record_led.removed = true;
-		led_classdev_unregister(&k90->record_led.cdev);
 		cancel_work_sync(&k90->record_led.work);
+		led_classdev_unregister(&k90->record_led.cdev);
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
-- 
2.25.1

Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal

[Jeffin Philip]

On Mon, 27 Jul 2026 09:35:00 +0800, Chen Changcheng wrote:

>The cleanup functions k90_cleanup_backlight() and
>k90_cleanup_macro_functions() call led_classdev_unregister() before
>cancel_work_sync():
>
>    led_classdev_unregister()     <-- may free led->cdev.dev
>    cancel_work_sync()            <-- wait for worker
>
>If the LED worker (k90_backlight_work / k90_record_led_work) is
>already running on another CPU, the following race can occur:
>
>    CPU 1 (worker)               CPU 2 (remove)
>    ---------------------         --------------------
>    if (led->removed) -> false
>    (passed the guard, about to read led->cdev.dev)
>    * preempted
>                                   removed = true
>                                   led_classdev_unregister()
>                                     -> led->cdev.dev freed
>                                   cancel_work_sync()
>                                     -> waits for worker
>    * resumes
>    dev = led->cdev.dev->parent   <-- UAF!
>
>Fix by swapping the order so that the worker is cancelled first:
>
>    cancel_work_sync()            <-- wait for worker first
>    led_classdev_unregister()     <-- then safe to unregister
>
>The removed flag is set before cancel_work_sync() so that if
>led_classdev_unregister() internally triggers another brightness
>update (which re-schedules the work), the worker will see the flag
>and return immediately.

The premise looks good, but after re-scheduling the work(possibly),
what happens when we call kfree in the cleanup function, that leads
to a ODEBUG warning as our work might be active when we try to kfree.
How can this solve the ODEBUG warning?

Reproduced here: https://syzkaller.appspot.com/bug?extid=0a031a76585d1c7e737d

Thanks,
Jeffin.

Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal

[Chen Changcheng]

Hi Jeffin,

Thanks for the review and for the pointer to the syzbot report.

You are right: led_classdev_unregister() internally calls
led_set_brightness(LED_OFF), which reaches k90_brightness_set() and
re-schedules the worker after cancel_work_sync() has returned, so the
work_struct can still be queued when kfree() is called.  This is the
issue syzbot reported (extid=0a031a76585d1c7e737d).

The follow-up patch below makes k90_brightness_set() a no-op once
removed is set, so the LED_OFF update from led_classdev_unregister()
cannot re-schedule the worker after it has been cancelled.  It also
applies the same cancel-before-unregister ordering to the probe error
path in k90_init_macro_functions() for consistency.

Thanks,
Chen Changcheng

[PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled

[Chen Changcheng]

From: Chen Changcheng <chenchangcheng@kylinos.cn>

Commit eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering
LED to fix use-after-free") moved cancel_work_sync() ahead of
led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions().  led_classdev_unregister() internally
calls led_set_brightness(LED_OFF), which reaches the driver's
k90_brightness_set() callback.  Since that callback schedules the worker
unconditionally, the worker was re-queued after cancel_work_sync() had
drained it, and the subsequent kfree() freed a still-active work_struct:

    ODEBUG: free active (active state 0) object type: work_struct
            hint: k90_record_led_work

The removed flag check inside the worker itself only stops it from
dereferencing freed memory once it runs; it cannot prevent the re-queue.

Fix this by making k90_brightness_set() a no-op once removed is set, so
the LED_OFF update issued from led_classdev_unregister() can no longer
re-schedule the worker after it has been cancelled.  Also apply the
cancel-before-unregister ordering to the probe error path
(k90_init_macro_functions() fail_sysfs) for consistency.

Fixes: eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
Reported-by: syzbot+0a031a76585d1c7e737d@syzkaller.appspotmail.com
Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..73b3c1ff78c6 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -194,6 +194,9 @@ static void k90_brightness_set(struct led_classdev *led_cdev,
 {
 	struct k90_led *led = container_of(led_cdev, struct k90_led, cdev);
 
+	if (led->removed)
+		return;
+
 	led->brightness = brightness;
 	schedule_work(&led->work);
 }
@@ -507,8 +510,8 @@ static int k90_init_macro_functions(struct hid_device *dev)
 
 fail_sysfs:
 	k90->record_led.removed = true;
-	led_classdev_unregister(&k90->record_led.cdev);
 	cancel_work_sync(&k90->record_led.work);
+	led_classdev_unregister(&k90->record_led.cdev);
 fail_record_led:
 	kfree(k90->record_led.cdev.name);
 fail_record_led_alloc:
-- 
2.25.1

Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal

[Jiri Kosina]

On Mon, 27 Jul 2026, Chen Changcheng wrote:

> This series fixes two use-after-free bugs in the Corsair HID driver
> found during code review.  The previous single-patch submission only
> addressed the first issue; the second was identified by Sashiko AI
> review.
> 
> v2:
>   - Split into 2 patches: event vs remove UAF, worker vs cleanup UAF
>   - Rewrote commit messages in plain ASCII
>   - Added drvdata->k90 = NULL as defensive measure (patch 1)
>   - Fixed LED worker ordering in both cleanup functions (patch 2)
> 
> Chen Changcheng (2):
>   HID: corsair: fix use-after-free by reordering remove sequence
>   HID: corsair: cancel worker before unregistering LED to fix
>     use-after-free

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs