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

[sashiko-bot@kernel.org]

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