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

[sashiko-bot@kernel.org]

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