* [PATCH] HID: corsair: fix use-after-free by reordering remove sequence
@ 2026-07-24 9:19 Chen Changcheng
2026-07-24 9:37 ` sashiko-bot
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
0 siblings, 2 replies; 12+ messages in thread
From: Chen Changcheng @ 2026-07-24 9:19 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] HID: corsair: fix use-after-free by reordering remove sequence
2026-07-24 9:19 [PATCH] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
@ 2026-07-24 9:37 ` sashiko-bot
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-07-24 9:37 UTC (permalink / raw)
To: Chen Changcheng; +Cc: dmitry.torokhov, linux-input
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal
2026-07-24 9:19 [PATCH] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
2026-07-24 9:37 ` sashiko-bot
@ 2026-07-27 1:34 ` Chen Changcheng
2026-07-27 1:34 ` [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
` (2 more replies)
1 sibling, 3 replies; 12+ messages in thread
From: Chen Changcheng @ 2026-07-27 1:34 UTC (permalink / raw)
To: bentiss, jikos; +Cc: linux-input, linux-kernel, Chen Changcheng
This series fixes two use-after-free bugs in the Corsair HID driver
found during code review. The previous single-patch submission only
addressed the first issue; the second was identified by Sashiko AI
review.
v2:
- Split into 2 patches: event vs remove UAF, worker vs cleanup UAF
- Rewrote commit messages in plain ASCII
- Added drvdata->k90 = NULL as defensive measure (patch 1)
- Fixed LED worker ordering in both cleanup functions (patch 2)
Chen Changcheng (2):
HID: corsair: fix use-after-free by reordering remove sequence
HID: corsair: cancel worker before unregistering LED to fix
use-after-free
drivers/hid/hid-corsair.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
@ 2026-07-27 1:34 ` Chen Changcheng
2026-07-27 1:46 ` sashiko-bot
2026-07-27 1:35 ` [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free Chen Changcheng
2026-08-03 19:22 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jiri Kosina
2 siblings, 1 reply; 12+ messages in thread
From: Chen Changcheng @ 2026-07-27 1:34 UTC (permalink / raw)
To: bentiss, jikos; +Cc: linux-input, linux-kernel, Chen Changcheng
The corsair_remove() function currently frees the k90 driver data before
calling hid_hw_stop(). Since hid_hw_stop() stops HID I/O, the event
callback corsair_event() can still be invoked between the kfree() and
hid_hw_stop(), and will dereference the freed drvdata->k90 pointer to
write record_led.brightness.
Reorder the remove sequence so that hid_hw_stop() is called first.
Once hid_hw_stop() completes, the HID device is disconnected and no
URBs are active, so corsair_event() cannot fire anymore. The driver
data is freed only afterwards.
Additionally, set drvdata->k90 to NULL after kfree() as a defensive
measure, matching the existing pattern in the error path of
k90_init_macro_functions().
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
2026-07-27 1:34 ` [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
@ 2026-07-27 1:35 ` Chen Changcheng
2026-07-27 1:45 ` sashiko-bot
2026-08-15 18:12 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jeffin Philip
2026-08-03 19:22 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jiri Kosina
2 siblings, 2 replies; 12+ messages in thread
From: Chen Changcheng @ 2026-07-27 1:35 UTC (permalink / raw)
To: bentiss, jikos; +Cc: linux-input, linux-kernel, Chen Changcheng, sashiko-bot
The cleanup functions k90_cleanup_backlight() and
k90_cleanup_macro_functions() call led_classdev_unregister() before
cancel_work_sync():
led_classdev_unregister() <-- may free led->cdev.dev
cancel_work_sync() <-- wait for worker
If the LED worker (k90_backlight_work / k90_record_led_work) is
already running on another CPU, the following race can occur:
CPU 1 (worker) CPU 2 (remove)
--------------------- --------------------
if (led->removed) -> false
(passed the guard, about to read led->cdev.dev)
* preempted
removed = true
led_classdev_unregister()
-> led->cdev.dev freed
cancel_work_sync()
-> waits for worker
* resumes
dev = led->cdev.dev->parent <-- UAF!
Fix by swapping the order so that the worker is cancelled first:
cancel_work_sync() <-- wait for worker first
led_classdev_unregister() <-- then safe to unregister
The removed flag is set before cancel_work_sync() so that if
led_classdev_unregister() internally triggers another brightness
update (which re-schedules the work), the worker will see the flag
and return immediately.
Signed-off-by: Chen Changcheng <chenchangcheng@kylinos.cn>
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
---
drivers/hid/hid-corsair.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index ac12877a6db7..278c6efb565d 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);
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);
kfree(k90->record_led.cdev.name);
kfree(k90);
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free
2026-07-27 1:35 ` [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free Chen Changcheng
@ 2026-07-27 1:45 ` sashiko-bot
2026-08-15 18:12 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jeffin Philip
1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-07-27 1:45 UTC (permalink / raw)
To: Chen Changcheng; +Cc: dmitry.torokhov, linux-input
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 <chenchangcheng@kylinos.cn>
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/20260727013500.23435-1-chenchangcheng@kylinos.cn?part=2
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence
2026-07-27 1:34 ` [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
@ 2026-07-27 1:46 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-07-27 1:46 UTC (permalink / raw)
To: Chen Changcheng; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Race condition in k90_init_macro_functions() error path leading to Use-After-Free in corsair_event().
--
--- Patch [1]: [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence ---
commit bca54ac8929144d32ee56dbe443017efaac3d7f5
Author: Chen Changcheng <chenchangcheng@kylinos.cn>
HID: corsair: fix use-after-free by reordering remove sequence
This commit prevents a use-after-free during driver removal by reordering
corsair_remove() to call hid_hw_stop() before cleaning up device data. It
also explicitly nullifies the drvdata->k90 pointer after freeing it.
> 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
> @@ -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;
[Severity: High]
This is a pre-existing issue, but does the initialization error path you are
mirroring here contain a race condition?
The commit message notes this matches the pattern in
k90_init_macro_functions(), but in corsair_probe(), hid_hw_start() is called
before k90_init_macro_functions(), meaning device events are fully active
during initialization.
If k90_init_macro_functions() encounters an error, the error path frees k90
but delays setting drvdata->k90 to NULL:
drivers/hid/hid-corsair.c:k90_init_macro_functions() {
...
fail_record_led_alloc:
kfree(k90);
fail_drvdata:
drvdata->k90 = NULL;
return ret;
}
Could an incoming USB interrupt trigger corsair_event() concurrently, causing
it to write to the freed drvdata->k90 pointer before it gets nullified?
drivers/hid/hid-corsair.c:corsair_event() {
...
if (!drvdata->k90)
return 0;
switch (usage->hid & HID_USAGE) {
case CORSAIR_USAGE_MACRO_RECORD_START:
drvdata->k90->record_led.brightness = 1;
...
}
> }
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260727013500.23435-1-chenchangcheng@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
2026-07-27 1:34 ` [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
2026-07-27 1:35 ` [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free Chen Changcheng
@ 2026-08-03 19:22 ` Jiri Kosina
2 siblings, 0 replies; 12+ messages in thread
From: Jiri Kosina @ 2026-08-03 19:22 UTC (permalink / raw)
To: Chen Changcheng; +Cc: bentiss, linux-input, linux-kernel
On Mon, 27 Jul 2026, Chen Changcheng wrote:
> This series fixes two use-after-free bugs in the Corsair HID driver
> found during code review. The previous single-patch submission only
> addressed the first issue; the second was identified by Sashiko AI
> review.
>
> v2:
> - Split into 2 patches: event vs remove UAF, worker vs cleanup UAF
> - Rewrote commit messages in plain ASCII
> - Added drvdata->k90 = NULL as defensive measure (patch 1)
> - Fixed LED worker ordering in both cleanup functions (patch 2)
>
> Chen Changcheng (2):
> HID: corsair: fix use-after-free by reordering remove sequence
> HID: corsair: cancel worker before unregistering LED to fix
> use-after-free
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal
2026-07-27 1:35 ` [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free Chen Changcheng
2026-07-27 1:45 ` sashiko-bot
@ 2026-08-15 18:12 ` Jeffin Philip
2026-08-17 7:23 ` Chen Changcheng
1 sibling, 1 reply; 12+ messages in thread
From: Jeffin Philip @ 2026-08-15 18:12 UTC (permalink / raw)
To: chenchangcheng; +Cc: bentiss, jikos, linux-input, linux-kernel, sashiko-bot
On Mon, 27 Jul 2026 09:35:00 +0800, Chen Changcheng wrote:
>The cleanup functions k90_cleanup_backlight() and
>k90_cleanup_macro_functions() call led_classdev_unregister() before
>cancel_work_sync():
>
> led_classdev_unregister() <-- may free led->cdev.dev
> cancel_work_sync() <-- wait for worker
>
>If the LED worker (k90_backlight_work / k90_record_led_work) is
>already running on another CPU, the following race can occur:
>
> CPU 1 (worker) CPU 2 (remove)
> --------------------- --------------------
> if (led->removed) -> false
> (passed the guard, about to read led->cdev.dev)
> * preempted
> removed = true
> led_classdev_unregister()
> -> led->cdev.dev freed
> cancel_work_sync()
> -> waits for worker
> * resumes
> dev = led->cdev.dev->parent <-- UAF!
>
>Fix by swapping the order so that the worker is cancelled first:
>
> cancel_work_sync() <-- wait for worker first
> led_classdev_unregister() <-- then safe to unregister
>
>The removed flag is set before cancel_work_sync() so that if
>led_classdev_unregister() internally triggers another brightness
>update (which re-schedules the work), the worker will see the flag
>and return immediately.
The premise looks good, but after re-scheduling the work(possibly),
what happens when we call kfree in the cleanup function, that leads
to a ODEBUG warning as our work might be active when we try to kfree.
How can this solve the ODEBUG warning?
Reproduced here: https://syzkaller.appspot.com/bug?extid=0a031a76585d1c7e737d
Thanks,
Jeffin.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal
2026-08-15 18:12 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jeffin Philip
@ 2026-08-17 7:23 ` Chen Changcheng
2026-08-17 7:28 ` [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled Chen Changcheng
0 siblings, 1 reply; 12+ messages in thread
From: Chen Changcheng @ 2026-08-17 7:23 UTC (permalink / raw)
To: jeffinphilip14
Cc: bentiss, chenchangcheng, jikos, linux-input, linux-kernel,
sashiko-bot, ccc194101
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled
2026-08-17 7:23 ` Chen Changcheng
@ 2026-08-17 7:28 ` Chen Changcheng
2026-08-17 7:47 ` sashiko-bot
0 siblings, 1 reply; 12+ messages in thread
From: Chen Changcheng @ 2026-08-17 7:28 UTC (permalink / raw)
To: ccc194101
Cc: bentiss, chenchangcheng, jeffinphilip14, jikos, linux-input,
linux-kernel, syzbot+0a031a76585d1c7e737d
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled
2026-08-17 7:28 ` [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled Chen Changcheng
@ 2026-08-17 7:47 ` sashiko-bot
0 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-08-17 7:47 UTC (permalink / raw)
To: Chen Changcheng; +Cc: linux-input, dmitry.torokhov
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2026-08-17 7:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-24 9:19 [PATCH] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
2026-07-24 9:37 ` sashiko-bot
2026-07-27 1:34 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Chen Changcheng
2026-07-27 1:34 ` [PATCH 1/2] HID: corsair: fix use-after-free by reordering remove sequence Chen Changcheng
2026-07-27 1:46 ` sashiko-bot
2026-07-27 1:35 ` [PATCH 2/2] HID: corsair: cancel worker before unregistering LED to fix use-after-free Chen Changcheng
2026-07-27 1:45 ` sashiko-bot
2026-08-15 18:12 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jeffin Philip
2026-08-17 7:23 ` Chen Changcheng
2026-08-17 7:28 ` [PATCH] HID: corsair: do not re-schedule LED worker after it has been cancelled Chen Changcheng
2026-08-17 7:47 ` sashiko-bot
2026-08-03 19:22 ` [PATCH 0/2] HID: corsair: fix two use-after-free bugs on device removal Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox