Re: [PATCH v2 3/6] HID: hid-lenovo-go: return configuration request errors

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added spinlock and completion are accessed by the raw event handler before they are initialized, causing a potential kernel crash.

Pre-existing issues:
- [High] Missing cleanup on probe failure leaves the hardware started, the device open, and sysfs groups leaked.
- [High] Missing hardware stop in remove for devices with no endpoints.
- [High] Global static `drvdata` structure causes state corruption and kernel panics when multiple devices are connected.
- [High] ABBA deadlock between device removal and sysfs attribute stores.
- [High] Possible out-of-bounds array access in get_endpoint_address if USB interface has no endpoints.
--

commit 0d711ce676d4cb403114e39243d7e14be625eec6
Author: Aditya Dash <mradityadash@gmail.com>
Subject: HID: hid-lenovo-go: return configuration request errors

This commit adds a mechanism to track pending command state and correctly
match configuration replies by command ID, sub-command, and device. It
ensures that unrelated replies, errors, timeouts, or interrupted waits are
properly reported as errors to the caller rather than silently returning
success.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b937d11dd6f6a..dbfacbb70394b 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -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;

[Severity: High]
This is a pre-existing issue, but the entire driver state is defined as a
single global variable instead of per-device driver data. Does this cause
state corruption or kernel panics when multiple supported devices are
connected simultaneously?

[ ... ]

> @@ -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)
> +{

[ ... ]

> +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 &&

[Severity: High]
Can hid_go_raw_event() be triggered by an early interrupt before
hid_go_cfg_probe() runs?

Since hid_hw_start() and hid_hw_open() are called during device probe,
the hardware can generate interrupts before hid_go_cfg_probe() initializes
this spinlock.

If an early interrupt arrives, it will call hid_go_raw_event()
and then hid_go_cmd_consume(), which locks drvdata.cmd.lock.

Locking an uninitialized spinlock will lead to lock corruption or kernel crashes.

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does get_endpoint_address() safely handle
USB interfaces with zero endpoints?

In get_endpoint_address():

    ep = intf->cur_altsetting->endpoint;
    if (!ep)
        return -ENODEV;
    return ep->desc.bEndpointAddress;

If a malformed USB interface reports 0 endpoints (bNumEndpoints == 0), the
endpoint array might be a 0-length array. The if (!ep) check only verifies
the pointer isn't NULL, but accessing ep->desc.bEndpointAddress on an empty
array could read memory out of bounds.

[ ... ]

> @@ -2362,9 +2426,12 @@ static int hid_go_cfg_probe(struct hid_device *hdev,
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	mutex_init(&drvdata.cfg_mutex);
> +	init_completion(&drvdata.cmd.done);
> +	spin_lock_init(&drvdata.cmd.lock);
> +	drvdata.cmd.pending = false;
>  	hid_set_drvdata(hdev, &drvdata);
>  	drvdata.hdev = hdev;
> -	mutex_init(&drvdata.cfg_mutex);

[Severity: High]
This is a pre-existing issue, but this unconditionally re-initializes global
synchronization primitives (and the newly added drvdata.cmd.lock and
drvdata.cmd.done).

If a second device is plugged in while the first is active, will this corrupt
the wait queues and lock states of the first device?

[ ... ]

> @@ -2388,8 +2455,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.
>  	 */

[Severity: High]
This is a pre-existing issue, but does failing here or later in hid_go_probe()
leave the hardware running and resources leaked?

If schedule_delayed_work() fails (or if hid_go_cfg_probe() fails earlier),
the error is returned but the error paths do not call hid_hw_stop(),
hid_hw_close(), or clean up the manually created sysfs groups.

This leaves the hardware in an active state and leaks resources upon device
removal.


I also noticed a few other pre-existing issues in the driver outside the scope
of this patch:

[Severity: High]
This is a pre-existing issue, but does hid_go_remove() safely tear down
the hardware for all devices?

In hid_go_remove():

    int ep = get_endpoint_address(hdev);

    if (ep <= 0)
        return;

If get_endpoint_address() fails and returns <= 0, the function immediately
returns without calling hid_hw_close() and hid_hw_stop(). This prevents
the driver from tearing down the hardware properly, leaving active interfaces
unrestored and leaking open counts when the device is unregistered.

[Severity: High]
This is a pre-existing issue, but can hid_go_cfg_remove() deadlock with
concurrent sysfs operations?

In hid_go_cfg_remove():

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

If userspace writes to a sysfs attribute, kernfs acquires an active reference
for the file and calls mcu_property_out(), which blocks waiting for
drvdata.cfg_mutex. During device removal, hid_go_cfg_remove() acquires
drvdata.cfg_mutex first and then calls sysfs_remove_groups(), which blocks
waiting for all active kernfs references to drain, causing an ABBA deadlock.

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