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

[sashiko-bot@kernel.org]

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

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

commit 087e7a307d94a2ccdf00bf5d59ce9cddc609d002
Author: Aditya Dash <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 ...

> 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
[ ... ]
> 	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 in hid_go_probe() leak
hardware resources?

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

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

> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

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

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

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

> 	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 this error path in hid_go_cfg_probe()
leak sysfs attributes?

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

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

> 	u32 tx_dongle_version_hardware;
> 	u32 tx_dongle_version_product;
> 	u32 tx_dongle_version_protocol;
> } drvdata;
[ ... ]
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;
> 	mutex_init(&drvdata.cfg_mutex);

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

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

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

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

> static void hid_go_cfg_remove(struct hid_device *hdev)
> {
[ ... ]
> 	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
> 
> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);
> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);

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

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

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

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

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