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

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

[Aditya Dash]

This series fixes several issues in the controller configuration and
calibration paths in hid-lenovo-go.

1/7 fixes the calibration attributes, where the command and sub-command
are swapped. 2/7 fixes two right-controller attributes: imu_enabled
selects the bypass feature, and reset targets the left controller.

The configuration interface initializes its command completion after the
HID hardware starts and opens. 3/7 moves that initialization before both
hardware operations.

Before 4/7, any decoded configuration reply completes the shared
completion. The driver then ignores the result of the wait, so an
unrelated reply, a firmware error, an interrupted wait, or a timeout can
still be reported as success.

4/7 tracks one pending request in the driver data and matches replies by
command ID, command, sub-command, and device. It initializes the new
completion and spinlock at the pre-hardware point established by 3/7. It
also returns request errors to the caller.

The firmware has no sequence number, so a late reply with the same tuple
can still match a newer request.

The remaining patches fix calibration status handling.

The calibration status table defines 0x00 as unknown, 0x01 as success,
and 0x02 as failure. A Legion Go 1 returned 0x08 after a Stop was sent
while no calibration was active. The driver stores the raw value as a
status-text index, so a later read returns -EINVAL.

5/7 preserves the defined values and maps larger status values to
failure.

6/7 rejects 'unknown' as an action. The action table contains it at index
zero, but the options attribute advertises only 'start' and 'stop'.

A previous result can also hide a new calibration attempt:

failure -> start -> failure

The status appears unchanged for the whole operation. 7/7 clears the
selected status to unknown before submitting a Start. Stop leaves the
status unchanged.

I built every patch against kernel 7.2.1-ogc3.1.fc44.x86_64 with
W=1 and KCFLAGS=-Werror. I tested the exact final module on an Original
Legion Go. The configuration interface bound without a kernel fault.
An idle Stop result of 0x08 was reported as failure. Writing 'unknown'
returned -EINVAL without an output report or status change. A later Start
produced failure -> unknown -> failure; the final report contained result
0x02 and error 0x0007. The five unselected statuses stayed unchanged.

---
Changes in v3:
- Initialize command completion state before hid_hw_start() and
  hid_hw_open().
- Remove the unreachable zero-length calibration action path after
  rejecting 'unknown'.

v2 review:
https://lore.kernel.org/all/CAFqHKTnQiTQc6joz-Y6tmjR=C5giF-MuHeBwu3rw+bmFVMQtqw@mail.gmail.com/

v2:
https://lore.kernel.org/all/cover.1788096666.git.mradityadash@gmail.com/

Changes in v2:
- Drop the FPS DPI patch; the existing request is correct.
- Drop the v1 Boolean calibration-result mapping. Preserve values 0x00
  through 0x02 and map only larger failure values.
- Move the existing pending command state into the driver data and
  initialize it in cfg_probe().
- Use scoped IRQ-save guards and validate the report size and ID together.
- Drop the v1 no-wait calibration path. Correct requests receive their
  matching SET replies within the existing 50 ms timeout.
- Reject 'unknown' and clear only the selected status on Start.

v1:
https://lore.kernel.org/all/20260821214810.87826-1-mradityadash@gmail.com/


Aditya Dash (7):
  HID: hid-lenovo-go: use the correct calibration commands
  HID: hid-lenovo-go: use the right controller selectors
  HID: hid-lenovo-go: initialize command completion before hardware
  HID: hid-lenovo-go: return configuration request errors
  HID: hid-lenovo-go: normalize calibration failure status
  HID: hid-lenovo-go: reject unknown calibration action
  HID: hid-lenovo-go: clear calibration status on start

 drivers/hid/hid-lenovo-go.c | 191 +++++++++++++++++++++++++++---------
 1 file changed, 142 insertions(+), 49 deletions(-)


base-commit: 1292bca0f8d835d2ad96d309595b2e97f3106d3d
-- 
2.55.0

[PATCH v3 1/7] 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>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@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 v3 1/7] HID: hid-lenovo-go: use the correct calibration commands

[sashiko-bot]

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 hid_go_probe() error path.
- [High] Sysfs attribute groups are leaked on error paths in hid_go_cfg_probe(), leading to an Execute-After-Free panic.
- [High] hid_go_remove fails to stop and close the HID hardware if get_endpoint_address returns a negative value.
- [High] Accessing the USB endpoint array without verifying the endpoint count can lead to an out-of-bounds read.
- [High] The driver uses a statically allocated global variable for device private data, breaking support for multiple connected devices.
--

commit 0f59f6a616b3448c35bd0ae030452c8605182e3d
Author: Aditya Dash <mradityadash@gmail.com>

    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 ...
    
    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>
    Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
    Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>

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

[ ... ]

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

> static int get_endpoint_address(struct hid_device *hdev)
> {
> 	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;
> }
        ^^^^
[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?


> static int hid_go_cfg_probe(struct hid_device *hdev,
> 			    const struct hid_device_id *_id)
> {
> 	unsigned char *buf;
> 	int ret;
> 
> 	buf = devm_kzalloc(&hdev->dev, GO_PACKET_SIZE, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;
> 	mutex_init(&drvdata.cfg_mutex);
        ^^^^
[Severity: High]
This is a pre-existing issue, but does using a single static 'drvdata'
struct break support for multiple connected devices?

If two devices are connected (or one exposes multiple interfaces), the
second probe will unconditionally overwrite the global drvdata.hdev
pointer and re-initialize the mutex. Would dynamically allocating this
state (e.g., via devm_kzalloc) prevent cross-device command routing
issues and deadlocks?

> 	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 does the error path leak sysfs attributes?

If devm_led_classdev_multicolor_register() fails, the function returns
without calling sysfs_remove_groups(). If the driver unbinds, could reading
or writing the dangling sysfs files trigger an execute-after-free panic?


> static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> 	int ret, ep;
> 
> 	if (!hid_is_usb(hdev))
> 		return -EINVAL;
[ ... ]
> 	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 leave
the hardware and input nodes running?

If hid_go_cfg_probe() fails, the error code is returned directly without
calling hid_hw_close() and hid_hw_stop() to tear down what was started
earlier.

Since the HID core doesn't automatically stop the hardware if a custom probe
fails after hid_hw_start() succeeds, could this cause a use-after-free
if the module is unloaded while callbacks remain active?


> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;
        ^^^^
[Severity: High]
This is a pre-existing issue, but can this skip the required hardware
teardown when the driver is unbound?

If get_endpoint_address() returns a negative error code during unbind, the
function returns early. This skips hid_hw_close() and hid_hw_stop(), leaving
the hardware active.

Could this result in a use-after-free since the input_dev and hidraw
interfaces would remain active after the driver's resources are freed?

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

[PATCH v3 2/7] 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>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 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 b8248bf8965c..b937d11dd6f6 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -1999,13 +1999,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 v3 2/7] 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] The driver relies on static global structures (`drvdata`, `go_cdev_rgb`) to maintain per-device state and register LED devices, breaking multi-device support and causing memory corruption.
- [High] Hardware is left in an active, open state if `hid_go_cfg_probe()` fails, due to missing `hid_hw_stop()` and `hid_hw_close()` calls in the `hid_go_probe()` error path.
- [High] Hardware cleanup and workcancellation are bypassed in `hid_go_remove()` for generic endpoints, leaving unmanaged active hardware and risking use-after-free.
- [High] Out-of-bounds array read in `get_endpoint_address()` if an interface has no endpoints.
--

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

HID: hid-lenovo-go: use the right controller selectors

This commit corrects the right controller selectors by changing the
imu_enabled attribute to select FEATURE_IMU_ENABLE. It also fixes the right
reset attribute to target the right controller instead of the left.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..b937d11dd6f6a 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/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/cover.1788222001.git.mradityadash@gmail.com?part=2

[PATCH v3 3/7] HID: hid-lenovo-go: initialize command completion before hardware

[Aditya Dash]

The configuration interface initializes its command completion in
cfg_probe(). The hardware starts and opens before this call. Keep the
completion ready before hardware I/O becomes available.

Identify the interface after parsing it. Initialize the completion for the
configuration interface before starting and opening HID hardware. Keep the
remaining configuration setup in cfg_probe().

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>
---
 drivers/hid/hid-lenovo-go.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index b937d11dd6f6..51e3112780e1 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -2388,8 +2388,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.
 	 */
@@ -2436,6 +2434,10 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
+	ep = get_endpoint_address(hdev);
+	if (ep == GO_GP_INTF_IN)
+		init_completion(&drvdata.send_cmd_complete);
+
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Failed to start HID device\n");
@@ -2449,7 +2451,6 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	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;
-- 
2.55.0

Re: [PATCH v3 3/7] HID: hid-lenovo-go: initialize command completion before hardware

[sashiko-bot]

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

Pre-existing issues:
- [High] Global state singleton causes severe concurrency issues, state corruption, and waitqueue destruction when multiple devices or interfaces are probed.
- [High] Missing hardware cleanup and sysfs rollback in the `hid_go_probe()` error paths.
- [High] AB-BA deadlock between `cfg_mutex` and kernfs active protection during device removal.
- [High] Bypassed hardware stop in `hid_go_remove()` for invalid endpoints leads to severe UAF.
- [High] Devres LIFO ordering trap leaves the LED classdev active after the hardware is stopped.
--

commit 16e38723509d50d9bcf65a4f08049990d12d0c0f
Author: Aditya Dash <mradityadash@gmail.com>

    HID: hid-lenovo-go: initialize command completion before hardware
    
    The configuration interface initializes its command completion in
    cfg_probe(). The hardware starts and opens before this call. Keep the
    completion ready before hardware I/O becomes available.
    
    Identify the interface after parsing it. Initialize the completion for the
    configuration interface before starting and opening HID hardware. Keep the
    remaining configuration setup in cfg_probe().

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b937d11dd6f6a..51e3112780e18 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

> @@ -2436,6 +2434,10 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> +	ep = get_endpoint_address(hdev);
> +	if (ep == GO_GP_INTF_IN)
> +		init_completion(&drvdata.send_cmd_complete);
> +

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

>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {
>  		hid_err(hdev, "Failed to start HID device\n");
> @@ -2449,7 +2451,6 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> -	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;

[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/cover.1788222001.git.mradityadash@gmail.com?part=3

[PATCH v3 4/7] HID: hid-lenovo-go: return configuration request errors

[Aditya Dash]

Any decoded configuration reply completes the shared completion. The driver
also ignores the result of its interruptible wait. An unrelated reply, a
firmware error, an interrupted wait, or a timeout can therefore be reported
as success.

Add pending command state to the driver data. Match replies by command ID,
command, sub-command, and device, and return request errors to the caller.
Clear the pending request after an interruption or timeout so a late reply
cannot replace that result.

Keep the existing 50 ms wait for every configuration request. Corrected
calibration requests on an Original Legion Go received their matching SET
replies within 8 ms. The final calibration result remains asynchronous.

The firmware has no sequence number. A late reply with the same 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>
---
 drivers/hid/hid-lenovo-go.c | 109 +++++++++++++++++++++++++++++-------
 1 file changed, 88 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 51e3112780e1..deed7acd2dc4 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,27 @@
 #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
+#define GO_COMMAND_TIMEOUT_MS	50
+
+/* 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_cfg {
 	struct delayed_work go_cfg_setup;
-	struct completion send_cmd_complete;
+	struct hid_go_cmd cmd;
 	struct led_classdev *led_cdev;
 	struct hid_device *hdev;
 	struct mutex cfg_mutex; /*ensure single synchronous output report*/
@@ -331,6 +346,62 @@ static const char *const os_mode_text[] = {
 	[LINUX] = "linux",
 };
 
+static void hid_go_cmd_arm(u8 id, u8 command, u8 sub_command, u8 device)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	reinit_completion(&drvdata.cmd.done);
+	drvdata.cmd.pending = true;
+	drvdata.cmd.id = id;
+	drvdata.cmd.command = command;
+	drvdata.cmd.sub_command = sub_command;
+	drvdata.cmd.device = device;
+}
+
+static void hid_go_cmd_consume(const struct command_report *cmd_rep, int result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	if (drvdata.cmd.pending && cmd_rep->id == drvdata.cmd.id &&
+	    cmd_rep->cmd == drvdata.cmd.command &&
+	    cmd_rep->sub_cmd == drvdata.cmd.sub_command &&
+	    cmd_rep->device_type == drvdata.cmd.device) {
+		drvdata.cmd.pending = false;
+		drvdata.cmd.result = result;
+		complete(&drvdata.cmd.done);
+	}
+}
+
+static int hid_go_cmd_finish(long wait_result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	if (wait_result <= 0) {
+		drvdata.cmd.pending = false;
+		return wait_result < 0 ? wait_result : -ETIMEDOUT;
+	}
+
+	return drvdata.cmd.result;
+}
+
+static int hid_go_cmd_cancel(int result)
+{
+	guard(spinlock_irqsave)(&drvdata.cmd.lock);
+
+	drvdata.cmd.pending = false;
+	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) {
@@ -654,7 +725,7 @@ static int hid_go_raw_event(struct hid_device *hdev, struct hid_report *report,
 	struct command_report *cmd_rep;
 	int ep, ret;
 
-	if (size != GO_PACKET_SIZE)
+	if (size != GO_PACKET_SIZE || data[0] != GO_INPUT_REPORT_ID)
 		goto passthrough;
 
 	ep = get_endpoint_address(hdev);
@@ -707,7 +778,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:
@@ -722,7 +793,8 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
 	unsigned char *dmabuf __free(kfree) = NULL;
 	u8 header[] = { GO_OUTPUT_REPORT_ID, id, command, index, device };
 	size_t header_size = ARRAY_SIZE(header);
-	int timeout = 50;
+	unsigned long timeout = msecs_to_jiffies(GO_COMMAND_TIMEOUT_MS);
+	long wait_result;
 	int ret;
 
 	if (header_size + len > GO_PACKET_SIZE)
@@ -740,22 +812,14 @@ 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;
-
-	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;
+		return hid_go_cmd_cancel(ret);
 
-	reinit_completion(&drvdata.send_cmd_complete);
-	return 0;
+	wait_result = wait_for_completion_interruptible_timeout(&drvdata.cmd.done,
+								timeout);
+	return hid_go_cmd_finish(wait_result);
 }
 
 static ssize_t version_show(struct device *dev, struct device_attribute *attr,
@@ -2362,9 +2426,9 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
 	if (!buf)
 		return -ENOMEM;
 
+	mutex_init(&drvdata.cfg_mutex);
 	hid_set_drvdata(hdev, &drvdata);
 	drvdata.hdev = hdev;
-	mutex_init(&drvdata.cfg_mutex);
 
 	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
 	if (ret) {
@@ -2435,8 +2499,11 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	ep = get_endpoint_address(hdev);
-	if (ep == GO_GP_INTF_IN)
-		init_completion(&drvdata.send_cmd_complete);
+	if (ep == GO_GP_INTF_IN) {
+		init_completion(&drvdata.cmd.done);
+		spin_lock_init(&drvdata.cmd.lock);
+		drvdata.cmd.pending = false;
+	}
 
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
-- 
2.55.0

Re: [PATCH v3 4/7] HID: hid-lenovo-go: return configuration request errors

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver uses a static global `drvdata` structure for per-device state, causing state corruption across multiple devices.
- [High] AB-BA deadlock between `hid_go_cfg_remove()` and sysfs attributes due to inverse lock ordering with `cfg_mutex`.
- [High] Missing cleanup on `hid_go_probe()` and `hid_go_cfg_probe()` error paths leaks hardware state and sysfs groups.
- [High] Missing hardware stop and close in `hid_go_remove()` if the device has no valid endpoints.
--

commit 8f2d8679bb969b5284e4834a5d944cc1e08f97e3
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: return configuration request errors

This patch adds pending command state tracking to the driver data to match
replies by command ID, subcommand, and device. It also returns request errors
to the caller and clears pending requests on interruption or timeout.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 51e3112780e18..deed7acd2dc4b 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

> @@ -2362,9 +2426,9 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	mutex_init(&drvdata.cfg_mutex);
>  	hid_set_drvdata(hdev, &drvdata);
>  	drvdata.hdev = hdev;
> -	mutex_init(&drvdata.cfg_mutex);

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

[ ... ]

> @@ -2435,8 +2499,11 @@ static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	}
>  
>  	ep = get_endpoint_address(hdev);
> -	if (ep == GO_GP_INTF_IN)
> -		init_completion(&drvdata.send_cmd_complete);
> +	if (ep == GO_GP_INTF_IN) {
> +		init_completion(&drvdata.cmd.done);
> +		spin_lock_init(&drvdata.cmd.lock);
> +		drvdata.cmd.pending = false;
> +	}

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

>  
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret) {

[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/cover.1788222001.git.mradityadash@gmail.com?part=4

[PATCH v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[Aditya Dash]

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

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

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

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index deed7acd2dc4..59b457683357 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -648,17 +648,22 @@ 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];
+
+	if (status > CAL_STAT_FAILURE)
+		status = 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;
@@ -667,13 +672,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 v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Global State Corruption and Race Conditions due to statically allocated `drvdata`
- [Critical] Failure to perform required hardware stops and sysfs cleanup in error paths and during removal
- [High] AB-BA deadlock between `cfg_mutex` and kernfs node active reference
- [High] Unprotected state modifications leading to TOCTOU out-of-bounds sysfs read
- [High] LED `brightness_set` callback sleeps in atomic context
--

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

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

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

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index deed7acd2dc4b..59b457683357e 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -648,17 +648,22 @@ 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];
> +
> +	if (status > CAL_STAT_FAILURE)
> +		status = 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;
> @@ -667,13 +672,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;

[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/cover.1788222001.git.mradityadash@gmail.com?part=5

Re: [PATCH v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[Derek John Clark]

On Mon, Aug 31, 2026 at 11:36 PM Aditya Dash <mradityadash@gmail.com> wrote:
>
> 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.

Hi Aditya,

The 0x08 thing seemed odd to me and I meant to dig into the docs sooner.
I think the real reason for the bug you were seeing is a mixup of what data
bytet means what. There is an off by 1 error with using data[1] since
device_type is really the 0 byte for the information payload. data[1] is the
reason code for the failure.

From the docs:
Byte 0: Device Type:
    0x01: Gyroscope
    0x02: Joystick
    0x03: Trigger
    0x04: Joystick Trigger (Currently Unused)
Byte 1: Calibration Status:
    0x01: Calibration Successful
    0x02: Calibration Failed
Byte 2-3: Failure Reason:
Gyro:
    0x0001 - Not stationary, timeout
    0x0002 - Connection status changed
Joystick:
    0x0100 - Joystick not fully deflected
    0x0200 - Joystick not centered
    0x0400 - Joystick not rotated two turns
    0x0800 - Connection status changed
Trigger:
    0x0001 - Trigger not fully depressed
    0x0002 - Trigger not returned to starting position
    0x0004 - Trigger not pressed twice
    0x0008 - Connection status changed


Since we're matching on cmd_rep->device_type(byte 0 corollary) I think
switching to data[0] is sufficient to resolve the bug. If my hunch is
correct a trigger and gyro calibration will always succeed in the
driver as it stands. I won't be able to validate this myself for a
couple of hours on actual hardware, but I'll respond after I do.

Thanks,
Derek


> 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 | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index deed7acd2dc4..59b457683357 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -648,17 +648,22 @@ 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];
> +
> +       if (status > CAL_STAT_FAILURE)
> +               status = 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;
> @@ -667,13 +672,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 v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[Aditya]

Hi Derek,

Just to clarify, this behaviour was seen when sending a left
calibration joystick stop command when no calibration was
being performed.

Sending a respective start command returns after 10 seconds
with the error codes you have provided.

Here are the exact requests and responses. Consider ellipsis
to mean that the rest of it is zeros:

Left joystick calibration start:
Request:
    05 00 0c 04 03 01 ...
Response (after 10s, no movement):
    04 00 a0 02 03 02 02 07 ...

Left joystick calibration stop (when no calibration is running):
Request:
    05 00 0c 04 03 02 ...
Response (this is near immediate):
    04 00 a0 02 03 02 08 00 ...

Thanks,
Aditya

Re: [PATCH v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[Derek J. Clark]

On September 1, 2026 10:06:18 AM PDT, Aditya <mradityadash@gmail.com> wrote:
>Hi Derek,
>
>Just to clarify, this behaviour was seen when sending a left
>calibration joystick stop command when no calibration was
>being performed.
>
>Sending a respective start command returns after 10 seconds
>with the error codes you have provided.
>
>Here are the exact requests and responses. Consider ellipsis
>to mean that the rest of it is zeros:
>
>Left joystick calibration start:
>Request:
>    05 00 0c 04 03 01 ...
>Response (after 10s, no movement):
>    04 00 a0 02 03 02 02 07 ...
This makes it look like the reason code is a bitmask rather than a specific value. Probably more difficult to present in sysfs than the value it provides. 

> Left joystick calibration stop (when no calibration is running):
>Request:
>    05 00 0c 04 03 02 ...
>Response (this is near immediate):
>    04 00 a0 02 03 02 08 00 ...

Interesting. This wouldn't be the first time documentation was incomplete or inaccurate from Lenovo so I'm not super surprised. We can then consider a 0x08 response on the status byte as stopped with no call running, vice failed, if you think that information is relevant to the end user. I think it might be. If you stop it with a cal running is there a meaningful difference in the response? 

Thanks, 
Derek

>Thanks,
>Aditya

Re: [PATCH v3 5/7] HID: hid-lenovo-go: normalize calibration failure status

[Aditya Dash]

Hi Derek,

The Stop reply changes based on calibration state. Its status appears to be
the current failure_mask + 1. However, this mask does not always match the
failure mask returned when calibration times out.

Here are some requests and responses from the experiments done:

Joystick calibration:

Left Start:
  05 00 0c 04 03 01 ...

Left Stop:
  05 00 0c 04 03 02 ...

No movement:

Without Stop:
  04 00 a0 02 03 02 02 07 00 ...

With Stop:
  04 00 a0 02 03 02 06 00 00 ...

The timeout error contains deflection 0x01, center 0x02, and turns 0x04.
Stop does not set the center bit, giving mask 0x05 and status 0x06.

One full outer turn, returned to center:

Without Stop:
  04 00 a0 02 03 02 02 06 00 ...

With Stop:
  04 00 a0 02 03 02 05 00 00 ...

The timeout error contains center 0x02 and turns 0x04. Stop does not set the
center bit, giving mask 0x04 and status 0x05.

One full outer turn, then held away from center (right controller):

Without Stop:
  04 00 a0 02 04 02 02 06 00 ...

With Stop:
  04 00 a0 02 04 02 07 00 00 ...

Two full outer turns, then held away from center:

Without Stop:
  04 00 a0 02 03 02 02 02 00 ...

With Stop:
  04 00 a0 02 03 02 03 00 00 ...

Stop with no calibration running:
  04 00 a0 02 03 02 08 00 00 ...

Trigger calibration:

Start:
  05 00 0a 04 03 01 ...

Stop:
  05 00 0a 04 03 02 ...

One full press and release:

Without Stop:
  04 00 a0 02 03 03 02 00 06 ...

With Stop:
  04 00 a0 02 03 03 05 00 00 ...

The nonzero timeout reason byte is 0x06: return 0x02 and press count 0x04.
Stop does not set the return bit, giving mask 0x04 and status 0x05.

Gyroscope calibration:

Start:
  05 00 0e 06 03 01 ...

Stop:
  05 00 0e 06 03 02 ...

Continuous movement:

Without Stop:
  04 00 a0 02 03 01 02 01 00 ...

With Stop:
  04 00 a0 02 03 01 02 02 00 ...

The reason changes from 0x0001 to 0x0002 although no connection change was
made. The cause of this discrepancy is not known.

Successful calibration responses were as expected. The corresponding cases
produced the same outcomes on the other controller where repeated.

Connection-change cases are not yet tested.

Thanks,
Aditya

[PATCH v3 6/7] HID: hid-lenovo-go: reject unknown calibration action

[Aditya Dash]

The calibration action table includes 'unknown' at index zero. The options
attribute skips that entry and lists only 'start' and 'stop', but the store
searches the full table. Writing 'unknown' sends a request without an
action byte.

Reject CAL_UNKNOWN before building the request. Start and Stop both use a
one-byte action, so pass that size directly.

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 | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index 59b457683357..a6dec5490d17 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -1412,20 +1412,18 @@ static ssize_t calibrate_config_store(struct device *dev,
 				      const char *buf, u8 cmd, u8 sub_cmd,
 				      size_t count, enum dev_type device_type)
 {
-	size_t size = 1;
-	u8 val = 0;
+	u8 val;
 	int ret;
 
 	ret = sysfs_match_string(cal_enabled_text, buf);
 	if (ret < 0)
 		return ret;
+	if (ret == CAL_UNKNOWN)
+		return -EINVAL;
 
 	val = ret;
-	if (!val)
-		size = 0;
-
 	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, cmd, sub_cmd,
-			       device_type, &val, size);
+			       device_type, &val, sizeof(val));
 	if (ret < 0)
 		return ret;
 
-- 
2.55.0

Re: [PATCH v3 6/7] HID: hid-lenovo-go: reject unknown calibration action

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver uses a static global singleton `drvdata` structure for per-device context, breaking support for multiple devices.
- [High] Missing HID hardware cleanup in `hid_go_probe` error path.
- [High] Sysfs attribute groups are permanently leaked in the `hid_go_cfg_probe()` error paths.
- [High] Early return in `hid_go_remove()` on endpoint lookup failure skips crucial teardown steps.
- [High] Improper workqueue error handling causes a Use-After-Free if `schedule_delayed_work()` returns 0.
- [High] Uninitialized `rgb_profile` leads to an out-of-bounds firmware command configuration payload.
- [High] Incomplete fix: identical logical vulnerability exists in `feature_status_store` and `motor_config_store` causing malformed payloads.
--

commit ba76c1361f7d941a3336995df53ed1361e960284
Author: Aditya Dash <mradityadash@gmail.com>
Date:   Tue Sep 1 06:36:07 2026 +0000

    HID: hid-lenovo-go: reject unknown calibration action
    
    The calibration action table includes 'unknown' at index zero. The options
    attribute skips that entry and lists only 'start' and 'stop', but the store
    searches the full table. Writing 'unknown' sends a request without an
    action byte.
    
    Reject CAL_UNKNOWN before building the request. Start and Stop both use a
    one-byte action, so pass that size directly.
    
    Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
    Assisted-by: Pi:gpt-5.6-sol
    Signed-off-by: Aditya Dash <mradityadash@gmail.com>

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 59b457683357e..a6dec5490d179 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1412,20 +1412,18 @@ static ssize_t calibrate_config_store(struct device *dev,
>  				      const char *buf, u8 cmd, u8 sub_cmd,
>  				      size_t count, enum dev_type device_type)
>  {
> -	size_t size = 1;
> -	u8 val = 0;
> +	u8 val;
>  	int ret;
>  
>  	ret = sysfs_match_string(cal_enabled_text, buf);
>  	if (ret < 0)
>  		return ret;
> +	if (ret == CAL_UNKNOWN)
> +		return -EINVAL;
>  
>  	val = ret;
> -	if (!val)
> -		size = 0;
> -
>  	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, cmd, sub_cmd,
> -			       device_type, &val, size);
> +			       device_type, &val, sizeof(val));
>  	if (ret < 0)
>  		return ret;

[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/cover.1788222001.git.mradityadash@gmail.com?part=6

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

[Aditya Dash]

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.

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 | 38 +++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
index a6dec5490d17..749536e19d2d 100644
--- a/drivers/hid/hid-lenovo-go.c
+++ b/drivers/hid/hid-lenovo-go.c
@@ -111,6 +111,11 @@ struct go_cfg_attr {
 	u8 index;
 };
 
+struct go_cal_cfg_attr {
+	u8 index;
+	u8 *status;
+};
+
 struct command_report {
 	u8 report_id;
 	u8 id;
@@ -1410,7 +1415,8 @@ static ssize_t device_status_show(struct device *dev,
 static ssize_t calibrate_config_store(struct device *dev,
 				      struct device_attribute *attr,
 				      const char *buf, u8 cmd, u8 sub_cmd,
-				      size_t count, enum dev_type device_type)
+				      size_t count, enum dev_type device_type,
+				      u8 *cal_status)
 {
 	u8 val;
 	int ret;
@@ -1422,6 +1428,9 @@ static ssize_t calibrate_config_store(struct device *dev,
 		return -EINVAL;
 
 	val = ret;
+	if (val == CAL_START)
+		*cal_status = CAL_STAT_UNKNOWN;
+
 	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, cmd, sub_cmd,
 			       device_type, &val, sizeof(val));
 	if (ret < 0)
@@ -1818,7 +1827,8 @@ static void hid_go_brightness_set(struct led_classdev *led_cdev,
 				     const char *buf, size_t count)           \
 	{                                                                     \
 		return calibrate_config_store(dev, attr, buf, _name.index,    \
-					      _scmd, count, _dtype);          \
+					      _scmd, count, _dtype,           \
+					      _name.status);                   \
 	}                                                                     \
 	static ssize_t _name##_##_rtype##_show(                               \
 		struct device *dev, struct device_attribute *attr, char *buf) \
@@ -1978,17 +1988,23 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
 static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
 			    "rumble_notification_index");
 
-static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
+static struct go_cal_cfg_attr cal_trigg_left = {
+	SET_TRIGGER_CFG, &drvdata.gp_left_trigg_cal_status
+};
 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 = { SET_JOYSTICK_CFG };
+static struct go_cal_cfg_attr cal_joy_left = {
+	SET_JOYSTICK_CFG, &drvdata.gp_left_joy_cal_status
+};
 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 = { SET_GYRO_CFG };
+static struct go_cal_cfg_attr cal_gyro_left = {
+	SET_GYRO_CFG, &drvdata.gp_left_gyro_cal_status
+};
 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");
@@ -2085,17 +2101,23 @@ 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 = { SET_TRIGGER_CFG };
+static struct go_cal_cfg_attr cal_trigg_right = {
+	SET_TRIGGER_CFG, &drvdata.gp_right_trigg_cal_status
+};
 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 = { SET_JOYSTICK_CFG };
+static struct go_cal_cfg_attr cal_joy_right = {
+	SET_JOYSTICK_CFG, &drvdata.gp_right_joy_cal_status
+};
 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 = { SET_GYRO_CFG };
+static struct go_cal_cfg_attr cal_gyro_right = {
+	SET_GYRO_CFG, &drvdata.gp_right_gyro_cal_status
+};
 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 v3 7/7] HID: hid-lenovo-go: clear calibration status on start

[sashiko-bot]

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

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

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

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

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

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

[ ... ]

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

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

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

[ ... ]

> @@ -1978,17 +1988,23 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
>  static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
>  			    "rumble_notification_index");
>  
> -static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
> +static struct go_cal_cfg_attr cal_trigg_left = {
> +	SET_TRIGGER_CFG, &drvdata.gp_left_trigg_cal_status
> +};

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

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


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

drivers/hid/hid-lenovo-go.c:hid_go_probe() {
    ...
	ret = hid_go_cfg_probe(hdev, id);
	if (ret)
		dev_err_probe(&hdev->dev, ret, "Failed to start configuration interface\n");

	dev_dbg(&hdev->dev, "Started Legion Go HID Device: %x\n", ep);

	return ret;
}

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

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

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

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

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

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

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

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

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

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
    ...
	guard(mutex)(&drvdata.cfg_mutex);
	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
    ...
}

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

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

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

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

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

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

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

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