Re: [PATCH v2 4/6] HID: hid-lenovo-go: normalize calibration failure status

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The drivers use static global variables for per-device state and LED registration, preventing support for multiple devices and causing state corruption.
- [High] Missing `hid_hw_stop()` and `hid_hw_close()` in probe error paths.
- [High] LED devres unregistration happens after `hid_hw_stop()`, leaving a window for illegal hardware access.
- [High] The LED `brightness_set` callback illegally sleeps.
- [High] Hardware stop leaked during device removal if endpoint address retrieval fails.
--

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

HID: hid-lenovo-go: normalize calibration failure status

The driver stores the firmware result byte as an index into the
calibration status text. A Legion Go 1 returned 0x08 after an idle Stop.
The status table has only three entries, so a later read returns -EINVAL.

Keep the defined values 0x00 through 0x02. Treat larger result values as
failure before storing them.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>

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

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak
the HID hardware state?

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_go_cfg_probe() fails, the function returns an error without unwinding
the started hardware with hid_hw_stop() and hid_hw_close(). Since the custom
remove callback overrides the core's default unbind behavior, could this lead
to a use-after-free?

The hid-lenovo-go-s driver has the identical omission in its nested probe
error path:

drivers/hid/hid-lenovo-go-s.c:hid_gos_probe() {
	...
	ret = hid_gos_cfg_probe(hdev, id);
	if (ret)
		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface");
	...
}

[Severity: Critical]
This is a pre-existing issue, but does using a static global variable for
per-device state prevent support for multiple devices?

drivers/hid/hid-lenovo-go.c:
static struct hid_go_cfg {
	struct delayed_work go_cfg_setup;
	struct hid_go_cmd cmd;
...

If a second Lenovo Go controller is connected, the second probe overwrites
the global drvdata.hdev pointer in hid_go_cfg_probe():

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
	...
	mutex_init(&drvdata.cfg_mutex);
	init_completion(&drvdata.cmd.done);
	spin_lock_init(&drvdata.cmd.lock);
	drvdata.cmd.pending = false;
	hid_set_drvdata(hdev, &drvdata);
	drvdata.hdev = hdev;
	...
	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
	...
}

Does registering the exact same static go_cdev_rgb struct with devres
corrupt the LED core lists and crash the kernel?

[Severity: High]
This is a pre-existing issue, but does manually stopping the hardware here
leave a window for illegal hardware access?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
	...
	hid_hw_close(hdev);
	hid_hw_stop(hdev);
	hid_set_drvdata(hdev, NULL);
	...
}

Because the LED is cleaned up asynchronously by devres after remove()
returns, could a user writing to the LED brightness sysfs file in this
window invoke hid_go_brightness_set(), sending an output report to a
stopped HID device using a dangling drvdata.hdev pointer?

[Severity: High]
This is a pre-existing issue, but does the LED brightness_set callback
illegally sleep?

drivers/hid/hid-lenovo-go.c:hid_go_brightness_set() {
	...
	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
	...
}

The brightness_set LED callback is expected to be non-blocking, but it calls
rgb_cfg_call() which waits on a completion. If the LED is controlled by a
timer trigger, could this callback execute in a softirq/timer context and
cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but does this early return leak the hardware
start state?

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

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

If a USB HID interface with no endpoints successfully passes probe() by
bypassing the config probe, does returning early here during removal entirely
bypass hid_hw_close() and hid_hw_stop(), leading to unbalanced HID core
reference counts?

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