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

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

[Chen Changcheng]

Commit eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering
LED to fix use-after-free") moved cancel_work_sync() ahead of
led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions().  led_classdev_unregister() internally
calls led_set_brightness(LED_OFF), which reaches the driver's
k90_brightness_set() callback.  Since that callback schedules the worker
unconditionally, the worker was re-queued after cancel_work_sync() had
drained it, and the subsequent kfree() freed a still-active work_struct:

    ODEBUG: free active (active state 0) object type: work_struct
            hint: k90_record_led_work

The removed flag check inside the worker itself only stops it from
dereferencing freed memory once it runs; it cannot prevent the re-queue.

Fix this by making k90_brightness_set() a no-op once removed is set, so
the LED_OFF update issued from led_classdev_unregister() can no longer
re-schedule the worker after it has been cancelled.  Also apply the
cancel-before-unregister ordering to the probe error path
(k90_init_macro_functions() fail_sysfs) for consistency.

Fixes: eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
Reported-by: syzbot+0a031a76585d1c7e737d@syzkaller.appspotmail.com
Reported-by: syzbot+0b8bff5929865345b29e@syzkaller.appspotmail.com
https://lore.kernel.org/all/6a937393.1d9ded08.62e62.0113.GAE@google.com/
Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..73b3c1ff78c6 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;
+
 	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);
 fail_record_led:
 	kfree(k90->record_led.cdev.name);
 fail_record_led_alloc:
-- 
2.25.1

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

[sashiko-bot]

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

Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal

[Chen Changcheng]

Hi Jeffin,

Thanks for the review and for the pointer to the syzbot report.

You are right: led_classdev_unregister() internally calls
led_set_brightness(LED_OFF), which reaches k90_brightness_set() and
re-schedules the worker after cancel_work_sync() has returned, so the
work_struct can still be queued when kfree() is called.  This is the
issue syzbot reported (extid=0a031a76585d1c7e737d).

The follow-up patch below makes k90_brightness_set() a no-op once
removed is set, so the LED_OFF update from led_classdev_unregister()
cannot re-schedule the worker after it has been cancelled.  It also
applies the same cancel-before-unregister ordering to the probe error
path in k90_init_macro_functions() for consistency.

Thanks,
Chen Changcheng

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

[Chen Changcheng]

From: Chen Changcheng <chenchangcheng@kylinos.cn>

Commit eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering
LED to fix use-after-free") moved cancel_work_sync() ahead of
led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions().  led_classdev_unregister() internally
calls led_set_brightness(LED_OFF), which reaches the driver's
k90_brightness_set() callback.  Since that callback schedules the worker
unconditionally, the worker was re-queued after cancel_work_sync() had
drained it, and the subsequent kfree() freed a still-active work_struct:

    ODEBUG: free active (active state 0) object type: work_struct
            hint: k90_record_led_work

The removed flag check inside the worker itself only stops it from
dereferencing freed memory once it runs; it cannot prevent the re-queue.

Fix this by making k90_brightness_set() a no-op once removed is set, so
the LED_OFF update issued from led_classdev_unregister() can no longer
re-schedule the worker after it has been cancelled.  Also apply the
cancel-before-unregister ordering to the probe error path
(k90_init_macro_functions() fail_sysfs) for consistency.

Fixes: eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
Reported-by: syzbot+0a031a76585d1c7e737d@syzkaller.appspotmail.com
Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
---
 drivers/hid/hid-corsair.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..73b3c1ff78c6 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;
+
 	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);
 fail_record_led:
 	kfree(k90->record_led.cdev.name);
 fail_record_led_alloc:
-- 
2.25.1

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

[sashiko-bot]

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

New issues:
- [High] Moving cancel_work_sync() before led_classdev_unregister() in the probe error path introduces a TOCTOU race with concurrent sysfs writes, leading to a Use-After-Free.

Pre-existing issues:
- [High] Hardware events can trigger Use-After-Free in corsair_event() due to hid_hw_stop() being called after freeing driver data in corsair_remove().
- [High] TOCTOU Use-after-free in LED workers due to led_classdev_unregister() being called before cancel_work_sync() in cleanup functions.
--

--- Patch [1]: [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled ---
commit 4542e75f6964d50f56a268d347abdebb867081d5
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 by preventing the LED worker
from being re-scheduled after it has been cancelled. It adds a removed flag
check and reorders the cleanup sequence in the probe error path.

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

[ ... ]

> @@ -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: High]
Does swapping these calls introduce a Time-of-Check to Time-of-Use race
with concurrent sysfs writes?

Since led_classdev_unregister() acts as the synchronization barrier against
sysfs writes, moving cancel_work_sync() before it might allow a concurrent
write to bypass the barrier.

Could a concurrent sysfs write evaluate led->removed as false in
k90_brightness_set(), get preempted, and then schedule the work after
cancel_work_sync() has already finished?

Sysfs thread:
k90_brightness_set()
    if (led->removed)
        return;
    <preempted>

Probe error thread:
fail_sysfs:
    k90->record_led.removed = true;
    cancel_work_sync(&k90->record_led.work);
    led_classdev_unregister(&k90->record_led.cdev);

Sysfs thread resumes:
    led->brightness = brightness;
    schedule_work(&led->work);

If the probe error thread then finishes and frees the k90 struct, wouldn't
the newly queued work execute later on freed memory?

>  fail_record_led:
>  	kfree(k90->record_led.cdev.name);
>  fail_record_led_alloc:


This is a pre-existing issue, but looking at the commit message and the
cleanup functions:

The commit message states that a previous commit moved cancel_work_sync()
ahead of led_classdev_unregister() in k90_cleanup_backlight() and
k90_cleanup_macro_functions() to fix a use-after-free. However, looking at
those functions in the current code, led_classdev_unregister() is still
called first:

k90_cleanup_macro_functions(struct hid_device *dev) {
    ...
    led_classdev_unregister(&k90->record_led.cdev);
    cancel_work_sync(&k90->record_led.work);
    ...
}

[Severity: High]
Does calling led_classdev_unregister() before cancel_work_sync() cause a
use-after-free during driver unbind?

led_classdev_unregister() drops the refcount and frees the underlying device
struct. If a worker thread like k90_backlight_work is executing concurrently,
passes the removed check, and gets preempted, it could resume after the
device is freed:

k90_backlight_work(struct work_struct *work) {
    ...
    dev = led->cdev.dev->parent;
    ...
}


This is also a pre-existing issue, but looking at corsair_remove():

corsair_remove(struct hid_device *dev) {
    ...
    k90_cleanup_macro_functions(dev);
    k90_cleanup_backlight(dev);

    hid_hw_stop(dev);
}

[Severity: High]
Does calling hid_hw_stop() after freeing driver data allow hardware events
to trigger a use-after-free?

In k90_cleanup_macro_functions(), the k90 struct is freed via kfree(k90),
but the drvdata->k90 pointer is not set to NULL. Since hid_hw_stop() hasn't
been called yet, the HID core can still deliver incoming USB hardware events.

If an event arrives in this window before the hardware is stopped, could
corsair_event() dereference the dangling pointer?

corsair_event(...) {
    ...
    drvdata->k90->record_led.brightness = 1;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817072831.139954-1-ccc194101@163.com?part=1

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

[Jeffin Philip]

On Mon, 17 Aug 2026 15:28:31 +0800, Chen Changcheng wrote:
>Commit eb51c9f8cb4f0 ("HID: corsair: cancel worker before unregistering
>LED to fix use-after-free") moved cancel_work_sync() ahead of
>led_classdev_unregister() in k90_cleanup_backlight() and
>k90_cleanup_macro_functions().  led_classdev_unregister() internally
>calls led_set_brightness(LED_OFF), which reaches the driver's
>k90_brightness_set() callback.  Since that callback schedules the worker
>unconditionally, the worker was re-queued after cancel_work_sync() had
>drained it, and the subsequent kfree() freed a still-active work_struct:
>
>    ODEBUG: free active (active state 0) object type: work_struct
>            hint: k90_record_led_work
>
>The removed flag check inside the worker itself only stops it from
>dereferencing freed memory once it runs; it cannot prevent the re-queue.
>
>Fix this by making k90_brightness_set() a no-op once removed is set, so
>the LED_OFF update issued from led_classdev_unregister() can no longer
>re-schedule the worker after it has been cancelled.  Also apply the
>cancel-before-unregister ordering to the probe error path
>(k90_init_macro_functions() fail_sysfs) for consistency.

This UAF[1] can be prevented by your patch, would you consider adding that too
in your patch?

[1]: https://lore.kernel.org/all/6a937393.1d9ded08.62e62.0113.GAE@google.com/#R

Thanks,
Jeffin.