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

[sashiko-bot@kernel.org]

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