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

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 <[email protected]>

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/[email protected]?part=2