Re: [PATCH v2 1/2] HID: lenovo-go: reject non-USB transports in probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] This problem wasn't introduced by this patch, but both drivers use a static global variable (`drvdata`) to store device state rather than allocating it per-device.
- [High] This problem wasn't introduced by this patch, but multicolor LED configuration structs (`go_cdev_rgb` and `gos_cdev_rgb`) are declared as static global variables but are passed to `devm_led_classdev_multicolor_register()`.
- [High] This problem wasn't introduced by this patch, but in `hid-lenovo-go.c`, the removal function `hid_go_cfg_remove()` fails to cancel the delayed workqueue `go_cfg_setup`.
- [High] This problem wasn't introduced by this patch, but probe failures do not correctly unwind started hardware states and sysfs groups.
- [High] This problem wasn't introduced by this patch, but a NULL pointer is passed to `memcpy` in `mcu_property_out()` when performing `GET_*` property requests.
- [High] This problem wasn't introduced by this patch, but completion synchronization is broken in `mcu_property_out()` due to incorrect placement of `reinit_completion()`.
--

commit 7a455849d3633575795dff10836095e31a4bbca3
Author: Louis Clinckx <clinckx.louis@gmail.com>

HID: lenovo-go: reject non-USB transports in probe

These drivers only match HID_USB_DEVICE() entries and assume the
underlying bus is USB. Make that explicit at probe by rejecting any
non-USB hdev.

> diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
> index 01c7bdd4fbe04..abf477e680428 100644
> --- a/drivers/hid/hid-lenovo-go-s.c
> +++ b/drivers/hid/hid-lenovo-go-s.c
> @@ -1432,6 +1432,9 @@ static int hid_gos_probe(struct hid_device *hdev,
>  {
>  	int ret, ep;
>  
> +	if (!hid_is_usb(hdev))
> +		return -EINVAL;
> +

[ ... ]

>  	ret = hid_gos_cfg_probe(hdev, id);
>  	if (ret)
>  		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");

This isn't a regression from this patch, but do these probe failures
leave the hardware state active?

If hid_gos_cfg_probe() fails, the code returns the error immediately without
calling hid_hw_close(hdev) and hid_hw_stop(hdev).

Additionally, does sysfs_create_groups() need to be rolled back if later
steps in the config probe fail?

>  	dev_dbg(&hdev->dev, "Started interface %x as Go S configuration interface\n", ep);
>  	return ret;
>  }

[ ... ]

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index d4d26c7833563..3fa1fe83f7e5e 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -2419,6 +2419,9 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	int ret, ep;
>  
> +	if (!hid_is_usb(hdev))
> +		return -EINVAL;
> +

[ ... ]

>  	ret = hid_go_cfg_probe(hdev, id);
>  	if (ret)
>  		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");

This isn't a regression from this patch, but does this early return
bypass hid_hw_close(hdev) and hid_hw_stop(hdev) as well?

>  	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);
>  
>  	return ret;
>  }

[ ... ]

This isn't a regression from this patch, but does the use of a static
global drvdata struct cause data races when multiple devices are connected?

Since drvdata is shared, functions like mutex_init() and init_completion()
called during probe() would re-execute on the same memory space. Could this
cause type confusion over drvdata.hdev and memory corruption from concurrent
sysfs accesses or input events?

This isn't a regression from this patch, but do the static global
go_cdev_rgb and gos_cdev_rgb structs corrupt the kernel's global leds_list?

The led framework embeds a struct list_head into the led_classdev structure.
If devm_led_classdev_multicolor_register() is called for multiple devices
using the same static struct, it would double-add the list node.

Would unbinding one device then unlink the shared struct, leading to a
use-after-free when the other device is later accessed or removed?

This isn't a regression from this patch, but does hid_go_cfg_remove()
need to cancel the go_cfg_setup delayed workqueue?

If the controller is rapidly unplugged after hid_go_cfg_probe() schedules
drvdata.go_cfg_setup, hid_go_cfg_remove() will tear down the device.
Without calling cancel_delayed_work_sync(), could the workqueue execute
and dereference the freed hdev pointer in mcu_property_out()?

This isn't a regression from this patch, but does mcu_property_out() pass
a NULL pointer to memcpy() during get property commands?

Callers intentionally pass NULL as the data pointer and 0 as len for these
commands. Since passing NULL to memcpy() is undefined behavior even when
len is 0, could this trigger sanitizer violations?

This isn't a regression from this patch, but is the completion
synchronization in mcu_property_out() susceptible to spontaneous hardware
events?

Because reinit_completion() is called at the end of mcu_property_out(), if
hid_go_raw_event() invokes complete() due to spontaneous events or multiple
responses, the completion object could be left in a signaled state.

Would the next call to wait_for_completion_interruptible_timeout() then
return instantly and provide stale or uninitialized memory to user-space?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778701376.git.clinckx.louis@gmail.com?part=1