Re: [PATCH v3 2/7] HID: hid-lenovo-go: use the right controller selectors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver relies on static global structures (`drvdata`, `go_cdev_rgb`) to maintain per-device state and register LED devices, breaking multi-device support and causing memory corruption.
- [High] Hardware is left in an active, open state if `hid_go_cfg_probe()` fails, due to missing `hid_hw_stop()` and `hid_hw_close()` calls in the `hid_go_probe()` error path.
- [High] Hardware cleanup and workcancellation are bypassed in `hid_go_remove()` for generic endpoints, leaving unmanaged active hardware and risking use-after-free.
- [High] Out-of-bounds array read in `get_endpoint_address()` if an interface has no endpoints.
--

commit e72b397410199e2e2416cfeb7ba8e9af743302f7
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: use the right controller selectors

This commit corrects the right controller selectors by changing the
imu_enabled attribute to select FEATURE_IMU_ENABLE. It also fixes the right
reset attribute to target the right controller instead of the left.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..b937d11dd6f6a 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does relying on static global structures
for per-device state break multi-device support and cause memory corruption?

Looking at drivers/hid/hid-lenovo-go.c, the driver defines global structures
rather than allocating per-device structures with devm_kzalloc():

} drvdata;
...
static struct led_classdev_mc go_cdev_rgb = {

And in hid_go_cfg_probe(), it forces all probed instances to share this
state by overwriting drvdata.hdev:

	hid_set_drvdata(hdev, &drvdata);
	drvdata.hdev = hdev;
	mutex_init(&drvdata.cfg_mutex);

If multiple gamepads are plugged in, will concurrent probes overwrite active
locks and the delayed work struct?

Also, will calling devm_led_classdev_multicolor_register() on the same
global go_cdev_rgb multiple times double-add its embedded list_head node
into the core LED subsystem lists, causing linked-list corruption?

[Severity: High]
This isn't a bug introduced by this patch, but does hid_go_probe() leak
the hardware state if hid_go_cfg_probe() fails?

In hid_go_probe(), after hid_hw_start() and hid_hw_open() are successfully
called, if hid_go_cfg_probe() fails, the error path returns without calling
hid_hw_close() and hid_hw_stop():

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

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

	return ret;

Could this leave the device unmanaged but actively running, causing resource
leaks and potential use-after-free when devres cleanup frees structures?

[Severity: High]
This is a pre-existing issue, but is hardware cleanup bypassed in
hid_go_remove() for generic endpoints?

In hid_go_remove(), the driver unconditionally returns early for
non-configured generic endpoints:

	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;

Since hid_go_probe() accepts devices even if get_endpoint_address() returns
an error, leaving the generic device bound, started, and opened, does this
early return in hid_go_remove() bypass hid_hw_close() and hid_hw_stop()
entirely?

According to HID subsystem rules, omitting hid_hw_stop() in remove() leaves
the input and hidraw devices registered and active while the driver's devres
memory is freed. Can this lead to a use-after-free if userspace interacts
with the node?

[Severity: High]
This isn't a bug introduced by this patch, but can get_endpoint_address()
cause an out-of-bounds array read if an interface has no endpoints?

The function fetches the endpoint array and checks if ep is NULL, but it
dereferences it without checking if desc.bNumEndpoints > 0:

	ep = intf->cur_altsetting->endpoint;
	if (!ep)
		return -ENODEV;

	return ep->desc.bEndpointAddress;

If the interface has bNumEndpoints == 0, the USB core allocates a
zero-length array, returning ZERO_SIZE_PTR.

Since if (!ep) evaluates to false for ZERO_SIZE_PTR, will dereferencing it
cause an out-of-bounds read?

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