[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing Use-After-Free race condition in k90_record_led_work and k90_backlight_work.
--

--- Patch [1]: [PATCH] HID: corsair: fix use-after-free by reordering remove sequence ---
commit 79119529f5e0e6e15e9750d9ceb51ce25886fb73
Author: Chen Changcheng <chenchangcheng@kylinos.cn>
Subject: HID: corsair: fix use-after-free by reordering remove sequence

This commit fixes a use-after-free in the corsair driver during device
removal. It reorders the remove sequence to call hid_hw_stop() before
freeing structures, preventing concurrent USB events from accessing freed
memory. It also sets drvdata->k90 to NULL after freeing it as a defensive
measure.

> diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
> index 21cd8b12a7572..ac12877a6db7b 100644
> --- a/drivers/hid/hid-corsair.c
> +++ b/drivers/hid/hid-corsair.c

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race between
these cleanup functions and the LED worker threads?

If a user modifies the LED brightness via sysfs, it schedules
k90_backlight_work() or k90_record_led_work(). If the device is unplugged
concurrently, they can race:

CPU1 (Worker)
k90_backlight_work() {
    if (led->removed)
        return;
    <preempted>

CPU2 (Remove)
corsair_remove() {
    k90_cleanup_backlight() {
        drvdata->backlight->removed = true;
        led_classdev_unregister(&drvdata->backlight->cdev);
        // This unregisters and can free led->cdev.dev

        cancel_work_sync(&drvdata->backlight->work);
        // Blocks waiting for CPU1
    }
}

CPU1 (Worker resumes)
    dev = led->cdev.dev->parent; 
    // UAF dereferencing the freed led->cdev.dev

Would swapping the order of led_classdev_unregister() and cancel_work_sync()
in both cleanup functions prevent this memory corruption?

> -
> -	hid_hw_stop(dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724091906.240588-1-chenchangcheng@kylinos.cn?part=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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in k90_init_macro_functions() error path leading to Use-After-Free in corsair_event().
--

--- Patch [1]: [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence ---
commit bca54ac8929144d32ee56dbe443017efaac3d7f5
Author: Chen Changcheng <chenchangcheng@kylinos.cn>

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

This commit prevents a use-after-free during driver removal by reordering
corsair_remove() to call hid_hw_stop() before cleaning up device data. It
also explicitly nullifies the drvdata->k90 pointer after freeing it.

> diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
> index 21cd8b12a7572..ac12877a6db7b 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;

[Severity: High]
This is a pre-existing issue, but does the initialization error path you are
mirroring here contain a race condition?

The commit message notes this matches the pattern in
k90_init_macro_functions(), but in corsair_probe(), hid_hw_start() is called
before k90_init_macro_functions(), meaning device events are fully active
during initialization.

If k90_init_macro_functions() encounters an error, the error path frees k90
but delays setting drvdata->k90 to NULL:

drivers/hid/hid-corsair.c:k90_init_macro_functions() {
...
fail_record_led_alloc:
	kfree(k90);
fail_drvdata:
	drvdata->k90 = NULL;
	return ret;
}

Could an incoming USB interrupt trigger corsair_event() concurrently, causing
it to write to the freed drvdata->k90 pointer before it gets nullified?

drivers/hid/hid-corsair.c:corsair_event() {
...
	if (!drvdata->k90)
		return 0;

	switch (usage->hid & HID_USAGE) {
	case CORSAIR_USAGE_MACRO_RECORD_START:
		drvdata->k90->record_led.brightness = 1;
...
}

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727013500.23435-1-chenchangcheng@kylinos.cn?part=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 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Reordering `cancel_work_sync()` before `led_classdev_unregister()` introduces a deterministic Use-After-Free and workqueue corruption, because unregistering the LED unconditionally schedules new work immediately before the work struct is freed.

Pre-existing issues:
- [High] The original race condition causing a use-after-free was not fixed in the initialization error path.
- [Medium] The driver manually registers LED classdevs instead of using the managed `devm_led_classdev_register()` API.
--

--- Patch [2]: [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free ---
commit 8341efa9384aeb1521083777d15f9e9a3ba4a7fd
Author: Chen Changcheng <chenchangcheng@kylinos.cn>

HID: corsair: cancel worker before unregistering LED to fix use-after-free

This patch swaps the order of cancel_work_sync() and
led_classdev_unregister() in the cleanup paths of the corsair HID driver.
It attempts to fix a race condition where the LED worker accesses device
structures while they are being unregistered.

> diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
> index ac12877a6db7b..278c6efb565df 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);

[Severity: Critical]
Does this sequence introduce a use-after-free? When unregistering the LED,
led_classdev_unregister() turns off the LED by default, calling the
brightness_set callback. In this driver, k90_brightness_set() calls
schedule_work().

If led_classdev_unregister() is called after cancel_work_sync(), it will
unconditionally schedule a new work item on the workqueue immediately before
the following kfree() destroys the work structure.

>  		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);

[Severity: Critical]
Similarly, does this apply to the macro record LED? Moving the unregister
call after cancel_work_sync() will schedule work right before freeing the
k90 structure in k90_cleanup_macro_functions().

>  		kfree(k90->record_led.cdev.name);
>  
>  		kfree(k90);

This isn't a bug introduced by this patch, but the error path in
k90_init_macro_functions() was not updated to match the new ordering:

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);
    ...
}

[Severity: High]
Does this error path retain the original race condition that this patch aims
to fix?

This is a pre-existing issue, but the driver manually registers the LED
class devices in the initialization functions:

drivers/hid/hid-corsair.c:k90_init_backlight() {
    ...
    ret = led_classdev_register(&dev->dev, &drvdata->backlight->cdev);
    ...
}

[Severity: Medium]
Could the driver be converted to use the managed devm_led_classdev_register()
API to automatically handle lifecycle cleanup and prevent registration leaks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727013500.23435-1-chenchangcheng@kylinos.cn?part=2