HID: hid-lenovo-go: fix controller configuration

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

Pre-existing issues:
- [Critical] Early return in remove() skips hid_hw_stop(), leading to Use-After-Free.
- [Critical] Singleton global state prevents multi-device support and causes data corruption.
- [High] Missing hid_hw_stop() and hid_hw_close() on probe error path.
- [High] Sysfs attribute groups are leaked on probe error paths.
- [High] ABBA deadlock between sysfs active references and cfg_mutex during removal. --

commit 087e7a307d94a2ccdf00bf5d59ce9cddc609d002
Author: Aditya Dash <[email protected]>

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

The calibration attributes pass the calibration operation as the command and the configuration command as the sub-command. This sends each request with the two fields reversed.

Swap the command and sub-command values for all six calibration attributes. For example, the malformed left joystick Start request is:

05 00 04 0c 03 01 ...

The corrected request is:

05 00 0c 04 03 01 ...

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_probe() leak hardware resources?

If hid_go_cfg_probe() fails, the function logs the error and returns the negative error code directly.

Does this bypass the mandatory hid_hw_close() and hid_hw_stop() teardown calls, leaving the device in an active state despite a failed probe?

[Severity: Critical]
This isn't a bug introduced by this patch, but does this early return in hid_go_remove() skip driver teardown steps?

If a malformed USB device without endpoints is probed, get_endpoint_address() returns an error. If hid_go_probe() successfully binds the device as a generic HID device, then during driver unbind, get_endpoint_address() again returns an error.

Does this cause hid_go_remove() to hit the early return and exit immediately, skipping hid_hw_stop() and leaving dangling input interfaces active while devres releases the driver's underlying memory?

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_cfg_probe() leak sysfs attributes?

If a subsequent initialization step like devm_led_classdev_multicolor_register() fails, the function returns immediately without calling sysfs_remove_groups().

Could accessing these dangling sysfs nodes after module unload result in a Use-After-Free panic?

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but does this static global structure prevent multi-device support and corrupt state on concurrent probes?

The driver stores all per-device state, including the hdev pointer and cfg_mutex, in a single static global struct named drvdata.

If a second Lenovo Legion Go device is plugged in, its probe sequence in hid_go_cfg_probe() overwrites the global hdev pointer and calls mutex_init(&drvdata.cfg_mutex) on an active lock.

Does this lead to memory corruption and unpredictable driver behavior for all connected devices?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this create an ABBA deadlock between sysfs active references and cfg_mutex?

hid_go_cfg_remove() first acquires cfg_mutex and then calls sysfs_remove_groups(), which internally blocks until all active sysfs read and write operations complete.

Concurrently, a sysfs read first takes the kernfs active reference and then calls mcu_property_out(), which attempts to lock cfg_mutex.

Does this establish a circular locking dependency that freezes the driver unbind process and indefinitely hangs any tasks attempting to interact with the device's sysfs files?

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

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

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in probe error paths.
- [High] Missing hardware cleanup in remove() due to early return.
- [High] Global singleton used for per-device state, causing corruption and Use-After-Free with multiple devices.
- [High] Leaked sysfs attribute groups on probe failure.
- [High] Deadlock during device unbind due to lock ordering with sysfs_remove_groups.
--

commit c2d6b2210c5ca6d1fa356349f3cbde71b00ce595
Author: Aditya Dash <[email protected]>
Subject: HID: hid-lenovo-go: use the right controller selectors

This commit fixes the attribute selectors for the right controller on Lenovo Go gamepads. It updates the imu_enabled_right attribute to select FEATURE_IMU_ENABLE and ensures the reset_right attribute targets the right controller.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does using a global singleton for instance- specific state break the driver model if multiple devices are plugged in?

Since drvdata is globally allocated in drivers/hid/hid-lenovo-go.c, setting drvdata.hdev here in hid_go_cfg_probe() will clobber the pointer from any previously probed device.

[Severity: High]
This isn't a bug introduced by this patch, but does this corrupt the LED subsystem lists?

Since go_cdev_rgb is a statically allocated global struct, registering the same object in hid_go_cfg_probe() for every device instance could corrupt the lists if multiple devices are present.

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_cfg_probe() leave sysfs attributes behind?

If devm_led_classdev_multicolor_register() fails, this returns immediately without calling sysfs_remove_groups(). While devres will free the driver's memory, the sysfs attributes remain active and reference freed memory, which could cause a use-after-free if userspace interacts with them.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can this lock ordering in hid_go_cfg_remove() cause a deadlock during device unbind?

This holds drvdata.cfg_mutex while synchronously waiting for sysfs_remove_groups() to finish. If userspace is concurrently writing to a sysfs attribute, that write path (e.g., mcu_property_out()) will attempt to acquire drvdata.cfg_mutex, leading to an ABBA deadlock that freezes the kernel thread.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this error path in hid_go_probe() fail to tear down the hardware?

If hid_go_cfg_probe() fails, the error code is immediately returned without calling hid_hw_stop() or hid_hw_close(). The hardware is left running while driver private data is freed by devres, which will trigger a use-after-free when subsequent input events arrive.

[Severity: High]
This isn't a bug introduced by this patch, but does this early return in hid_go_remove() skip mandatory hardware cleanup?

If get_endpoint_address() fails during device unplug, it hits this early return and bypasses hid_hw_close() and hid_hw_stop(). Input interrupts will keep firing into freed driver structures.

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

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

New issues:
- [High] The newly added spinlock and completion are accessed by the raw event handler before they are initialized, causing a potential kernel crash.

Pre-existing issues:
- [High] Missing cleanup on probe failure leaves the hardware started, the device open, and sysfs groups leaked.
- [High] Missing hardware stop in remove for devices with no endpoints.
- [High] Global static drvdata structure causes state corruption and kernel panics when multiple devices are connected.
- [High] ABBA deadlock between device removal and sysfs attribute stores.
- [High] Possible out-of-bounds array access in get_endpoint_address if USB interface has no endpoints. --

commit 0d711ce676d4cb403114e39243d7e14be625eec6
Author: Aditya Dash <[email protected]>
Subject: HID: hid-lenovo-go: return configuration request errors

This commit adds a mechanism to track pending command state and correctly match configuration replies by command ID, sub-command, and device. It ensures that unrelated replies, errors, timeouts, or interrupted waits are properly reported as errors to the caller rather than silently returning success.

[ ... ]

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

    int ep = get_endpoint_address(hdev);

    if (ep <= 0)
        return;

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

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

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 <[email protected]>

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.

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

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

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

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

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

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

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

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

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

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

New issues:
- [Low] The patch leaves behind dead code (if (!val) size = 0;) after explicitly rejecting CAL_UNKNOWN.

Pre-existing issues:
- [Critical] The driver incorrectly uses static global structs to store per-device state.
- [High] Missing hid_hw_close() and hid_hw_stop() in hid_go_probe() error paths.
- [High] Early return in hid_go_remove() bypasses hid_hw_stop() cleanup.
- [High] Missing sysfs_remove_groups() on the error paths in hid_go_cfg_probe().
- [High] AB-BA deadlock in hid_go_cfg_remove() with sysfs operations.
- [High] LED brightness_set callback performs sleeping operations.
--

commit a78ed68c3272ef95327340abb6446f933e2d3711
Author: Aditya Dash <[email protected]>
HID: hid-lenovo-go: reject unknown calibration action

This patch explicitly rejects CAL_UNKNOWN in the store handler for the calibration configuration. It prevents requests without an action byte from being sent by returning -EINVAL before building the request.

[Severity: Low]
Does this change leave behind dead code? Since CAL_UNKNOWN maps to 0 and is now explicitly rejected above, the subsequent assignment val = ret guarantees that val is non-zero.

Because of this, if (!val) will evaluate to false under all possible conditions, making size = 0 dead code.

[ ... ]

[Severity: High]
This isn't a regression introduced by this patch, but can this error path lead to a resource leak and potential use-after-free?

In hid_go_probe(), if hid_go_cfg_probe() returns an error, the function immediately returns the error without rolling back prior successful calls to hid_hw_start() and hid_hw_open(). This leaves the device in an active hardware state with input interfaces registered, violating HID subsystem lifecycle rules.

[ ... ]

[Severity: High]
This isn't a regression introduced by this patch, but does this early return bypass the default case needed to clean up generic HID devices?

If a device without the expected endpoint is unbound, get_endpoint_address() returns an error (< 0) and this early return in hid_go_remove() triggers. Omitting hid_hw_stop() here leaves the HID hardware, input devices, and hidraw nodes active and registered after the driver has unbound.

When userspace interacts with these lingering nodes, it will access freed devres data, causing a use-after-free.

[ ... ]

[ ... ]

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does this incorrectly use static global structs to store per-device state?

If multiple supported devices are plugged in simultaneously, hid_go_cfg_probe() is called multiple times and unconditionally re-initializes the global drvdata.cfg_mutex, drvdata.cmd.done, and drvdata.cmd.lock, and overwrites drvdata.hdev.

Re-initializing active synchronization primitives corrupts lockdep and waitqueues, leading to kernel panics. Additionally, registering the same static global led_classdev_mc multiple times corrupts the LED subsystem's linked lists, and sharing drvdata.hdev means one device's interrupts or sysfs operations will randomly operate on the other device.

[ ... ]

[Severity: High]
This isn't a regression introduced by this patch, but does this error path permanently leak sysfs attributes?

Earlier in hid_go_cfg_probe(), sysfs_create_groups() is called to create attributes. If devm_led_classdev_multicolor_register() fails here, the function returns the error directly without calling sysfs_remove_groups().

This leaves orphaned sysfs files on the device's kobject. If accessed, they will reference uninitialized or invalid driver state, and reloading or re-binding the driver will fail because the files already exist.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can this cause an AB-BA deadlock?

In hid_go_cfg_remove(), the driver acquires drvdata.cfg_mutex and then calls sysfs_remove_groups(), which blocks waiting for the kernfs active reference to drain.

Concurrently, any sysfs read/write handler (like feature_status_show()) first acquires the kernfs active reference, and then calls mcu_property_out(), which blocks trying to acquire drvdata.cfg_mutex. This forms an AB-BA deadlock that will hang driver removal and userspace sysfs access.

[ ... ]

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this callback perform sleeping operations in an atomic context?

The driver assigns hid_go_brightness_set to the .brightness_set field. The LED subsystem invokes this callback directly from atomic contexts, such as timer interrupts for the timer trigger.

However, hid_go_brightness_set() calls mcu_property_out(), which acquires a mutex, allocates memory with GFP_KERNEL, and sleeps on a completion timeout. Triggering this callback from an atomic context will cause a panic. The driver must use .brightness_set_blocking instead.

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

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

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in hid_go_probe() error paths.
- [High] Missing hid_hw_stop() in hid_go_remove() on early return.
- [High] Sysfs attribute groups are leaked if hid_go_cfg_probe() fails midway.
- [High] Singleton driver state (drvdata) causes state corruption, lock re-initialization, and UAF when multiple devices are probed.
- [High] Deadlock in driver removal path between sysfs_remove_groups and mcu_property_out.
- [High] UAF vulnerability due to devm-managed LED class device unregistration ordering.
--

commit 6b546e272fefac54ad5bf113ee4d2d91e487fb03
Author: Aditya Dash <[email protected]>

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.

Link each calibration action attribute to its status field. Set only that field to unknown before submitting Start. Stop leaves the status unchanged, and the next firmware result replaces unknown with success or failure.

[ ... ]

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 isn't a bug introduced by this patch, but does this error path cause a use-after-free?

If hid_go_cfg_probe() fails, hid_go_probe() returns the error code without cleaning up. By omitting hid_hw_close() and hid_hw_stop(), the input and hidraw interfaces remain registered and active while the driver binding fails and devres resources are freed. Userspace interaction can then trigger a UAF.

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 can this early return cause a use-after-free if a malicious device with no endpoints is unplugged?

When a device is unplugged or unbound and get_endpoint_address() returns -ENODEV, this early return completely bypasses the hid_hw_stop() call, leaving input interfaces registered while driver devres resources are freed.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
	if (ret) {
		dev_err_probe(&hdev->dev, ret,
			      "Failed to create gamepad configuration attributes\n");
		return 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;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but do the sysfs attribute groups get leaked here?

If devm_led_classdev_multicolor_register() fails, the manually created sysfs groups are never removed because sysfs_remove_groups() is missing in the error path. This leaves dangling sysfs files that can be accessed by userspace after the driver fails to bind.

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

[Severity: High]
This isn't a bug introduced by this patch, but does the use of a single global static drvdata struct cause state corruption if multiple physical devices are connected?

Probing a second device unconditionally overwrites drvdata.hdev and re-initializes cfg_mutex and cmd.lock while they might be held, which could lead to deadlocks or a use-after-free.

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 could this lock ordering cause an AB-BA deadlock during device removal?

hid_go_cfg_remove() acquires drvdata.cfg_mutex and calls sysfs_remove_groups(), which waits to drain active kernfs references. Meanwhile, an active sysfs callback holds a kernfs reference and attempts to acquire drvdata.cfg_mutex inside mcu_property_out(), which would cause a deadlock.

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

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

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free window here during device removal?

Devres cleans up resources (unregistering the LED class device) after the driver's remove() callback returns. Inside hid_go_cfg_remove(), hid_hw_stop() is called.

During the window before devres runs, userspace can still invoke hid_go_brightness_set(), which accesses drvdata.hdev and calls hid_hw_output_report() on a stopped hardware device.

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