Re: [PATCH v3 7/7] HID: hid-lenovo-go: clear calibration status on start

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Lockless modification of calibration status causes a logical race condition and potential overwrite.

Pre-existing issues:
- [Critical] The driver uses statically allocated global structures (`drvdata` and `go_cdev_rgb`) to store device state and subclass registration, inherently breaking multi-device support and causing memory corruption.
- [High] Missing `hid_hw_stop()` and `hid_hw_close()` on the error path of `hid_go_probe()` leads to hardware state leakage and use-after-free vulnerabilities.
- [High] `hid_go_remove` executes an early return without calling `hid_hw_stop()`, violating unbind safety rules.
- [High] LED `brightness_set` callback performs sleeping operations, violating non-blocking requirements.
- [High] ABBA deadlock between `hid_go_cfg_remove()` and sysfs store/show operations.
- [High] Devm-managed LED sysfs attributes race with `hid_hw_stop()` during device removal (Devres LIFO Trap).
--

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

    HID: hid-lenovo-go: clear calibration status on start
    
    A completed calibration leaves success or failure in its status file. If
    the next calibration has the same result, userspace sees no change and
    cannot tell when the new attempt starts or finishes.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index a6dec5490d179..749536e19d2d2 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

> @@ -1422,6 +1428,9 @@ static ssize_t calibrate_config_store(struct device *dev,
>  		return -EINVAL;
>  
>  	val = ret;
> +	if (val == CAL_START)
> +		*cal_status = CAL_STAT_UNKNOWN;

[Severity: Medium]
Since this locklessly sets the calibration status to UNKNOWN in process
context, can an incoming raw HID event in interrupt context concurrently
trigger hid_go_device_status_event() and overwrite the same location?

If a status report is slightly delayed, could it overwrite the UNKNOWN state
before the new command completes?

[ ... ]

> @@ -1978,17 +1988,23 @@ 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 = { SET_TRIGGER_CFG };
> +static struct go_cal_cfg_attr cal_trigg_left = {
> +	SET_TRIGGER_CFG, &drvdata.gp_left_trigg_cal_status
> +};

[Severity: Critical]
This is a pre-existing issue, but does using a statically allocated global
structure (drvdata and go_cdev_rgb) inherently break multi-device support?

If a second compatible Lenovo Legion Go device is plugged in, won't
hid_go_cfg_probe() unconditionally overwrite the global drvdata pointer and
attempt to re-register the same static go_cdev_rgb struct into the LED
subsystem using devm, causing memory and linked list corruption?


While reviewing this code, I noticed a few pre-existing issues in functions not
modified by this patch:

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

[Severity: High]
This is a pre-existing issue, but does this error path propagate the failure
from hid_go_cfg_probe() without tearing down the started HID hardware?

If hid_go_cfg_probe() fails, the function propagates the error code directly.
Because hid_hw_start() previously succeeded, failing to call hid_hw_stop()
and hid_hw_close() leaves the hardware active and input nodes registered with
the core while the driver's devres context is freed, leading to a resource
leak and use-after-free.

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

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

[Severity: High]
This is a pre-existing issue, but does this early return skip the teardown
logic for the device, leaking the hardware state?

If get_endpoint_address() fails during device removal, it executes an early
return. However, the driver allows fallback initialization as a generic HID
device during probe even if the endpoint is not found, meaning hid_hw_start()
was called. Skipping hid_hw_stop() leaves the hardware and input nodes running
after unbind.

drivers/hid/hid-lenovo-go.c:go_cdev_rgb {
    ...
	.led_cdev = {
		.name = "go:rgb:joystick_rings",
		.color = LED_COLOR_ID_RGB,
		.brightness = 0x50,
		.max_brightness = 0x64,
		.brightness_set = hid_go_brightness_set,
	},
    ...
}

[Severity: High]
This is a pre-existing issue, but does this assign a sleeping function to a
callback that must remain non-blocking?

The LED core invokes .brightness_set from atomic contexts (e.g., holding
spinlocks or from a softirq). The assigned callback invokes
mcu_property_out(), which acquires a mutex and calls
wait_for_completion_interruptible_timeout(). Should it use
brightness_set_blocking instead to avoid a kernel panic?

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);
    ...
}

[Severity: High]
This is a pre-existing issue, but does holding the driver's main mutex while
calling sysfs_remove_groups() introduce an ABBA deadlock condition?

During device unbind, hid_go_cfg_remove() acquires drvdata.cfg_mutex and then
calls sysfs_remove_groups(), which blocks waiting for any active sysfs file
operations to finish. Concurrently, a sysfs operation acquires the sysfs
active reference and calls mcu_property_out(), which blocks waiting for
drvdata.cfg_mutex.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);
    ...
}

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

[Severity: High]
This is a pre-existing issue, but do the devm-managed LED sysfs attributes
race with hid_hw_stop() during device removal?

The driver registers the LED via devm_led_classdev_multicolor_register(),
meaning it gets unregistered strictly after the custom remove() callback
completes. However, hid_go_cfg_remove() manually executes hid_hw_stop().

This creates a window where the LED sysfs interfaces remain accessible to
userspace while the underlying hardware endpoints have been torn down. A
concurrent sysfs write could trigger an output report to a stopped hardware
device, causing a use-after-free.

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