[PATCH 0/5] HID: hid-lenovo-go: fix controller configuration

[PATCH 0/5] HID: hid-lenovo-go: fix controller configuration

[Aditya Dash]

The hid-lenovo-go driver exposes controller configuration attributes, but
several requests use incorrect fields or report the wrong result. Calibration
uses reversed command fields, FPS mode DPI includes an extra device byte,
firmware calibration failure appears as unknown, and two right-controller
attributes use the wrong selectors.

The first four patches correct those declarations and request layouts. The
last patch fixes the shared synchronous request path so that only the matching
reply completes a request and transport, firmware, interruption, and timeout
errors reach the caller. It uses the corrected calibration command pairs from
the first patch.

Calibration completion is asynchronous. The driver now submits those commands
without waiting for an immediate reply and continues to receive final state
through the existing completion report. Firmware does not provide a sequence
number, so a late reply with the same complete command tuple remains
indistinguishable from a newer reply.

Aditya Dash (5):
  HID: hid-lenovo-go: use the correct calibration commands
  HID: hid-lenovo-go: fix the FPS mode DPI request
  HID: hid-lenovo-go: record calibration completion state
  HID: hid-lenovo-go: use the right controller selectors
  HID: hid-lenovo-go: return configuration request errors

 drivers/hid/hid-lenovo-go.c | 165 +++++++++++++++++++++++++++---------
 1 file changed, 127 insertions(+), 38 deletions(-)


base-commit: 1292bca0f8d835d2ad96d309595b2e97f3106d3d
-- 
2.55.0

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

[Aditya Dash]

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

The bytes identify the output report, configuration group, command,
sub-command, left controller, and Start action, in that order.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
 drivers/hid/hid-lenovo-go.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 318b1152ff8b..b8248bf8965c 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");
 
-static struct go_cfg_attr cal_joy_left = { JOYSTICK_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_joy_left, "calibrate_joystick", SET_JOYSTICK_CFG,
+static struct go_cfg_attr cal_joy_left = { SET_JOYSTICK_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_joy_left, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_left_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_left = { GYRO_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_gyro_left, "calibrate_gyro", SET_GYRO_CFG,
+static struct go_cfg_attr cal_gyro_left = { SET_GYRO_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_gyro_left, "calibrate_gyro", GYRO_CALIBRATE,
 		     LEFT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_left_index, "calibrate_gyro_index");
 
@@ -2018,18 +2018,18 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_right, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_right_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_right = { TRIGGER_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_trigg_right, "calibrate_trigger", SET_TRIGGER_CFG,
+static struct go_cfg_attr cal_trigg_right = { SET_TRIGGER_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_trigg_right, "calibrate_trigger", TRIGGER_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_trigg_right_index, "calibrate_trigger_index");
 
-static struct go_cfg_attr cal_joy_right = { JOYSTICK_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_joy_right, "calibrate_joystick", SET_JOYSTICK_CFG,
+static struct go_cfg_attr cal_joy_right = { SET_JOYSTICK_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_joy_right, "calibrate_joystick", JOYSTICK_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_joy_right_index, "calibrate_joystick_index");
 
-static struct go_cfg_attr cal_gyro_right = { GYRO_CALIBRATE };
-LEGO_CAL_DEVICE_ATTR(cal_gyro_right, "calibrate_gyro", SET_GYRO_CFG,
+static struct go_cfg_attr cal_gyro_right = { SET_GYRO_CFG };
+LEGO_CAL_DEVICE_ATTR(cal_gyro_right, "calibrate_gyro", GYRO_CALIBRATE,
 		     RIGHT_CONTROLLER, index);
 static DEVICE_ATTR_RO_NAMED(cal_gyro_right_index, "calibrate_gyro_index");
 
-- 
2.55.0

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

[sashiko-bot]

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 <mradityadash@gmail.com>

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/20260821214810.87826-1-mradityadash@gmail.com?part=1

[PATCH 2/5] HID: hid-lenovo-go: fix the FPS mode DPI request

[Aditya Dash]

mcu_property_out() adds the five-byte request header used by most
configuration commands. For FPS mode DPI writes, this inserts a device byte
before the four-byte DPI value.

The FPS mode DPI request does not contain a device byte. Its little-endian
DPI value starts immediately after the command and sub-command.

Use the four-byte header only for FPS mode DPI writes. Keep the common
header for all other requests.

Fixes: f0bedee60607 ("HID: hid-lenovo-go: Add FPS Mode DPI settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
The corrected FPS mode DPI write was not tested on hardware.

 drivers/hid/hid-lenovo-go.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index b8248bf8965c..a6e9a130ad41 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -725,6 +725,11 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	int timeout = 50;
 	int ret;
 
+	/* The FPS mode DPI request does not contain a device byte. */
+	if (id == MCU_CONFIG_DATA && command == SET_DPI_CFG &&
+	    index == FPS_MODE_DPI)
+		header_size--;
+
 	if (header_size + len > GO_PACKET_SIZE)
 		return -EINVAL;
 
-- 
2.55.0

Re: [PATCH 2/5] HID: hid-lenovo-go: fix the FPS mode DPI request

[sashiko-bot]

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

New issues:
- [High] FPS mode DPI read requests and responses use incorrect byte offsets and headers.

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in the hid_go_probe() error path.
- [High] The driver uses a single global static variable drvdata for device-specific state.
- [High] Early return in hid_go_remove() bypasses hid_hw_stop() completely.
- [High] get_endpoint_address() lacks a boundary check for bNumEndpoints.
- [High] AB-BA deadlock between device removal and sysfs file access.
- [High] Passing a NULL pointer to memcpy() when processing GET requests.
- [Medium] Sysfs attribute groups are leaked on hid_go_cfg_probe() error paths.
--

--- Patch [2]: [PATCH 2/5] HID: hid-lenovo-go: fix the FPS mode DPI request ---
commit 9b984e3644e77c41adb68e3622677de0ae7cd145
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: fix the FPS mode DPI request

mcu_property_out() adds the five-byte request header used by most
configuration commands. For FPS mode DPI writes, this inserts a device byte
before the four-byte DPI value.

The FPS mode DPI request does not contain a device byte. Its little-endian
DPI value starts immediately after the command and sub-command.

Use the four-byte header only for FPS mode DPI writes. Keep the common
header for all other requests.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..a6e9a130ad417 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -725,6 +725,11 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
>  	int timeout = 50;
>  	int ret;
>  
> +	/* The FPS mode DPI request does not contain a device byte. */
> +	if (id == MCU_CONFIG_DATA && command == SET_DPI_CFG &&
> +	    index == FPS_MODE_DPI)
> +		header_size--;
> +

[Severity: High]
Does this change result in FPS mode DPI read requests and responses using
incorrect byte offsets and headers?

Since the check specifically targets SET_DPI_CFG, a GET_DPI_CFG request from
fps_mode_dpi_show() will still send a 5-byte header. hid_go_fps_dpi_event()
will then read the response from an incorrect byte offset due to asymmetric
header tracking.

>  	if (header_size + len > GO_PACKET_SIZE)
>  		return -EINVAL;
>  

While reviewing this patch, I also noticed a few preexisting issues in this
file.

[Severity: High]
This isn't a bug introduced by this patch, but does this invoke undefined
behavior by passing a NULL pointer to memcpy()?

drivers/hid/hid-lenovo-go.c:mcu_property_out()
    ...
    memcpy(dmabuf, header, header_size);
    memcpy(dmabuf + header_size, data, len);
    ...

When mcu_property_out() is called for GET requests (like GET_DPI_CFG in
fps_mode_dpi_show()), the data pointer is NULL and len is 0.


[Severity: High]
This isn't a bug introduced by this patch, but does using a single global
static variable drvdata cause multiple devices to corrupt each other's state?

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

The global drvdata is mutated per-device and its mutex is re-initialized on
every probe, which could cause issues if more than one of these devices is
connected.


[Severity: High]
This isn't a bug introduced by this patch, but does the hid_go_probe() error
path miss calling hid_hw_stop() and hid_hw_close()?

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, this returns the error without tearing down the
hardware that was started earlier, which could lead to a resource leak and
use-after-free.


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

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

The function returns directly on error without calling sysfs_remove_groups()
to clean up the groups created earlier by sysfs_create_groups().


[Severity: High]
This isn't a bug introduced by this patch, but does this early return in
hid_go_remove() completely bypass hid_hw_stop()?

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

    if (ep <= 0)
        return;

If the endpoint is not found or is less than zero on disconnect, the early
return skips hid_hw_stop() and hid_hw_close(), leaving hardware active.


[Severity: High]
This isn't a bug introduced by this patch, but does get_endpoint_address() lack
a boundary check for bNumEndpoints?

drivers/hid/hid-lenovo-go.c:get_endpoint_address()
    ...
    ep = intf->cur_altsetting->endpoint;
    if (!ep)
        return -ENODEV;

    return ep->desc.bEndpointAddress;

Accessing ep->desc.bEndpointAddress directly without verifying that
intf->cur_altsetting->desc.bNumEndpoints > 0 could result in an out-of-bounds
read on the endpoint descriptor array.


[Severity: High]
This isn't a bug introduced by this patch, but is there an AB-BA deadlock
between device removal and sysfs file access here?

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

hid_go_cfg_remove() acquires cfg_mutex before sysfs_remove_groups(). If a
sysfs write is waiting on cfg_mutex while holding the kernfs active
reference, calling sysfs_remove_groups() while holding cfg_mutex will
deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821214810.87826-1-mradityadash@gmail.com?part=2

[PATCH 3/5] HID: hid-lenovo-go: record calibration completion state

[Aditya Dash]

The firmware uses result value 0 for failure and 1 for success. The driver
stores this value directly as a state index. Index 0 means unknown, so the
driver reports a firmware failure as unknown.

Map the result value to the matching failure or success state.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
A successful calibration completion was tested on hardware. No final
failure completion was captured.

 drivers/hid/hid-lenovo-go.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index a6e9a130ad41..d2d134873dd1 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -577,17 +577,19 @@ static int hid_go_light_event(struct command_report *cmd_rep)
 
 static int hid_go_device_status_event(struct command_report *cmd_rep)
 {
+	u8 status = cmd_rep->data[1] ? CAL_STAT_SUCCESS : CAL_STAT_FAILURE;
+
 	switch (cmd_rep->device_type) {
 	case LEFT_CONTROLLER:
 		switch (cmd_rep->data[0]) {
 		case CALDEV_GYROSCOPE:
-			drvdata.gp_left_gyro_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_gyro_cal_status = status;
 			return 0;
 		case CALDEV_JOYSTICK:
-			drvdata.gp_left_joy_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_joy_cal_status = status;
 			return 0;
 		case CALDEV_TRIGGER:
-			drvdata.gp_left_trigg_cal_status = cmd_rep->data[1];
+			drvdata.gp_left_trigg_cal_status = status;
 			return 0;
 		default:
 			return -EINVAL;
@@ -596,13 +598,13 @@ static int hid_go_device_status_event(struct command_report *cmd_rep)
 	case RIGHT_CONTROLLER:
 		switch (cmd_rep->data[0]) {
 		case CALDEV_GYROSCOPE:
-			drvdata.gp_right_gyro_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_gyro_cal_status = status;
 			return 0;
 		case CALDEV_JOYSTICK:
-			drvdata.gp_right_joy_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_joy_cal_status = status;
 			return 0;
 		case CALDEV_TRIGGER:
-			drvdata.gp_right_trigg_cal_status = cmd_rep->data[1];
+			drvdata.gp_right_trigg_cal_status = status;
 			return 0;
 		default:
 			return -EINVAL;
-- 
2.55.0

Re: [PATCH 3/5] HID: hid-lenovo-go: record calibration completion state

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Dangling pointer read when USB interface has 0 endpoints.
- [Critical] Driver uses a single global struct for device-specific state.
- [High] Missing `hid_hw_stop()` and resource cleanup in `probe()` error paths.
- [High] Early return in `remove()` omits mandatory `hid_hw_stop()`.
- [High] LED unregistration sends hardware commands after `hid_hw_stop()` has been called.
- [High] Delayed work is not cancelled on probe error path, risking Use-After-Free.
- [High] AB-BA deadlock during device removal when cleaning up sysfs groups.
- [High] AB-BA deadlock in `hid_gos_cfg_remove()` due to incorrect mutex locking order.
--

--- Patch [3]: [PATCH 3/5] HID: hid-lenovo-go: record calibration completion state ---
commit 0c0b87dd6928291724bdcbfc25cdec4b399b857c
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: record calibration completion state

The firmware uses result value 0 for failure and 1 for success. The driver
stores this value directly as a state index, mapping 0 to unknown. Map the
result value to the matching failure or success state.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index a6e9a130ad417..d2d134873dd1f 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -577,17 +577,19 @@ static int hid_go_light_event(struct command_report *cmd_rep)
>  
>  static int hid_go_device_status_event(struct command_report *cmd_rep)
>  {
> +	u8 status = cmd_rep->data[1] ? CAL_STAT_SUCCESS : CAL_STAT_FAILURE;
> +
>  	switch (cmd_rep->device_type) {
>  	case LEFT_CONTROLLER:
>  		switch (cmd_rep->data[0]) {
>  		case CALDEV_GYROSCOPE:
> -			drvdata.gp_left_gyro_cal_status = cmd_rep->data[1];
> +			drvdata.gp_left_gyro_cal_status = status;

[Severity: Critical]
This is a pre-existing issue, but might using a single global struct for
device-specific state cause cross-device state corruption?

When multiple compatible devices are plugged in, their probes concurrently
overwrite drvdata.hdev, reinitialize the same cfg_mutex, and queue the same
delayed work:

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

Can this be easily triggered by physically connecting two Lenovo Go controllers
or via a crafted USB device presenting multiple matching interfaces?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could the probe function be
returning an error code without stopping the hardware if configuration fails?

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 to create the RGB device, it returns an error:

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

Does failing to clean up the manually allocated sysfs groups on this error path
leak them? Also, does this leave the hardware running without a bound driver,
which could lead to a use-after-free when events are processed?


[Severity: High]
This is a pre-existing issue, but is the mandatory hardware stop and teardown
bypassed by the early return in the remove function?

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

    if (ep <= 0)
        return;

    switch (ep) {
    ...
}

If get_endpoint_address() returns <= 0, the mandatory hardware stop and teardown
are bypassed. Since probe() successfully starts the hardware for such devices,
does this leave the HID device active after unbind, causing a use-after-free
when devres releases memory?


[Severity: Critical]
This is a pre-existing issue, but are we risking an out-of-bounds memory access
in get_endpoint_address() if the USB interface has 0 endpoints?

drivers/hid/hid-lenovo-go.c:get_endpoint_address() {
    ...
    ep = &intf->cur_altsetting->endpoint[0];
    if (!ep)
        return -EINVAL;
    ...
}

If a USB device declares 0 endpoints, the USB core allocates a 0-length endpoint
array. Does accessing the first endpoint here without validating bNumEndpoints
lead to an out-of-bounds memory access?


[Severity: High]
This is a pre-existing issue, but will LED unregistration attempt to send
hardware commands after the hardware has been stopped?

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

Since hid_hw_stop() is called here before devres automatically unregisters the
LED, the subsystem will issue a turn-off command via hid_go_brightness_set().
Does this attempt to send an output report to the already-stopped device,
causing transport driver errors or unbind stalls?


[Severity: High]
This isn't a bug introduced by this patch, but are we leaving delayed work
active in the probe error path?

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, the driver treats this as an error and
returns -ENODEV directly. Devres frees the device resources, but the delayed
work is never cancelled. Does this lead to a use-after-free when the dangling
delayed work handler executes?


[Severity: High]
This is a pre-existing issue, but could a lock inversion deadlock occur during
device removal when cleaning up sysfs groups?

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

hid_go_cfg_remove() acquires cfg_mutex and then waits for sysfs operations to
drain. Concurrently, a sysfs write acquires the active kernfs reference first
and then attempts to acquire the lock:

drivers/hid/hid-lenovo-go.c:mcu_property_out() {
    ...
    guard(mutex)(&drvdata.cfg_mutex);
    ...
}

Could a local user with sysfs write privileges trigger this deadlock during a
device unplug event?


[Severity: High]
This isn't a bug introduced by this patch, but does a similar lock inversion
deadlock exist in the companion driver?

drivers/hid/hid-lenovo-go-s.c:hid_gos_cfg_remove() {
    ...
    guard(mutex)(&drvdata.cfg_mutex);
    cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
    ...
}

The cfg_mutex is acquired before cancel_delayed_work_sync() is called. If the
worker is running, it will attempt to call mcu_property_out(), which blocks on
acquiring the same cfg_mutex. Could this cause a deadlock during device unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821214810.87826-1-mradityadash@gmail.com?part=3

[PATCH 4/5] HID: hid-lenovo-go: use the right controller selectors

[Aditya Dash]

The right imu_enabled attribute selects FEATURE_IMU_BYPASS, and the right
reset attribute targets the left controller.

Select FEATURE_IMU_ENABLE for the right imu_enabled attribute. Target the
right controller for a right reset.

Fixes: 82cd9bc866e1 ("HID: hid-lenovo-go: Add Feature Status Attributes")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
The corrected right reset and right IMU writes were not tested on
hardware.

 drivers/hid/hid-lenovo-go.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index d2d134873dd1..1e6915c969d8 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -2006,13 +2006,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,
 		    index, feature_status);
 static DEVICE_ATTR_RO_NAMED(imu_bypass_right_index, "imu_bypass_enabled_index");
 
-static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_BYPASS };
+static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_ENABLE };
 LEGO_DEVICE_ATTR_RW(imu_enabled_right, "imu_enabled", RIGHT_CONTROLLER, index,
 		    feature_status);
 static DEVICE_ATTR_RO_NAMED(imu_enabled_right_index, "imu_enabled_index");
 
 static struct go_cfg_attr reset_right = { FEATURE_RESET_GAMEPAD };
-LEGO_DEVICE_ATTR_WO(reset_right, "reset", LEFT_CONTROLLER, feature_status);
+LEGO_DEVICE_ATTR_WO(reset_right, "reset", RIGHT_CONTROLLER, feature_status);
 
 static struct go_cfg_attr rumble_mode_right = { RUMBLE_MODE };
 LEGO_DEVICE_ATTR_RW(rumble_mode_right, "rumble_mode", RIGHT_CONTROLLER, index,
-- 
2.55.0

Re: [PATCH 4/5] HID: hid-lenovo-go: use the right controller selectors

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cleanup in hid_go_probe() error paths.
- [High] Global state corruption from statically allocated per-device state.
- [High] AB-BA deadlock in hid_go_cfg_remove() with sysfs.
- [High] Early return in hid_go_remove() bypassing cleanup causes Use-After-Free.
--

--- Patch [4]: [PATCH 4/5] HID: hid-lenovo-go: use the right controller selectors ---
commit 21361c83148d2f3658bbae16a33bf294b6a2f9f0
Author: Aditya Dash <mradityadash@gmail.com>

    HID: hid-lenovo-go: use the right controller selectors
    
    The right imu_enabled attribute selects FEATURE_IMU_BYPASS, and the right
    reset attribute targets the left controller. Select FEATURE_IMU_ENABLE
    for the right imu_enabled attribute. Target the right controller for a
    right reset.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index d2d134873dd1f..1e6915c969d8c 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -2006,13 +2006,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,
>  		    index, feature_status);
>  static DEVICE_ATTR_RO_NAMED(imu_bypass_right_index, "imu_bypass_enabled_index");
>  
> -static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_BYPASS };
> +static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_ENABLE };
>  LEGO_DEVICE_ATTR_RW(imu_enabled_right, "imu_enabled", RIGHT_CONTROLLER, index,
>  		    feature_status);
>  static DEVICE_ATTR_RO_NAMED(imu_enabled_right_index, "imu_enabled_index");
>  
>  static struct go_cfg_attr reset_right = { FEATURE_RESET_GAMEPAD };
> -LEGO_DEVICE_ATTR_WO(reset_right, "reset", LEFT_CONTROLLER, feature_status);
> +LEGO_DEVICE_ATTR_WO(reset_right, "reset", RIGHT_CONTROLLER, feature_status);
>  
>  static struct go_cfg_attr rumble_mode_right = { RUMBLE_MODE };
>  LEGO_DEVICE_ATTR_RW(rumble_mode_right, "rumble_mode", RIGHT_CONTROLLER, index,

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the error
path in hid_go_probe() to 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");
        return ret;
    }
    ...
}

If hid_go_cfg_probe() fails (e.g., due to sysfs_create_groups() returning
an error), it propagates the error and returns without calling
hid_hw_close() and hid_hw_stop().

Additionally, does hid_go_cfg_probe() leave sysfs attributes active if it
fails?

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

Subsequent error paths in hid_go_cfg_probe() do not appear to clean up
these sysfs groups before returning. If the module is subsequently unloaded,
the un-removed sysfs attributes could remain on the device.

[Severity: High]
This is a pre-existing issue, but does using a static global struct for
device state cause corruption if multiple devices are probed?

drivers/hid/hid-lenovo-go.c: {
...
static struct hid_go_cfg {
    struct delayed_work go_cfg_setup;
    struct led_classdev *led_cdev;
    struct hid_device *hdev;
    struct mutex cfg_mutex;
//...
} drvdata;
...
}

When hid_go_cfg_probe() is called, it overwrites the hardware device pointer
and re-initializes the work item:

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

If a second gamepad is connected, could this overwrite drvdata.hdev, causing
sysfs requests for the first device to operate on the second? Also, could
calling INIT_DELAYED_WORK() while it is pending for the first device corrupt
the timer list entry?

[Severity: High]
This is a pre-existing issue, but is there a potential deadlock between
sysfs and the configuration mutex in hid_go_cfg_remove()?

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

sysfs_remove_groups() synchronously waits for active sysfs readers/writers
to drop their kernfs active references. Concurrently, a sysfs reader/writer
might invoke mcu_property_out(), which attempts to acquire
drvdata.cfg_mutex. Could this result in the remove thread holding the mutex
and waiting for sysfs, while the sysfs thread holds the active reference and
waits for the mutex?

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

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

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

In hid_go_probe(), missing endpoints result in returning success (0),
leaving the hardware active:

drivers/hid/hid-lenovo-go.c:hid_go_probe() {
    ...
    ep = get_endpoint_address(hdev);
    if (ep != GO_GP_INTF_IN) {
        dev_dbg(&hdev->dev, "Started interface %x as generic HID device\n", ep);
        return 0;
    }
    ...
}

If the device is later unplugged or unbound, hid_go_remove() returns early,
bypassing hid_hw_stop(). Could the HID hardware remain active while devres
frees the driver data, potentially allowing subsequent incoming hardware
events to access freed devres structures?

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

[PATCH 5/5] HID: hid-lenovo-go: return configuration request errors

[Aditya Dash]

The driver completes a pending request when it receives any decoded
configuration reply. It also discards the result of its interruptible wait.
A configuration operation can therefore report success after an unrelated
reply, firmware error, interrupted wait, or timeout.

Track one pending request and match replies by command ID, command,
sub-command, and device. Return output errors, firmware errors, interrupted
waits, and timeouts to the caller. Treat a timeout or interruption as final
if a matching reply arrives after the wait finishes.

Calibration completion is asynchronous, and some Start commands do not send
an immediate reply. Submit each exact calibration command and sub-command
pair once and return its HID output status without a synchronous wait.

The firmware does not include a sequence number. A late reply with the same
complete tuple can still match a newer request.

Fixes: d69ccfcbc955 ("HID: hid-lenovo-go: Add Lenovo Legion Go Series HID Driver")
Assisted-by: Pi:gpt-5.6-sol
Signed-off-by: Aditya Dash <mradityadash@gmail.com>
---
On an Original Legion Go, left-joystick Start did not return an
immediate reply in two controller sessions although firmware began
calibration. A focused target-kernel KUnit test covered timeout,
interruption, firmware error, and reply ownership. No natural firmware
rejection was reproduced.

 drivers/hid/hid-lenovo-go.c | 118 ++++++++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 1e6915c969d8..a1a255c28e2d 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -21,6 +21,7 @@
 #include <linux/led-class-multicolor.h>
 #include <linux/mutex.h>
 #include <linux/printk.h>
+#include <linux/spinlock.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
 #include <linux/unaligned.h>
@@ -31,13 +32,30 @@
 #include "hid-ids.h"
 
 #define GO_GP_INTF_IN		0x83
+#define GO_INPUT_REPORT_ID	0x04
 #define GO_OUTPUT_REPORT_ID	0x05
 #define GO_GP_RESET_SUCCESS	0x01
 #define GO_PACKET_SIZE		64
 
+/* Lenovo replies identify a command and a sub-command, but have no sequence. */
+struct hid_go_cmd {
+	struct completion done;
+	spinlock_t lock; /* protects fields below */
+	bool pending;
+	u8 id;
+	u8 command;
+	u8 sub_command;
+	u8 device;
+	int result;
+};
+
+static struct hid_go_cmd go_cmd = {
+	.done = COMPLETION_INITIALIZER(go_cmd.done),
+	.lock = __SPIN_LOCK_UNLOCKED(go_cmd.lock),
+};
+
 static struct hid_go_cfg {
 	struct delayed_work go_cfg_setup;
-	struct completion send_cmd_complete;
 	struct led_classdev *led_cdev;
 	struct hid_device *hdev;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
@@ -331,6 +349,72 @@ static const char *const os_mode_text[] = {
 	[LINUX] = "linux",
 };
 
+static void hid_go_cmd_arm(u8 id, u8 command, u8 sub_command, u8 device)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&go_cmd.lock, flags);
+	reinit_completion(&go_cmd.done);
+	go_cmd.pending = true;
+	go_cmd.id = id;
+	go_cmd.command = command;
+	go_cmd.sub_command = sub_command;
+	go_cmd.device = device;
+	spin_unlock_irqrestore(&go_cmd.lock, flags);
+}
+
+static void hid_go_cmd_consume(const struct command_report *cmd_rep, int result)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&go_cmd.lock, flags);
+	if (go_cmd.pending && cmd_rep->id == go_cmd.id &&
+	    cmd_rep->cmd == go_cmd.command &&
+	    cmd_rep->sub_cmd == go_cmd.sub_command &&
+	    cmd_rep->device_type == go_cmd.device) {
+		go_cmd.pending = false;
+		go_cmd.result = result;
+		complete(&go_cmd.done);
+	}
+	spin_unlock_irqrestore(&go_cmd.lock, flags);
+}
+
+static int hid_go_cmd_finish(long wait_result)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&go_cmd.lock, flags);
+	if (wait_result <= 0) {
+		go_cmd.pending = false;
+		ret = wait_result < 0 ? wait_result : -ETIMEDOUT;
+	} else {
+		ret = go_cmd.result;
+	}
+	spin_unlock_irqrestore(&go_cmd.lock, flags);
+	return ret;
+}
+
+static int hid_go_cmd_cancel(int result)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&go_cmd.lock, flags);
+	go_cmd.pending = false;
+	spin_unlock_irqrestore(&go_cmd.lock, flags);
+	return result;
+}
+
+static int hid_go_send_output_report(struct hid_device *hdev, u8 *packet)
+{
+	int ret;
+
+	ret = hid_hw_output_report(hdev, packet, GO_PACKET_SIZE);
+	if (ret < 0)
+		return ret;
+	return ret == GO_PACKET_SIZE ? 0 : -EINVAL;
+}
+
 static int hid_go_version_event(struct command_report *cmd_rep)
 {
 	switch (cmd_rep->sub_cmd) {
@@ -658,6 +742,8 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
 
 	if (size != GO_PACKET_SIZE)
 		goto passthrough;
+	if (data[0] != GO_INPUT_REPORT_ID)
+		goto passthrough;
 
 	ep = get_endpoint_address(hdev);
 	if (ep != GO_GP_INTF_IN)
@@ -709,7 +795,7 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
 	dev_dbg(&hdev->dev, "Rx data as raw input report: [%*ph]\n",
 		GO_PACKET_SIZE, data);
 
-	complete(&drvdata.send_cmd_complete);
+	hid_go_cmd_consume(cmd_rep, ret);
 	return ret;
 
 passthrough:
@@ -725,6 +811,7 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	u8 header[] = { GO_OUTPUT_REPORT_ID, id, command, index, device };
 	size_t header_size = ARRAY_SIZE(header);
 	int timeout = 50;
+	long wait_result;
 	int ret;
 
 	/* The FPS mode DPI request does not contain a device byte. */
@@ -747,22 +834,19 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
 		GO_PACKET_SIZE, dmabuf);
 
-	ret = hid_hw_output_report(hdev, dmabuf, GO_PACKET_SIZE);
-	if (ret < 0)
-		return ret;
+	if (id == MCU_CONFIG_DATA &&
+	    ((command == SET_TRIGGER_CFG && index == TRIGGER_CALIBRATE) ||
+	     (command == SET_JOYSTICK_CFG && index == JOYSTICK_CALIBRATE) ||
+	     (command == SET_GYRO_CFG && index == GYRO_CALIBRATE)))
+		return hid_go_send_output_report(hdev, dmabuf);
 
-	ret = ret == GO_PACKET_SIZE ? 0 : -EINVAL;
+	hid_go_cmd_arm(id, command, index, device);
+	ret = hid_go_send_output_report(hdev, dmabuf);
 	if (ret)
-		return ret;
-
-	ret = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
-							msecs_to_jiffies(timeout));
-
-	if (ret == 0) /* timeout occurred */
-		ret = -EBUSY;
-
-	reinit_completion(&drvdata.send_cmd_complete);
-	return 0;
+		return hid_go_cmd_cancel(ret);
+	wait_result = wait_for_completion_interruptible_timeout(&go_cmd.done,
+								msecs_to_jiffies(timeout));
+	return hid_go_cmd_finish(wait_result);
 }
 
 static ssize_t version_show(struct device *dev, struct device_attribute *attr,
@@ -2395,8 +2479,6 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
 
 	drvdata.led_cdev = &go_cdev_rgb.led_cdev;
 
-	init_completion(&drvdata.send_cmd_complete);
-
 	/* Executing calls prior to returning from probe will lock the MCU. Schedule
 	 * initial data call after probe has completed and MCU can accept calls.
 	 */
-- 
2.55.0

Re: [PATCH 5/5] HID: hid-lenovo-go: return configuration request errors

[sashiko-bot]

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

New issues:
- [High] Global state variables prevent support for multiple devices, introduce cross-talk, and cause list corruption.
- [High] Propagating timeouts and errors from mcu_property_out() causes cfg_setup() to abort prematurely, leaving device state uninitialized.

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in probe error paths, and missing hid_hw_stop() in remove() for malformed endpoints.
- [High] Failure during schedule_delayed_work leaves the delayed work pending while probe fails, leading to a Use-After-Free.
- [High] ABBA deadlock between `drvdata.cfg_mutex` and kernfs active reference during unbind.
--

--- Patch [5]: [PATCH 5/5] HID: hid-lenovo-go: return configuration request errors ---
commit 3a9434af7b761a1d874276ba01fa7f976cec0f1f
Author: Aditya Dash <mradityadash@gmail.com>

    HID: hid-lenovo-go: return configuration request errors
    
    The driver completes a pending request when it receives any decoded
    configuration reply. It also discards the result of its interruptible wait.
    A configuration operation can therefore report success after an unrelated
    reply, firmware error, interrupted wait, or timeout.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 1e6915c969d8c..a1a255c28e2d2 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

> +/* Lenovo replies identify a command and a sub-command, but have no sequence. */
> +struct hid_go_cmd {
> +	struct completion done;
> +	spinlock_t lock; /* protects fields below */
> +	bool pending;
> +	u8 id;
> +	u8 command;
> +	u8 sub_command;
> +	u8 device;
> +	int result;
> +};
> +
> +static struct hid_go_cmd go_cmd = {
> +	.done = COMPLETION_INITIALIZER(go_cmd.done),
> +	.lock = __SPIN_LOCK_UNLOCKED(go_cmd.lock),
> +};

[Severity: High]
Does this global state variable prevent support for multiple devices and
introduce cross-talk?

If multiple Lenovo Legion Go controllers are connected simultaneously, they
will share this global go_cmd singleton. A late reply from one controller
could satisfy a pending wait on another, or hid_go_cmd_arm() will blindly
overwrite the state of a concurrent command.

Additionally, this driver uses global state in hid_go_cfg_probe():

	ret = devm_led_classdev_multicolor_register(&hdev->dev, &go_cdev_rgb);

and:

	INIT_DELAYED_WORK(&drvdata.go_cfg_setup, &cfg_setup);

Using a static global struct pointer (&go_cdev_rgb) corrupts the LED
subsystem's linked lists when a second device is probed. Unconditionally
resetting a global work structure with INIT_DELAYED_WORK corrupts workqueue
linked lists if the work is already pending.

[ ... ]

> @@ -747,22 +834,19 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
>  	dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
>  		GO_PACKET_SIZE, dmabuf);
>  
> -	ret = hid_hw_output_report(hdev, dmabuf, GO_PACKET_SIZE);
> -	if (ret < 0)
> -		return ret;
> +	if (id == MCU_CONFIG_DATA &&
> +	    ((command == SET_TRIGGER_CFG && index == TRIGGER_CALIBRATE) ||
> +	     (command == SET_JOYSTICK_CFG && index == JOYSTICK_CALIBRATE) ||
> +	     (command == SET_GYRO_CFG && index == GYRO_CALIBRATE)))
> +		return hid_go_send_output_report(hdev, dmabuf);
>  
> -	ret = ret == GO_PACKET_SIZE ? 0 : -EINVAL;
> +	hid_go_cmd_arm(id, command, index, device);
> +	ret = hid_go_send_output_report(hdev, dmabuf);
>  	if (ret)
> -		return ret;
> -
> -	ret = wait_for_completion_interruptible_timeout(&drvdata.send_cmd_complete,
> -							msecs_to_jiffies(timeout));
> -
> -	if (ret == 0) /* timeout occurred */
> -		ret = -EBUSY;
> -
> -	reinit_completion(&drvdata.send_cmd_complete);
> -	return 0;
> +		return hid_go_cmd_cancel(ret);
> +	wait_result = wait_for_completion_interruptible_timeout(&go_cmd.done,
> +								msecs_to_jiffies(timeout));
> +	return hid_go_cmd_finish(wait_result);

[Severity: High]
Does returning the exact timeout or error from mcu_property_out() cause
cfg_setup() to abort prematurely and leave the device uninitialized?

Prior to this patch, mcu_property_out() unconditionally returned 0 on timeout.
The initialization worker cfg_setup() sequentially probes all sub-components
and aborts on the first error:

	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, GET_VERSION_DATA,
			       PRODUCT_VERSION, USB_MCU, NULL, 0);
	if (ret < 0) {
		dev_err(&drvdata.hdev->dev,
			"Failed to retrieve USB_MCU Product Version: %i\n", ret);
		return;
	}

Since mcu_property_out() now propagates -ETIMEDOUT or firmware errors, the
first non-responsive component (such as querying TX_DONGLE when connected
via USB) will cause cfg_setup() to completely abort, leaving RGB LEDs and
other attributes uninitialized.

[ ... ]

> @@ -2395,8 +2479,6 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>  
>  	drvdata.led_cdev = &go_cdev_rgb.led_cdev;
>  
> -	init_completion(&drvdata.send_cmd_complete);
> -
>  	/* Executing calls prior to returning from probe will lock the MCU. Schedule
>  	 * initial data call after probe has completed and MCU can accept calls.
>  	 */
>  	INIT_DELAYED_WORK(&drvdata.go_cfg_setup, &cfg_setup);
>  	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;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does returning -ENODEV here
leave the delayed work pending while the probe fails?

If schedule_delayed_work() returns 0 (which happens if the work is already
queued by a previously probed device), probe fails and returns -ENODEV. The
HID core will destroy the device and free its devres-managed memory, but the
delayed work is never canceled in this error path. When it fires, cfg_setup()
will dereference the freed hdev pointer, resulting in a use-after-free.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does missing hid_hw_stop() and hid_hw_close()
in probe error paths cause resource leaks and use-after-free?

In 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 the error directly without
unwinding hid_hw_start() and hid_hw_open(). The HID core does not automatically
stop the hardware if a custom probe fails after hid_hw_start() has succeeded.

Similarly, in hid_go_remove():

	int ep = get_endpoint_address(hdev);

	if (ep <= 0)
		return;

If a malicious USB device presents no endpoints, get_endpoint_address() returns
-ENODEV (<= 0), causing the function to return early without calling
hid_hw_close() and hid_hw_stop().

[Severity: High]
This is a pre-existing issue, but does locking drvdata.cfg_mutex before
removing sysfs groups create an ABBA deadlock?

During unbind, hid_go_cfg_remove() does this:

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

When a sysfs file is read, kernfs first acquires an active reference, and then
the driver callback acquires drvdata.cfg_mutex.

During device removal, hid_go_cfg_remove() first acquires drvdata.cfg_mutex
and then calls sysfs_remove_groups(), which blocks waiting for all kernfs
active references to drop. If a sysfs read races with device unbind, Thread 1
holds the kernfs active ref and waits for cfg_mutex, while Thread 2 holds
cfg_mutex and waits for the kernfs active ref to drop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821214810.87826-1-mradityadash@gmail.com?part=5