[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