drivers/hid/hid-corsair.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Tearing down the K90 LEDs races with their brightness workers in two
different ways, and swapping cancel_work_sync() and
led_classdev_unregister() around only trades one for the other.
Cancelling before the unregister, as k90_cleanup_backlight() and
k90_cleanup_macro_functions() have done since
commit eb51c9f8cb4f ("HID: corsair: cancel worker before
unregistering LED to fix use-after-free"), leaves
led_classdev_unregister() free to re-arm the worker: it calls
led_set_brightness(led_cdev, LED_OFF), which reaches
k90_brightness_set() and schedule_work()s the item again. The structure
holding that work is then freed while it is still linked into the
worklist:
BUG: KASAN: slab-use-after-free in __list_add_valid_or_report+0x186/0x210
Read of size 8 at addr ffff888102a1ead0 by task kworker/0:1/11
__queue_work+0xade/0x1350
queue_work_on+0xb6/0xc0
led_classdev_unregister+0x26b/0x340
corsair_remove+0x1c2/0x2d0
hid_device_remove+0xba/0x1e0
usbhid_disconnect+0xa0/0xe0
Allocated by task 129:
corsair_probe+0x560/0xd50
Freed by task 11:
corsair_remove+0xec/0x2d0
Cancelling after the unregister avoids that, but reintroduces precisely
the use-after-free that commit set out to fix: a worker that has
already tested led->removed and found it false goes on to dereference
led->cdev.dev, which led_classdev_unregister() has meanwhile freed via
device_unregister().
Use disable_work_sync(), which provides both halves at once. It waits
for a worker that is already executing, so nothing can be sitting
between the led->removed test and the led->cdev.dev dereference when the
device goes away, and it makes the subsequent schedule_work() from the
LED_OFF callback fail rather than queue, so nothing is left on the
worklist to be freed. The ordering then stops mattering. The removed
flag is left alone; it is redundant for these paths now but harmless.
The fail_sysfs error path in k90_init_macro_functions() unregisters
before cancelling, and so still carries the dereference-after-free that
was fixed in the teardown paths, so convert it too.
Fixes: eb51c9f8cb4f ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
Reported-by: syzbot+abcedffc9201f2bb66c2@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=abcedffc9201f2bb66c2
Assisted-by: LLM
Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
---
Changes in v2:
- v1 moved cancel_work_sync() after led_classdev_unregister(). That was
in effect a revert of the commit in the Fixes: tag and reintroduced
the use-after-free it fixed; thanks to the Sashiko review on the v1
thread for catching it.
- Use disable_work_sync() instead, which closes both races without
depending on the ordering at all.
- Also convert the fail_sysfs error path in k90_init_macro_functions(),
which v1 left alone and which still had the same use-after-free.
Tested with the reproducer below on a KASAN kernel with panic_on_warn=1,
so any report would have been fatal: 15 probe/remove cycles, no reports.
Unpatched it panics on the first unbind. That exercises the re-arm race
only. The dereference-after-free race is a few instructions wide and I
was not able to trigger it, so that half rests on the disable_work_sync()
semantics and on the analysis in the Fixes: commit.
Reproducer. Needs CONFIG_HID_CORSAIR, CONFIG_USB_DUMMY_HCD,
CONFIG_USB_CONFIGFS_F_HID and KASAN. It fakes a K90 with a configfs
gadget on dummy_hcd and unbinds it. syzbot has no reproducer for this one.
#!/bin/sh
# Reproducer for the hid-corsair teardown use-after-free.
#
# Emulates a Corsair K90 (1b1c:1b02) with a configfs USB gadget bound to
# dummy_hcd, so hid-corsair probes and registers its two LEDs. Unbinding the
# gadget drives corsair_remove(), which is the path under test:
#
# removed = true;
# cancel_work_sync(&work); <- work cancelled
# led_classdev_unregister(&cdev); <- led_set_brightness(LED_OFF) re-queues it
# kfree(k90); <- freed while still on the worklist
#
# The splat surfaces on the *next* worklist insertion, as a KASAN
# slab-use-after-free in __list_add_valid_or_report() under __queue_work().
#
# Run inside a KASAN guest. Fires on the first unbind.
set -e
G=/sys/kernel/config/usb_gadget/k90
mountpoint -q /sys/kernel/config || mount -t configfs none /sys/kernel/config
UDC=$(ls /sys/class/udc | head -1)
[ -n "$UDC" ] || { echo "no UDC found (need CONFIG_USB_DUMMY_HCD)"; exit 1; }
echo "using UDC: $UDC"
cleanup() {
[ -d "$G" ] || return 0
echo "" > "$G/UDC" 2>/dev/null || true
rm -f "$G"/configs/c.1/hid.usb0 2>/dev/null || true
rmdir "$G"/configs/c.1/strings/0x409 "$G"/configs/c.1 2>/dev/null || true
rmdir "$G"/functions/hid.usb0 "$G"/strings/0x409 "$G" 2>/dev/null || true
}
cleanup
mkdir -p "$G"
cd "$G"
echo 0x1b1c > idVendor # USB_VENDOR_ID_CORSAIR
echo 0x1b02 > idProduct # USB_DEVICE_ID_CORSAIR_K90
mkdir -p strings/0x409
echo "0001" > strings/0x409/serialnumber
echo "Corsair" > strings/0x409/manufacturer
echo "K90" > strings/0x409/product
mkdir -p functions/hid.usb0
echo 1 > functions/hid.usb0/protocol # keyboard
echo 1 > functions/hid.usb0/subclass
echo 8 > functions/hid.usb0/report_length
# Standard HID boot-keyboard report descriptor, 63 bytes.
# NOTE: octal escapes, not \xHH -- Debian's /bin/sh is dash, whose printf does
# not implement \xHH and would write the escapes out as literal ASCII text.
printf '\005\001\011\006\241\001\005\007\031\340\051\347\025\000\045\001\165\001\225\010\201\002\225\001\165\010\201\003\225\005\165\001\005\010\031\001\051\005\221\002\225\001\165\003\221\003\225\006\165\010\025\000\045\145\005\007\031\000\051\145\201\000\300' \
> functions/hid.usb0/report_desc
desc_sz=$(wc -c < functions/hid.usb0/report_desc)
[ "$desc_sz" -eq 63 ] || { echo "BAD DESCRIPTOR ($desc_sz bytes, want 63)"; exit 1; }
mkdir -p configs/c.1/strings/0x409
echo "c1" > configs/c.1/strings/0x409/configuration
ln -s functions/hid.usb0 configs/c.1/
echo "binding gadget..."
echo "$UDC" > UDC
sleep 2
dev=$(ls /sys/bus/hid/drivers/corsair/ 2>/dev/null | grep ':' || true)
[ -n "$dev" ] || { echo "hid-corsair did not bind - check dmesg"; cleanup; exit 1; }
echo "bound: $dev"
echo "LEDs: $(ls /sys/class/leds 2>/dev/null | grep "$dev" | tr '\n' ' ')"
echo "--- unbinding (triggers corsair_remove) ---"
echo "" > UDC
sleep 2
cleanup
echo "done (no splat)"
drivers/hid/hid-corsair.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..f966005d69dd 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -507,8 +507,8 @@ static int k90_init_macro_functions(struct hid_device *dev)
fail_sysfs:
k90->record_led.removed = true;
+ disable_work_sync(&k90->record_led.work);
led_classdev_unregister(&k90->record_led.cdev);
- cancel_work_sync(&k90->record_led.work);
fail_record_led:
kfree(k90->record_led.cdev.name);
fail_record_led_alloc:
@@ -524,7 +524,7 @@ static void k90_cleanup_backlight(struct hid_device *dev)
if (drvdata->backlight) {
drvdata->backlight->removed = true;
- cancel_work_sync(&drvdata->backlight->work);
+ disable_work_sync(&drvdata->backlight->work);
led_classdev_unregister(&drvdata->backlight->cdev);
kfree(drvdata->backlight->cdev.name);
kfree(drvdata->backlight);
@@ -540,7 +540,7 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
sysfs_remove_group(&dev->dev.kobj, &k90_attr_group);
k90->record_led.removed = true;
- cancel_work_sync(&k90->record_led.work);
+ disable_work_sync(&k90->record_led.work);
led_classdev_unregister(&k90->record_led.cdev);
kfree(k90->record_led.cdev.name);
--
2.55.0> Tearing down the K90 LEDs races with their brightness workers in two
> different ways, and swapping cancel_work_sync() and
> led_classdev_unregister() around only trades one for the other.
>
> Cancelling before the unregister, as k90_cleanup_backlight() and
> k90_cleanup_macro_functions() have done since
> commit eb51c9f8cb4f ("HID: corsair: cancel worker before
> unregistering LED to fix use-after-free"), leaves
> led_classdev_unregister() free to re-arm the worker: it calls
> led_set_brightness(led_cdev, LED_OFF), which reaches
> k90_brightness_set() and schedule_work()s the item again. The structure
> holding that work is then freed while it is still linked into the
> worklist:
>
> BUG: KASAN: slab-use-after-free in __list_add_valid_or_report+0x186/0x210
> Read of size 8 at addr ffff888102a1ead0 by task kworker/0:1/11
> __queue_work+0xade/0x1350
> queue_work_on+0xb6/0xc0
> led_classdev_unregister+0x26b/0x340
> corsair_remove+0x1c2/0x2d0
> hid_device_remove+0xba/0x1e0
> usbhid_disconnect+0xa0/0xe0
> Allocated by task 129:
> corsair_probe+0x560/0xd50
> Freed by task 11:
> corsair_remove+0xec/0x2d0
>
> Cancelling after the unregister avoids that, but reintroduces precisely
> the use-after-free that commit set out to fix: a worker that has
> already tested led->removed and found it false goes on to dereference
> led->cdev.dev, which led_classdev_unregister() has meanwhile freed via
> device_unregister().
>
> Use disable_work_sync(), which provides both halves at once. It waits
> for a worker that is already executing, so nothing can be sitting
> between the led->removed test and the led->cdev.dev dereference when the
> device goes away, and it makes the subsequent schedule_work() from the
> LED_OFF callback fail rather than queue, so nothing is left on the
> worklist to be freed. The ordering then stops mattering. The removed
> flag is left alone; it is redundant for these paths now but harmless.
>
> The fail_sysfs error path in k90_init_macro_functions() unregisters
> before cancelling, and so still carries the dereference-after-free that
> was fixed in the teardown paths, so convert it too.
>
> Fixes: eb51c9f8cb4f ("HID: corsair: cancel worker before unregistering LED to fix use-after-free")
> Reported-by: syzbot+abcedffc9201f2bb66c2@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?extid=abcedffc9201f2bb66c2
> Assisted-by: LLM
> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
> ---
>
> Changes in v2:
> - v1 moved cancel_work_sync() after led_classdev_unregister(). That was
> in effect a revert of the commit in the Fixes: tag and reintroduced
> the use-after-free it fixed; thanks to the Sashiko review on the v1
> thread for catching it.
> - Use disable_work_sync() instead, which closes both races without
> depending on the ordering at all.
> - Also convert the fail_sysfs error path in k90_init_macro_functions(),
> which v1 left alone and which still had the same use-after-free.
>
> Tested with the reproducer below on a KASAN kernel with panic_on_warn=1,
> so any report would have been fatal: 15 probe/remove cycles, no reports.
> Unpatched it panics on the first unbind. That exercises the re-arm race
> only. The dereference-after-free race is a few instructions wide and I
> was not able to trigger it, so that half rests on the disable_work_sync()
> semantics and on the analysis in the Fixes: commit.
>
> Reproducer. Needs CONFIG_HID_CORSAIR, CONFIG_USB_DUMMY_HCD,
> CONFIG_USB_CONFIGFS_F_HID and KASAN. It fakes a K90 with a configfs
> gadget on dummy_hcd and unbinds it. syzbot has no reproducer for this one.
>
> #!/bin/sh
> # Reproducer for the hid-corsair teardown use-after-free.
> #
> # Emulates a Corsair K90 (1b1c:1b02) with a configfs USB gadget bound to
> # dummy_hcd, so hid-corsair probes and registers its two LEDs. Unbinding the
> # gadget drives corsair_remove(), which is the path under test:
> #
> # removed = true;
> # cancel_work_sync(&work); <- work cancelled
> # led_classdev_unregister(&cdev); <- led_set_brightness(LED_OFF) re-queues it
> # kfree(k90); <- freed while still on the worklist
> #
> # The splat surfaces on the *next* worklist insertion, as a KASAN
> # slab-use-after-free in __list_add_valid_or_report() under __queue_work().
> #
> # Run inside a KASAN guest. Fires on the first unbind.
> set -e
>
> G=/sys/kernel/config/usb_gadget/k90
>
> mountpoint -q /sys/kernel/config || mount -t configfs none /sys/kernel/config
>
> UDC=$(ls /sys/class/udc | head -1)
> [ -n "$UDC" ] || { echo "no UDC found (need CONFIG_USB_DUMMY_HCD)"; exit 1; }
> echo "using UDC: $UDC"
>
> cleanup() {
> [ -d "$G" ] || return 0
> echo "" > "$G/UDC" 2>/dev/null || true
> rm -f "$G"/configs/c.1/hid.usb0 2>/dev/null || true
> rmdir "$G"/configs/c.1/strings/0x409 "$G"/configs/c.1 2>/dev/null || true
> rmdir "$G"/functions/hid.usb0 "$G"/strings/0x409 "$G" 2>/dev/null || true
> }
> cleanup
>
> mkdir -p "$G"
> cd "$G"
> echo 0x1b1c > idVendor # USB_VENDOR_ID_CORSAIR
> echo 0x1b02 > idProduct # USB_DEVICE_ID_CORSAIR_K90
>
> mkdir -p strings/0x409
> echo "0001" > strings/0x409/serialnumber
> echo "Corsair" > strings/0x409/manufacturer
> echo "K90" > strings/0x409/product
>
> mkdir -p functions/hid.usb0
> echo 1 > functions/hid.usb0/protocol # keyboard
> echo 1 > functions/hid.usb0/subclass
> echo 8 > functions/hid.usb0/report_length
> # Standard HID boot-keyboard report descriptor, 63 bytes.
> # NOTE: octal escapes, not \xHH -- Debian's /bin/sh is dash, whose printf does
> # not implement \xHH and would write the escapes out as literal ASCII text.
> printf '\005\001\011\006\241\001\005\007\031\340\051\347\025\000\045\001\165\001\225\010\201\002\225\001\165\010\201\003\225\005\165\001\005\010\031\001\051\005\221\002\225\001\165\003\221\003\225\006\165\010\025\000\045\145\005\007\031\000\051\145\201\000\300' \
> > functions/hid.usb0/report_desc
>
> desc_sz=$(wc -c < functions/hid.usb0/report_desc)
> [ "$desc_sz" -eq 63 ] || { echo "BAD DESCRIPTOR ($desc_sz bytes, want 63)"; exit 1; }
>
> mkdir -p configs/c.1/strings/0x409
> echo "c1" > configs/c.1/strings/0x409/configuration
> ln -s functions/hid.usb0 configs/c.1/
>
> echo "binding gadget..."
> echo "$UDC" > UDC
> sleep 2
>
> dev=$(ls /sys/bus/hid/drivers/corsair/ 2>/dev/null | grep ':' || true)
> [ -n "$dev" ] || { echo "hid-corsair did not bind - check dmesg"; cleanup; exit 1; }
> echo "bound: $dev"
> echo "LEDs: $(ls /sys/class/leds 2>/dev/null | grep "$dev" | tr '\n' ' ')"
>
> echo "--- unbinding (triggers corsair_remove) ---"
> echo "" > UDC
> sleep 2
>
> cleanup
> echo "done (no splat)"
> drivers/hid/hid-corsair.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
> index 278c6efb565d..f966005d69dd 100644
> --- a/drivers/hid/hid-corsair.c
> +++ b/drivers/hid/hid-corsair.c
> @@ -507,8 +507,8 @@ static int k90_init_macro_functions(struct hid_device *dev)
>
> fail_sysfs:
> k90->record_led.removed = true;
> + disable_work_sync(&k90->record_led.work);
> led_classdev_unregister(&k90->record_led.cdev);
> - cancel_work_sync(&k90->record_led.work);
> fail_record_led:
> kfree(k90->record_led.cdev.name);
> fail_record_led_alloc:
> @@ -524,7 +524,7 @@ static void k90_cleanup_backlight(struct hid_device *dev)
>
> if (drvdata->backlight) {
> drvdata->backlight->removed = true;
> - cancel_work_sync(&drvdata->backlight->work);
> + disable_work_sync(&drvdata->backlight->work);
> led_classdev_unregister(&drvdata->backlight->cdev);
> kfree(drvdata->backlight->cdev.name);
> kfree(drvdata->backlight);
> @@ -540,7 +540,7 @@ static void k90_cleanup_macro_functions(struct hid_device *dev)
> sysfs_remove_group(&dev->dev.kobj, &k90_attr_group);
>
> k90->record_led.removed = true;
> - cancel_work_sync(&k90->record_led.work);
> + disable_work_sync(&k90->record_led.work);
> led_classdev_unregister(&k90->record_led.cdev);
> kfree(k90->record_led.cdev.name);
>
> --
> 2.55.0
Hi Jeffin,
Reviewed-by: Chen Changcheng <chenchangcheng@kylinos.cn>
© 2016 - 2026 Red Hat, Inc.