* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.