Re: [PATCH 03/10] HID: hid-lenovo-go-s: Prevent deadlock if device removed during setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid-lenovo-go-s: UAF in usbhid_output_report due to unprotected sysfs access during remove
--

commit 5d8a6b9ab527316bf05d3abf3dffc7d80baf351f
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-lenovo-go-s: Prevent deadlock if device removed during setup

This commit fixes a deadlock by removing the cfg_mutex guard in
hid_gos_cfg_remove() before stopping the delayed work. It also switches
from cancel_delayed_work_sync() to disable_delayed_work_sync().

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 36505d8402ff1..8b16fc7d151e0 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
> @@ -1442,8 +1442,7 @@ static int hid_gos_cfg_probe(struct hid_device *hdev,
>  
>  static void hid_gos_cfg_remove(struct hid_device *hdev)
>  {
> -	guard(mutex)(&drvdata.cfg_mutex);
> -	cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
> +	disable_delayed_work_sync(&drvdata.gos_cfg_setup);

[Severity: High]
Does removing this mutex guard expose hid_hw_stop() to concurrent sysfs
accesses, potentially leading to a use-after-free?

Because the LED class device is registered using devres, its sysfs
attributes remain active until after hid_gos_cfg_remove() returns. Without
this mutex, a concurrent sysfs write to the LED brightness or RGB effect
attributes can invoke mcu_property_out() and then hid_hw_output_report()
while hid_hw_stop() is executing.

When hid_hw_stop() calls down to usbhid_stop(), it frees usbhid->urbout
and sets it to NULL without locking against concurrent
usbhid_output_report() calls. A racing usbhid_output_report() could pass
the !usbhid->urbout check and dereference the freed URB in
usb_interrupt_msg(), leading to a kernel panic if a local user writes to
sysfs during device hot-unplug.

Could the mutex be reacquired after disable_delayed_work_sync() to protect
the remaining teardown sequence?

>  	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914225303.868569-1-derekjohn.clark@gmail.com?part=3