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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The lockless `led->removed` flag check leaves a TOCTOU race window, allowing concurrent sysfs writes to schedule work after `cancel_work_sync()` has completed, leading to a Use-After-Free.
- [Medium] The patch prevents the physical LED from being turned off when the driver is unbound or fails to probe.
--

commit 682f4ec73cf3b2f84d2fd794aa7a7a3df0638944
Author: Chen Changcheng <chenchangcheng@kylinos.cn>

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

This commit attempts to fix a use-after-free issue by setting a removed
flag to prevent a cancelled LED worker from being re-scheduled during
device unregistration. It adds a check in k90_brightness_set() and
adjusts the probe error path teardown order.

Link: https://lore.kernel.org/all/6a937393.1d9ded08.62e62.0113.GAE@google.com/

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

[Severity: Medium]
Does this prevent the physical LED from being turned off when the driver
is unbound or fails to probe?

During unbinding or a probe error, led->removed is set to true before the
driver calls led_classdev_unregister(). The LED core attempts to cleanly
turn off the hardware by calling led_set_brightness(led_cdev, LED_OFF).
This call reaches k90_brightness_set(), which immediately returns due to
this check, silently dropping the hardware command and leaving the LEDs on.

[Severity: Critical]
Can this lockless check cause a memory corruption due to a Time-of-Check to
Time-of-Use race?

If a thread performing a sysfs write enters k90_brightness_set(), evaluates
led->removed as false, and is preempted:

Another thread handling cleanup can set led->removed to true and call
cancel_work_sync(), which returns immediately since no work is queued yet.
When the cleanup thread calls led_classdev_unregister(), it blocks in
device_unregister() waiting for the sysfs active reference to drain.

The sysfs thread can then resume, execute schedule_work(&led->work),
complete the write, and drop the reference. The cleanup thread unblocks,
finishes unregistration, and calls kfree(), leaving the workqueue to
execute the queued work using the freed memory.

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

[Severity: Critical]
Does propagating this teardown ordering into the probe error path cement
the race condition discussed above?

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