HID: hid-lenovo-go: fix controller configuration

drivers/hid/hid-lenovo-go.c LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification", · line 1916

[ ... ]

While reviewing this code, I noticed a few pre-existing issues elsewhere in the driver.

drivers/hid/hid-lenovo-go.c LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification", · line 1927

        ^^^^

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a malicious USB device to provide an interface with zero endpoints?

If so, this code reads ep->desc.bEndpointAddress without verifying the intf->cur_altsetting->desc.bNumEndpoints count. Does this need a check to prevent an out-of-bounds read of the endpoint array?

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/[email protected]?part=2

drivers/hid/hid-lenovo-go.c static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id) · line 2440

[Severity: High]
This isn't a bug introduced by this patch, but does blindly calling init_completion() here destroy the active waitqueue if a thread is already waiting from a concurrent device?

The global drvdata structure is overwritten when multiple devices bind, causing all sysfs and workqueue operations to operate on the last connected device's hdev. Registering the same static go_cdev_rgb multiple times corrupts the LED core linked lists.

drivers/hid/hid-lenovo-go.c static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id) · line 2456

[Severity: High]
This is a pre-existing issue, but if hid_go_cfg_probe() fails right after this check, does the driver return an error code from probe() without calling hid_hw_stop()?

The HID core will free driver devres resources, but the hardware is left active, violating HID lifecycle guidelines and leading to potential use-after-free.

Also, if devm_led_classdev_multicolor_register() fails in hid_go_cfg_probe(), does it return without removing the previously created sysfs groups, leaking them?

[ ... ]

A few other pre-existing issues were noticed in this driver:

[Severity: High]
This isn't a bug introduced by this patch, but does the driver have an AB-BA deadlock between cfg_mutex and kernfs active protection during device removal?

In hid_go_cfg_remove():

	guard(mutex)(&drvdata.cfg_mutex);
	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

The mutex is acquired before calling sysfs_remove_groups, initiating the deadlock condition. Meanwhile, a sysfs writer in mcu_property_out() attempts to acquire the mutex while holding the sysfs active refcount, completing the deadlock cycle.

[Severity: High]
This is a pre-existing issue, but does hid_go_remove() bypass the required hid_hw_close() and hid_hw_stop() cleanup phase for invalid endpoints?

In hid_go_remove():

	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;

If get_endpoint_address() fails in hid_go_probe(), the driver still starts the hardware and returns 0. During unbind, the early return in hid_go_remove() bypasses the cleanup, leaving the transport layer active and unstopped, causing an immediate use-after-free vulnerability.

[Severity: High]
This isn't a bug introduced by this patch, but does the devres LIFO ordering trap leave the LED classdev active after the hardware is stopped?

In hid_go_cfg_remove():

	hid_hw_close(hdev);
	hid_hw_stop(hdev);

This manually stops the hardware before devres unregisters the LED. Userspace interacting with the LED brightness via hid_go_brightness_set() can trigger a hardware report which assumes the device is still active, leaving a window for a use-after-free.

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

drivers/hid/hid-lenovo-go.c static int hid_go_cfg_probe(struct hid_device *hdev, · line 2367

[Severity: High]
This isn't a bug introduced by this patch, but does the use of cfg_mutex create an AB-BA deadlock between hid_go_cfg_remove() and sysfs attributes?

hid_go_cfg_remove() acquires drvdata.cfg_mutex and then calls sysfs_remove_groups(), which blocks until all active sysfs callbacks drain via the kernfs active reference.

Concurrently, if a sysfs write occurs, it acquires an active reference to the sysfs node and then calls mcu_property_out(), which waits to acquire drvdata.cfg_mutex. This forms an unresolvable circular dependency:
cfg_mutex -> kernfs ref vs kernfs ref -> cfg_mutex.

[ ... ]

drivers/hid/hid-lenovo-go.c static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id) · line 2506

[Severity: High]
This is a pre-existing issue, but does using a static global drvdata structure for per-device state cause state corruption across multiple devices?

If a user connects multiple Lenovo Go gamepads, or the device exposes multiple interfaces that match the driver's IDs and have the 0x83 endpoint, the probe function unconditionally executes hid_set_drvdata(hdev, &drvdata) and drvdata.hdev = hdev, overwriting the global pointer.

Furthermore, does unconditionally re-initializing drvdata.cmd.lock and drvdata.cmd.done during hid_go_probe() silently destroy the active lock and waitqueue if another device is currently processing a command, leading to list corruption and kernel panics?

drivers/hid/hid-lenovo-go.c static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id) · line 2509

[Severity: High]
This is a pre-existing issue, but does hid_go_probe() fail to clean up hardware state on error paths?

If hid_go_cfg_probe() fails (e.g., LED registration failure), it returns an error. hid_go_probe() propagates this error directly without calling hid_hw_stop() and hid_hw_close() to undo the previously successful hid_hw_start() and hid_hw_open().

Additionally, error paths within hid_go_cfg_probe() return without calling sysfs_remove_groups() to clean up previously created sysfs groups. While the devres group is released, the active hardware and leaked sysfs nodes will still invoke callbacks that dereference the freed memory.

[Severity: High]
This is a pre-existing issue, but does hid_go_remove() fail to stop the hardware if the device has no valid endpoints?

During hid_go_probe(), if the interface has no valid endpoints, get_endpoint_address() returns an error. The driver still successfully starts the device via hid_hw_start() and returns 0 because it falls back to a generic HID device path.

When the device is later unbound, hid_go_remove() checks get_endpoint_address(hdev) again, sees it is <= 0, and immediately returns without calling hid_hw_close() or hid_hw_stop(). The devres group is freed, but the device remains active, causing a use-after-free on subsequent interactions.

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

drivers/hid/hid-lenovo-go.c static int hid_go_device_status_event(struct command_report *cmd_rep) · line 684

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a TOCTOU out-of-bounds read to happen here?

drivers/hid/hid-lenovo-go.c:hid_go_light_event()

	case USR_LIGHT_PROFILE_3:
		mc_cdev = lcdev_to_mccdev(drvdata.led_cdev);
		drvdata.rgb_effect = cmd_rep->data[0];
		mc_cdev->subled_info[0].intensity = cmd_rep->data[1];

Since hid_go_light_event() overwrites drvdata.rgb_effect locklessly from a hardware event, could rgb_effect_show() read drvdata.rgb_effect out of bounds?

	if (drvdata.rgb_effect >= ARRAY_SIZE(rgb_effect_text))
		return -EINVAL;

	return sysfs_emit(buf, "%s\n", rgb_effect_text[drvdata.rgb_effect]);

Without READ_ONCE(), the compiler could emit two loads for drvdata.rgb_effect. If an interrupt fires and updates the value between the bounds check and the array access, could this result in an out-of-bounds array read?

[Severity: Critical]
This is a pre-existing issue, but does this driver corrupt locks, active completions, and work items when probing multiple devices or interfaces?

drivers/hid/hid-lenovo-go.c

static struct hid_go_cfg {
	struct delayed_work go_cfg_setup;
	struct hid_go_cmd cmd;

...
} drvdata;

When a subsequent interface or device is probed, hid_go_probe() and hid_go_cfg_probe() overwrite the global drvdata.hdev and globally re-initialize the active locks:

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe()

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

drivers/hid/hid-lenovo-go.c:hid_go_probe()

		init_completion(&drvdata.cmd.done);
		spin_lock_init(&drvdata.cmd.lock);

If multiple interfaces bind to the driver concurrently, wouldn't this definitive destruction of active locks and memory structures cause deadlocks, use-after-free, or list corruption panics for the first device?

[Severity: Critical]
This is a pre-existing issue, but does this code bypass hardware teardown and sysfs cleanup during error paths and removal?

In hid_go_probe(), if the configuration interface fails to start, the function returns directly without halting the manually started hardware:

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

	return ret;

Similarly, in hid_go_cfg_probe(), if LED registration fails, the error path fails to remove the sysfs groups created earlier:

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

Additionally, in hid_go_remove(), hardware teardown (hid_hw_stop) is entirely bypassed for endpoints that are not GO_GP_INTF_IN:

	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;

	switch (ep) {
	case GO_GP_INTF_IN:
		hid_go_cfg_remove(hdev);
		break;
	}
}

Will these omissions lead to Use-After-Free vulnerabilities or resource leaks when userspace accesses the remaining orphaned sysfs files or active interfaces?

[Severity: High]
This isn't a bug introduced by this patch, but is there an AB-BA deadlock between cfg_mutex and kernfs node active references?

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

A sysfs store callback (such as feature_status_store) holds a kernfs active reference while attempting to acquire drvdata.cfg_mutex via mcu_property_out().

Concurrently, during device removal, hid_go_cfg_remove() acquires drvdata.cfg_mutex first and then calls sysfs_remove_groups(), which blocks until all kernfs active references drain.

Could this trigger a system deadlock involving sysfs readers/writers and device unbind?

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

The driver assigns hid_go_brightness_set to the non-blocking API:

.brightness_set = hid_go_brightness_set,

However, this synchronous callback invokes sleeping functions:

drivers/hid/hid-lenovo-go.c:mcu_property_out()

	guard(mutex)(&drvdata.cfg_mutex);
...
	wait_result = wait_for_completion_interruptible_timeout(&drvdata.cmd.done,
								timeout);

Since the LED subsystem invokes this callback from atomic context (e.g., software timers for blinking or heartbeat), wouldn't calling mutex_lock() and wait_for_completion_interruptible_timeout() cause a scheduling while atomic kernel panic?

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

drivers/hid/hid-lenovo-go.c static ssize_t calibrate_config_store(struct device *dev, · line 1428

[Severity: High]
This is a pre-existing issue, but does this identical logical vulnerability still exist in feature_status_store() and motor_config_store()?

Both of those functions appear to use sysfs_match_string() with string tables where 'unknown' is index 0. If a user writes 'unknown' to features like imu_enabled, val will be 0, triggering size = 0, and sending a malformed payload missing its action byte to the firmware:

drivers/hid/hid-lenovo-go.c:feature_status_store() {
    ...
        ret = sysfs_match_string(enabled_status_text, buf);
        val = ret;
        break;
    ...
    if (!val)
        size = 0;

    ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA,
                           SET_FEATURE_STATUS, index, device_type, &val,
                           size);
    ...
}

Could the same fix be applied to those functions?

[Severity: Critical]
This isn't a bug introduced by this patch, but does the driver use a static global singleton drvdata structure for per-device context?

drivers/hid/hid-lenovo-go.c:

    ...
    } drvdata;
    ...

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    mutex_init(&drvdata.cfg_mutex);
    hid_set_drvdata(hdev, &drvdata);
    drvdata.hdev = hdev;
    ...
}

If multiple compatible devices are connected, would the active synchronization primitives and workqueue list heads be unconditionally re-initialized, potentially corrupting kernel linked lists and leading to a use-after-free or deadlocks?

[Severity: High]
This is a pre-existing issue, but does the hid_go_probe() error path miss hardware cleanup?

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 directly returns the error code. Does this leave the hardware active since hid_hw_close() or hid_hw_stop() are not called, potentially causing a use-after-free when hardware events trigger on the freed device?

[Severity: High]
This isn't a bug introduced by this patch, but are sysfs attribute groups permanently leaked in the hid_go_cfg_probe() error paths?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
    if (ret) {
    ...
    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;
    }
    ...
}

Since sysfs_create_groups() is not managed by devres, if subsequent steps like devm_led_classdev_multicolor_register() fail, the function returns directly. Since probe() fails, remove() will not be called. Could this leave permanently dangling sysfs nodes tied to the kobject that cause a use-after-free if accessed?

[Severity: High]
This is a pre-existing issue, but does the early return in hid_go_remove() on endpoint lookup failure skip crucial teardown steps?

drivers/hid/hid-lenovo-go.c:hid_go_remove() {
    ...
    if (ep <= 0)
        return;
    ...
}

If the interface has zero endpoints, get_endpoint_address() returns -ENODEV and the early return bypasses all teardown logic. Does this omit hid_hw_close() and hid_hw_stop(), permanently leaking the active device node and creating a use-after-free condition?

[Severity: High]
This isn't a bug introduced by this patch, but does improper workqueue error handling cause a use-after-free if schedule_delayed_work() returns 0?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    ret = schedule_delayed_work(&drvdata.go_cfg_setup, msecs_to_jiffies(2));
    if (!ret) {
        dev_err(&hdev->dev,
            "Failed to schedule startup delayed work\n");
        return -ENODEV;
    }
    ...
}

If schedule_delayed_work() returns 0 (meaning the work was already pending), the driver treats this as a fatal failure and returns -ENODEV. Since probe() fails, the HID core and devres will destroy the device and free hdev. Because the work is still pending and never canceled, when the timer expires, will cfg_setup() execute and dereference the freed drvdata.hdev?

[Severity: High]
This is a pre-existing issue, but can an uninitialized rgb_profile lead to an out-of-bounds firmware command configuration payload?

drivers/hid/hid-lenovo-go.c:rgb_effect_store() {
    ...
    index = drvdata.rgb_profile + 3;
    u8 rgb_profile[6] = { effect,
                  mc_cdev->subled_info[0].intensity,
    ...
    ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
    ...
}

drvdata.rgb_profile is statically initialized to 0. If a user writes to the rgb_effect or rgb_speed sysfs attributes before setting or reading rgb_profile, index evaluates to 3 (corresponding to LIGHT_PROFILE_SEL).

This executes rgb_cfg_call() with a 6-byte payload targeting LIGHT_PROFILE_SEL. If the firmware natively expects LIGHT_PROFILE_SEL to receive a 1-byte payload, could sending a 6-byte profile array cause an out-of-bounds write that crashes or corrupts the device firmware?

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

drivers/hid/hid-lenovo-go.c static ssize_t calibrate_config_store(struct device *dev, · line 1432

[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?

[ ... ]

drivers/hid/hid-lenovo-go.c LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification", · line 1993

[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/[email protected]?part=7