Re: [PATCH 4/5] HID: hid-lenovo-go: use the right controller selectors

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

Pre-existing issues:
- [High] Missing cleanup in hid_go_probe() error paths.
- [High] Global state corruption from statically allocated per-device state.
- [High] AB-BA deadlock in hid_go_cfg_remove() with sysfs.
- [High] Early return in hid_go_remove() bypassing cleanup causes Use-After-Free.
--

--- Patch [4]: [PATCH 4/5] HID: hid-lenovo-go: use the right controller selectors ---
commit 21361c83148d2f3658bbae16a33bf294b6a2f9f0
Author: Aditya Dash <[email protected]>

    HID: hid-lenovo-go: use the right controller selectors
    
    The right imu_enabled attribute selects FEATURE_IMU_BYPASS, and the right
    reset attribute targets the left controller. Select FEATURE_IMU_ENABLE
    for the right imu_enabled attribute. Target the right controller for a
    right reset.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index d2d134873dd1f..1e6915c969d8c 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -2006,13 +2006,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,
>  		    index, feature_status);
>  static DEVICE_ATTR_RO_NAMED(imu_bypass_right_index, "imu_bypass_enabled_index");
>  
> -static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_BYPASS };
> +static struct go_cfg_attr imu_enabled_right = { FEATURE_IMU_ENABLE };
>  LEGO_DEVICE_ATTR_RW(imu_enabled_right, "imu_enabled", RIGHT_CONTROLLER, index,
>  		    feature_status);
>  static DEVICE_ATTR_RO_NAMED(imu_enabled_right_index, "imu_enabled_index");
>  
>  static struct go_cfg_attr reset_right = { FEATURE_RESET_GAMEPAD };
> -LEGO_DEVICE_ATTR_WO(reset_right, "reset", LEFT_CONTROLLER, feature_status);
> +LEGO_DEVICE_ATTR_WO(reset_right, "reset", RIGHT_CONTROLLER, feature_status);
>  
>  static struct go_cfg_attr rumble_mode_right = { RUMBLE_MODE };
>  LEGO_DEVICE_ATTR_RW(rumble_mode_right, "rumble_mode", RIGHT_CONTROLLER, index,

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the error
path in hid_go_probe() to leak resources?

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

If hid_go_cfg_probe() fails (e.g., due to sysfs_create_groups() returning
an error), it propagates the error and returns without calling
hid_hw_close() and hid_hw_stop().

Additionally, does hid_go_cfg_probe() leave sysfs attributes active if it
fails?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
    ...
}

Subsequent error paths in hid_go_cfg_probe() do not appear to clean up
these sysfs groups before returning. If the module is subsequently unloaded,
the un-removed sysfs attributes could remain on the device.

[Severity: High]
This is a pre-existing issue, but does using a static global struct for
device state cause corruption if multiple devices are probed?

drivers/hid/hid-lenovo-go.c: {
...
static struct hid_go_cfg {
    struct delayed_work go_cfg_setup;
    struct led_classdev *led_cdev;
    struct hid_device *hdev;
    struct mutex cfg_mutex;
//...
} drvdata;
...
}

When hid_go_cfg_probe() is called, it overwrites the hardware device pointer
and re-initializes the work item:

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

If a second gamepad is connected, could this overwrite drvdata.hdev, causing
sysfs requests for the first device to operate on the second? Also, could
calling INIT_DELAYED_WORK() while it is pending for the first device corrupt
the timer list entry?

[Severity: High]
This is a pre-existing issue, but is there a potential deadlock between
sysfs and the configuration mutex in hid_go_cfg_remove()?

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

sysfs_remove_groups() synchronously waits for active sysfs readers/writers
to drop their kernfs active references. Concurrently, a sysfs reader/writer
might invoke mcu_property_out(), which attempts to acquire
drvdata.cfg_mutex. Could this result in the remove thread holding the mutex
and waiting for sysfs, while the sysfs thread holds the active reference and
waits for the mutex?

[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
hid_go_remove() bypass cleanup?

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

    if (ep <= 0)
        return;
    ...
}

In hid_go_probe(), missing endpoints result in returning success (0),
leaving the hardware active:

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

If the device is later unplugged or unbound, hid_go_remove() returns early,
bypassing hid_hw_stop(). Could the HID hardware remain active while devres
frees the driver data, potentially allowing subsequent incoming hardware
events to access freed devres structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4