[PATCH v2 0/2] HID: lenovo-go: clean up USB assumption

[PATCH v2 0/2] HID: lenovo-go: clean up USB assumption

[Louis Clinckx]

These drivers' id_tables only match HID_USB_DEVICE() entries and the
code freely assumes a USB transport (to_usb_interface() on hdev->dev.parent,
endpoint access in raw_event). Make the assumption explicit at probe and
drop a NULL check that the compiler-level definition of to_usb_interface()
makes unreachable.

No functional change for the supported (USB) devices.

Louis Clinckx (2):
  HID: lenovo-go: reject non-USB transports in probe
  HID: lenovo-go: drop dead NULL check on to_usb_interface()

 drivers/hid/hid-lenovo-go-s.c | 11 ++++++-----
 drivers/hid/hid-lenovo-go.c   |  6 +++---
 2 files changed, 9 insertions(+), 8 deletions(-)

---
Changes since v1:
 - Resubmitted on linux-input as a quality fix per Benjamin's review of v1
   (sent to security@kernel.org; the HID_USB_DEVICE() id_table already
    filters BUS_USB, so no exploitable path).
 - Added patch 2 to remove the dead NULL check, at Derek's suggestion.

base-commit: 50897c955902c93ae71c38698abb910525ebdc89
--
2.39.5

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

[Louis Clinckx]

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, following the pattern used by other HID drivers.

Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 3 +++
 drivers/hid/hid-lenovo-go.c   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index 01c7bdd4f..abf477e68 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_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "Parse failed\n");
diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index d4d26c783..3fa1fe83f 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;
+
 	hdev->quirks |= HID_QUIRK_INPUT_PER_APP | HID_QUIRK_MULTI_INPUT;
 
 	ret = hid_parse(hdev);
-- 
2.39.5

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

[sashiko-bot]

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

[PATCH v2 2/2] HID: lenovo-go: drop dead NULL check on to_usb_interface()

[Louis Clinckx]

to_usb_interface() is a container_of_const() macro: it performs
pointer arithmetic and never returns NULL. The if (!intf) and if
(intf) tests in get_endpoint_address() can never fire. Remove them
in both drivers.

No functional change.

Suggested-by: Derek J. Clark <derekjohn.clark@gmail.com>
Signed-off-by: Louis Clinckx <clinckx.louis@gmail.com>
---
 drivers/hid/hid-lenovo-go-s.c | 8 +++-----
 drivers/hid/hid-lenovo-go.c   | 3 ---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go-s.c b/drivers/hid/hid-lenovo-go-s.c
index abf477e68..f44aeb2c1 100644
--- a/drivers/hid/hid-lenovo-go-s.c
+++ b/drivers/hid/hid-lenovo-go-s.c
@@ -382,11 +382,9 @@ static int get_endpoint_address(struct hid_device *hdev)
 	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
 	struct usb_host_endpoint *ep;
 
-	if (intf) {
-		ep = intf->cur_altsetting->endpoint;
-		if (ep)
-			return ep->desc.bEndpointAddress;
-	}
+	ep = intf->cur_altsetting->endpoint;
+	if (ep)
+		return ep->desc.bEndpointAddress;
 
 	return -ENODEV;
 }
diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 3fa1fe83f..e0c9d5ec9 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -641,9 +641,6 @@ static int get_endpoint_address(struct hid_device *hdev)
 	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
 	struct usb_host_endpoint *ep;
 
-	if (!intf)
-		return -ENODEV;
-
 	ep = intf->cur_altsetting->endpoint;
 	if (!ep)
 		return -ENODEV;
-- 
2.39.5

Re: [PATCH v2 2/2] HID: lenovo-go: drop dead NULL check on to_usb_interface()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Resource Leak and Use-After-Free due to Asymmetric Device Cleanup
--

commit 7509c2865ff8f5400d44c180711298e07d9096ba
Author: Louis Clinckx <clinckx.louis@gmail.com>

HID: lenovo-go: drop dead NULL check on to_usb_interface()

to_usb_interface() is a container_of_const() macro: it performs
pointer arithmetic and never returns NULL. The if (!intf) and if
(intf) tests in get_endpoint_address() can never fire. Remove them
in both drivers.

[ ... ]

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 3fa1fe83f7e5e..e0c9d5ec9451b 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -641,9 +641,6 @@ static int get_endpoint_address(struct hid_device *hdev)
>  	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>  	struct usb_host_endpoint *ep;
>  
> -	if (!intf)
> -		return -ENODEV;
> -
>  	ep = intf->cur_altsetting->endpoint;
>  	if (!ep)
>  		return -ENODEV;

This isn't a regression introduced by your patch, but I noticed an issue with
how -ENODEV is handled by the driver.

If get_endpoint_address() returns -ENODEV (e.g., when the USB interface has no
endpoints and ep is NULL), hid_go_probe() successfully binds the device:

hid_go_probe() {
    ...
    ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    ...
    ret = hid_hw_open(hdev);
    ...
    ep = get_endpoint_address(hdev);
    if (ep != GO_GP_INTF_IN) {
        dev_dbg(&hdev->dev, "Started interface %x as generic HID device\n", ep);
        return 0;
    }
    ...
}

However, hid_go_remove() skips the cleanup if ep is -ENODEV:

hid_go_remove() {
    int ep = get_endpoint_address(hdev);

    if (ep <= 0)
        return;
    ...
}

Does this asymmetric cleanup cause a use-after-free or leak of the input devices
and HID parser buffers since hid_hw_close() and hid_hw_stop() are skipped?

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

Re: [PATCH v2 0/2] HID: lenovo-go: clean up USB assumption

[Derek J. Clark]

On May 15, 2026 7:57:38 AM PDT, Louis Clinckx <clinckx.louis@gmail.com> wrote:
>These drivers' id_tables only match HID_USB_DEVICE() entries and the
>code freely assumes a USB transport (to_usb_interface() on hdev->dev.parent,
>endpoint access in raw_event). Make the assumption explicit at probe and
>drop a NULL check that the compiler-level definition of to_usb_interface()
>makes unreachable.
>
>No functional change for the supported (USB) devices.
>
>Louis Clinckx (2):
>  HID: lenovo-go: reject non-USB transports in probe
>  HID: lenovo-go: drop dead NULL check on to_usb_interface()
>
> drivers/hid/hid-lenovo-go-s.c | 11 ++++++-----
> drivers/hid/hid-lenovo-go.c   |  6 +++---
> 2 files changed, 9 insertions(+), 8 deletions(-)
>
>---
>Changes since v1:
> - Resubmitted on linux-input as a quality fix per Benjamin's review of v1
>   (sent to security@kernel.org; the HID_USB_DEVICE() id_table already
>    filters BUS_USB, so no exploitable path).
> - Added patch 2 to remove the dead NULL check, at Derek's suggestion.
>
>base-commit: 50897c955902c93ae71c38698abb910525ebdc89
>--
>2.39.5
>

Thanks for the quick turnaround on this. I've already tested these changes, so for the series:
Reviewed-by: Derek J. Clark <derekjoh.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>

Thanks,
Derek