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

[Benjamin Tissoires <bentiss@kernel.org>]

On May 15 2026, sashiko-bot@kernel.org wrote:
> 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?

I suspect this is relatively safe because of the nature of the device: a
handled one so the chances that 2 USB devices using the same driver are
low, but it would be nice to address sashiko's comments in a followup
series. Derek, could you have a depper look, please?

Cheers,
Benjamin

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