Re: [PATCH 3/5] HID: hid-lenovo-go: record calibration completion state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dangling pointer read when USB interface has 0 endpoints.
- [Critical] Driver uses a single global struct for device-specific state.
- [High] Missing `hid_hw_stop()` and resource cleanup in `probe()` error paths.
- [High] Early return in `remove()` omits mandatory `hid_hw_stop()`.
- [High] LED unregistration sends hardware commands after `hid_hw_stop()` has been called.
- [High] Delayed work is not cancelled on probe error path, risking Use-After-Free.
- [High] AB-BA deadlock during device removal when cleaning up sysfs groups.
- [High] AB-BA deadlock in `hid_gos_cfg_remove()` due to incorrect mutex locking order.
--

--- Patch [3]: [PATCH 3/5] HID: hid-lenovo-go: record calibration completion state ---
commit 0c0b87dd6928291724bdcbfc25cdec4b399b857c
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: record calibration completion state

The firmware uses result value 0 for failure and 1 for success. The driver
stores this value directly as a state index, mapping 0 to unknown. Map the
result value to the matching failure or success state.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index a6e9a130ad417..d2d134873dd1f 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -577,17 +577,19 @@ 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] ? CAL_STAT_SUCCESS : 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;

[Severity: Critical]
This is a pre-existing issue, but might using a single global struct for
device-specific state cause cross-device state corruption?

When multiple compatible devices are plugged in, their probes concurrently
overwrite drvdata.hdev, reinitialize the same cfg_mutex, and queue the same
delayed work:

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    hid_set_drvdata(hdev, &drvdata);
    drvdata.hdev = hdev;
    mutex_init(&drvdata.cfg_mutex);
    ...
}

Can this be easily triggered by physically connecting two Lenovo Go controllers
or via a crafted USB device presenting multiple matching interfaces?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could the probe function be
returning an error code without stopping the hardware if configuration fails?

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 to create the RGB device, it returns an error:

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    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;
    }
    ...
}

Does failing to clean up the manually allocated sysfs groups on this error path
leak them? Also, does this leave the hardware running without a bound driver,
which could lead to a use-after-free when events are processed?


[Severity: High]
This is a pre-existing issue, but is the mandatory hardware stop and teardown
bypassed by the early return in the remove function?

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

    if (ep <= 0)
        return;

    switch (ep) {
    ...
}

If get_endpoint_address() returns <= 0, the mandatory hardware stop and teardown
are bypassed. Since probe() successfully starts the hardware for such devices,
does this leave the HID device active after unbind, causing a use-after-free
when devres releases memory?


[Severity: Critical]
This is a pre-existing issue, but are we risking an out-of-bounds memory access
in get_endpoint_address() if the USB interface has 0 endpoints?

drivers/hid/hid-lenovo-go.c:get_endpoint_address() {
    ...
    ep = &intf->cur_altsetting->endpoint[0];
    if (!ep)
        return -EINVAL;
    ...
}

If a USB device declares 0 endpoints, the USB core allocates a 0-length endpoint
array. Does accessing the first endpoint here without validating bNumEndpoints
lead to an out-of-bounds memory access?


[Severity: High]
This is a pre-existing issue, but will LED unregistration attempt to send
hardware commands after the hardware has been stopped?

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

Since hid_hw_stop() is called here before devres automatically unregisters the
LED, the subsystem will issue a turn-off command via hid_go_brightness_set().
Does this attempt to send an output report to the already-stopped device,
causing transport driver errors or unbind stalls?


[Severity: High]
This isn't a bug introduced by this patch, but are we leaving delayed work
active in the probe error path?

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, the driver treats this as an error and
returns -ENODEV directly. Devres frees the device resources, but the delayed
work is never cancelled. Does this lead to a use-after-free when the dangling
delayed work handler executes?


[Severity: High]
This is a pre-existing issue, but could a lock inversion deadlock occur during
device removal when cleaning up sysfs groups?

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

hid_go_cfg_remove() acquires cfg_mutex and then waits for sysfs operations to
drain. Concurrently, a sysfs write acquires the active kernfs reference first
and then attempts to acquire the lock:

drivers/hid/hid-lenovo-go.c:mcu_property_out() {
    ...
    guard(mutex)(&drvdata.cfg_mutex);
    ...
}

Could a local user with sysfs write privileges trigger this deadlock during a
device unplug event?


[Severity: High]
This isn't a bug introduced by this patch, but does a similar lock inversion
deadlock exist in the companion driver?

drivers/hid/hid-lenovo-go-s.c:hid_gos_cfg_remove() {
    ...
    guard(mutex)(&drvdata.cfg_mutex);
    cancel_delayed_work_sync(&drvdata.gos_cfg_setup);
    ...
}

The cfg_mutex is acquired before cancel_delayed_work_sync() is called. If the
worker is running, it will attempt to call mcu_property_out(), which blocks on
acquiring the same cfg_mutex. Could this cause a deadlock during device unbind?

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