[PATCH] HID: corsair: cancel worker after unregistering LED, not before

[PATCH] HID: corsair: cancel worker after unregistering LED, not before

[Danish Khateeb]

corsair_remove() tears down each LED with:

	removed = true;
	cancel_work_sync(&led->work);
	led_classdev_unregister(&led->cdev);
	kfree(...);

led_classdev_unregister() calls led_set_brightness(led_cdev, LED_OFF),
which reaches the driver's k90_brightness_set() and re-arms the worker
with schedule_work(). That happens after cancel_work_sync() has already
run, so the work item is queued again and the structure holding it is
then freed while it is still linked into the worklist.

The corruption surfaces on the next worklist insertion rather than in
the driver itself:

  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

k90 is freed in k90_cleanup_macro_functions(), and the stale
record_led.work entry left on the worklist is then tripped over by the
queue_work() that k90_cleanup_backlight() performs immediately
afterwards. The syzbot report appears to be the same bug on the other
LED, surfacing later when the worker itself runs and reads led->removed
from freed memory, but that variant was not reproduced here.

Cancel the worker after led_classdev_unregister() so that nothing can
re-arm it before the memory goes away. The removed flag is still set
first, so a worker already past the flag check returns without
dereferencing led->cdev.dev, which is what commit eb51c9f8cb4f ("HID:
corsair: cancel worker before unregistering LED to fix use-after-free")
set out to prevent. The fail_sysfs error path in
k90_init_macro_functions() already uses this ordering.

Reproduced by binding a configfs USB gadget carrying the K90's IDs
(1b1c:1b02) to dummy_hcd and then unbinding it; the splat above is from
the first unbind. With this patch the same sequence runs cleanly under
KASAN.

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

Reproducer, for anyone wanting to confirm the fix. 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; unpatched, the splat above fires
on the first unbind. 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 278c6efb565d..ac12877a6db7 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;
-		cancel_work_sync(&drvdata->backlight->work);
 		led_classdev_unregister(&drvdata->backlight->cdev);
+		cancel_work_sync(&drvdata->backlight->work);
 		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;
-		cancel_work_sync(&k90->record_led.work);
 		led_classdev_unregister(&k90->record_led.cdev);
+		cancel_work_sync(&k90->record_led.work);
 		kfree(k90->record_led.cdev.name);
 
 		kfree(k90);
-- 
2.55.0

Re: [PATCH] HID: corsair: cancel worker after unregistering LED, not before

[Danish Khateeb]

> - [High] Reordering `led_classdev_unregister()` before
> `cancel_work_sync()` reintroduces a Time-of-Check to Time-of-Use
> (TOCTOU) Use-After-Free race condition on `led->cdev.dev` inside the
> worker threads.

You are right. Please drop v1.

There are two races here and each ordering fixes only one. Cancelling
first leaves led_classdev_unregister() free to re-arm the worker via
led_set_brightness(LED_OFF), so the structure is freed while still
linked into the worklist. Cancelling last leaves a worker that has
already tested led->removed free to dereference led->cdev.dev after
device_unregister() has freed it -- the race eb51c9f8cb4f fixed. v1 was
in effect a revert of it, which I should have caught: that race is
spelled out in the changelog of the commit in my own Fixes: tag.

v2 uses disable_work_sync(), which waits for a running worker and makes
the later schedule_work() fail rather than queue, closing both. It also
converts the fail_sysfs error path, which still had the second race.

My reproducer only covered the re-arm race; the other is a few
instructions wide and I could not trigger it. v2 says so.

[PATCH v2] HID: corsair: use disable_work_sync() to tear down the LED workers

[Danish Khateeb]

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