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

[Chen Changcheng]

> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://syzkaller.appspot.com/bug?extid=abcedffc9201f2bb66c2
> Assisted-by: LLM
> Signed-off-by: Danish Khateeb <danishkhateeb03@xxxxxxxxx>
> ---
> 
> 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@xxxxxxxxxx>