Re: [PATCH 1/5] HID: hid-lenovo-go: use the correct calibration commands

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

Pre-existing issues:
- [High] The driver uses static global structures for device state (`drvdata`) and LED device registration, breaking support for multiple devices.
- [High] Missing `hid_hw_stop()` and `hid_hw_close()` on the `hid_go_probe()` error path.
- [High] Early return in `hid_go_remove()` skips hardware cleanup for interfaces without valid endpoints.
- [High] AB-BA deadlock between driver mutex and kernfs active protection during device removal.
- [High] The LED class device is managed by devres but the hardware is manually stopped in `remove()`, creating a use-after-free window.
- [High] `get_endpoint_address()` unconditionally reads endpoint descriptors without verifying the number of endpoints.
- [Medium] Sysfs attribute groups leak on error paths in `hid_go_cfg_probe()`.
- [Low] Unused memory allocation in `hid_go_cfg_probe()`.
--

--- Patch [1]: [PATCH 1/5] HID: hid-lenovo-go: use the correct calibration commands ---
commit 0cbce56e5fe2f7bb5d9b0dab89495451652240da
Author: Aditya Dash <[email protected]>

HID: hid-lenovo-go: use the correct calibration commands

This commit fixes malformed calibration requests in the hid-lenovo-go driver by
swapping the command and sub-command values for all six calibration attributes.
It reverses the previously incorrect ordering of the calibration operation and
configuration command.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 318b1152ff8ba..b8248bf8965c6 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1911,18 +1911,18 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
>  static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
>  			    "rumble_notification_index");
>  
> -static struct go_cfg_attr cal_trigg_left = { TRIGGER_CALIBRATE };
> -LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", SET_TRIGGER_CFG,
> +static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
> +LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", TRIGGER_CALIBRATE,
>  		     LEFT_CONTROLLER, index);
>  static DEVICE_ATTR_RO_NAMED(cal_trigg_left_index, "calibrate_trigger_index");

[Severity: High]
This is a pre-existing issue, but can the global static structure drvdata cause
data corruption and a use-after-free if multiple devices are connected?

drivers/hid/hid-lenovo-go.c:
	static struct hid_go_cfg {
	[ ... ]
	} drvdata;

In hid_go_cfg_probe(), the global state is unconditionally overwritten:

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

If multiple Lenovo Go controllers are plugged in, or if the device exposes
multiple interfaces that trigger probes, this will overwrite the global
drvdata pointer and attempt to re-register the statically allocated
go_cdev_rgb, leading to corrupted internal state and kernel panic during LED
registration.

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
hid_go_probe() leak resources?

drivers/hid/hid-lenovo-go.c:hid_go_probe() {
	[ ... ]
	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;
}

If hid_hw_start() and hid_hw_open() succeed but hid_go_cfg_probe() fails,
the driver returns the error immediately without calling hid_hw_stop() and
hid_hw_close(). This leaves the hardware in an active, undefined state.

[Severity: High]
This is a pre-existing issue, but does hid_go_remove() skip hardware cleanup
for interfaces without valid endpoints?

drivers/hid/hid-lenovo-go.c:hid_go_remove() {
	int ep = get_endpoint_address(hdev);

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

If a device successfully probes as a generic HID device without specific
endpoints, get_endpoint_address can return an error like -ENODEV. The early
return completely bypasses hid_hw_close() and hid_hw_stop(), leaking hardware
state.

[Severity: Medium]
This isn't a bug introduced by this patch, but does hid_go_cfg_probe() leak
sysfs attributes on error paths?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
	[ ... ]
	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
	[ ... ]
	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
	if (ret) {
		dev_err_probe(&hdev->dev, ret, "Failed to create RGB device\n");
		return ret;
	}
	[ ... ]
}

sysfs_create_groups() is not managed by devres. If subsequent operations
like devm_led_classdev_multicolor_register() fail, the function returns an
error without calling sysfs_remove_groups().

[Severity: High]
This is a pre-existing issue, but can hid_go_cfg_remove() cause an AB-BA
deadlock with sysfs operations?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
	[ ... ]
	guard(mutex)(&drvdata.cfg_mutex);
	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
	[ ... ]
}

The removal function acquires drvdata.cfg_mutex and then blocks in
sysfs_remove_groups() waiting for active sysfs callbacks to finish.
Simultaneously, sysfs callbacks like feature_status_show hold the kernfs
active protection and block attempting to acquire drvdata.cfg_mutex.

[Severity: Low]
This isn't a bug introduced by this patch, but is the buf memory allocation
in hid_go_cfg_probe() unused?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
	unsigned char *buf;
	int ret;

	buf = devm_kzalloc(&hdev->dev, GO_PACKET_SIZE, GFP_KERNEL);
	if (!buf)
		return -ENOMEM;
	[ ... ]
}

The buffer buf is statically allocated but never used or passed to any
function.

[Severity: High]
This is a pre-existing issue, but does mixing devres and manual teardown in
hid_go_cfg_remove() create a use-after-free window?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
	[ ... ]
	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
	hid_hw_close(hdev);
	hid_hw_stop(hdev);
	hid_set_drvdata(hdev, NULL);
}

The LED is registered with devm_led_classdev_multicolor_register. In
hid_go_cfg_remove(), the driver manually calls hid_hw_stop(). Since devres
cleans up the LED after the custom remove() callback completes, there is a
race window where the LED is exposed in sysfs but the underlying HID hardware
is stopped. A sysfs write to the LED brightness during this window will attempt
I/O on stopped hardware.

[Severity: High]
This isn't a bug introduced by this patch, but does get_endpoint_address()
perform an out-of-bounds memory read if bNumEndpoints is 0?

drivers/hid/hid-lenovo-go.c:get_endpoint_address() {
	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
	struct usb_host_endpoint *ep;

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

	return ep->desc.bEndpointAddress;
}

If a malicious USB device declares an interface with bNumEndpoints == 0, the
endpoint array has zero elements. However, the pointer may not be strictly
NULL but point to the adjacent memory block, bypassing the !ep check and
reading ep->desc.bEndpointAddress out of bounds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1