Re: [PATCH v3 6/7] HID: hid-lenovo-go: reject unknown calibration action

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver uses a static global singleton `drvdata` structure for per-device context, breaking support for multiple devices.
- [High] Missing HID hardware cleanup in `hid_go_probe` error path.
- [High] Sysfs attribute groups are permanently leaked in the `hid_go_cfg_probe()` error paths.
- [High] Early return in `hid_go_remove()` on endpoint lookup failure skips crucial teardown steps.
- [High] Improper workqueue error handling causes a Use-After-Free if `schedule_delayed_work()` returns 0.
- [High] Uninitialized `rgb_profile` leads to an out-of-bounds firmware command configuration payload.
- [High] Incomplete fix: identical logical vulnerability exists in `feature_status_store` and `motor_config_store` causing malformed payloads.
--

commit ba76c1361f7d941a3336995df53ed1361e960284
Author: Aditya Dash <mradityadash@gmail.com>
Date:   Tue Sep 1 06:36:07 2026 +0000

    HID: hid-lenovo-go: reject unknown calibration action
    
    The calibration action table includes 'unknown' at index zero. The options
    attribute skips that entry and lists only 'start' and 'stop', but the store
    searches the full table. Writing 'unknown' sends a request without an
    action byte.
    
    Reject CAL_UNKNOWN before building the request. Start and Stop both use a
    one-byte action, so pass that size directly.
    
    Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
    Assisted-by: Pi:gpt-5.6-sol
    Signed-off-by: Aditya Dash <mradityadash@gmail.com>

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 59b457683357e..a6dec5490d179 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1412,20 +1412,18 @@ static ssize_t calibrate_config_store(struct device *dev,
>  				      const char *buf, u8 cmd, u8 sub_cmd,
>  				      size_t count, enum dev_type device_type)
>  {
> -	size_t size = 1;
> -	u8 val = 0;
> +	u8 val;
>  	int ret;
>  
>  	ret = sysfs_match_string(cal_enabled_text, buf);
>  	if (ret < 0)
>  		return ret;
> +	if (ret == CAL_UNKNOWN)
> +		return -EINVAL;
>  
>  	val = ret;
> -	if (!val)
> -		size = 0;
> -
>  	ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA, cmd, sub_cmd,
> -			       device_type, &val, size);
> +			       device_type, &val, sizeof(val));
>  	if (ret < 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this identical logical vulnerability
still exist in feature_status_store() and motor_config_store()?

Both of those functions appear to use sysfs_match_string() with string tables
where 'unknown' is index 0. If a user writes 'unknown' to features like
imu_enabled, val will be 0, triggering size = 0, and sending a malformed
payload missing its action byte to the firmware:

drivers/hid/hid-lenovo-go.c:feature_status_store() {
    ...
        ret = sysfs_match_string(enabled_status_text, buf);
        val = ret;
        break;
    ...
    if (!val)
        size = 0;

    ret = mcu_property_out(drvdata.hdev, MCU_CONFIG_DATA,
                           SET_FEATURE_STATUS, index, device_type, &val,
                           size);
    ...
}

Could the same fix be applied to those functions?


[Severity: Critical]
This isn't a bug introduced by this patch, but does the driver use a static
global singleton drvdata structure for per-device context?

drivers/hid/hid-lenovo-go.c:
    ...
    } drvdata;
    ...

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

If multiple compatible devices are connected, would the active
synchronization primitives and workqueue list heads be unconditionally
re-initialized, potentially corrupting kernel linked lists and leading
to a use-after-free or deadlocks?


[Severity: High]
This is a pre-existing issue, but does the hid_go_probe() error path
miss hardware cleanup?

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, the function directly returns the error code.
Does this leave the hardware active since hid_hw_close() or hid_hw_stop()
are not called, potentially causing a use-after-free when hardware events
trigger on the freed device?


[Severity: High]
This isn't a bug introduced by this patch, but are sysfs attribute groups
permanently leaked in the hid_go_cfg_probe() error paths?

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
    ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
    if (ret) {
    ...
    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;
    }
    ...
}

Since sysfs_create_groups() is not managed by devres, if subsequent steps
like devm_led_classdev_multicolor_register() fail, the function returns
directly. Since probe() fails, remove() will not be called. Could this leave
permanently dangling sysfs nodes tied to the kobject that cause a
use-after-free if accessed?


[Severity: High]
This is a pre-existing issue, but does the early return in hid_go_remove()
on endpoint lookup failure skip crucial teardown steps?

drivers/hid/hid-lenovo-go.c:hid_go_remove() {
    ...
    if (ep <= 0)
        return;
    ...
}

If the interface has zero endpoints, get_endpoint_address() returns -ENODEV
and the early return bypasses all teardown logic. Does this omit
hid_hw_close() and hid_hw_stop(), permanently leaking the active device
node and creating a use-after-free condition?


[Severity: High]
This isn't a bug introduced by this patch, but does improper workqueue error
handling cause a use-after-free if schedule_delayed_work() returns 0?

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 (meaning the work was already pending),
the driver treats this as a fatal failure and returns -ENODEV. Since probe()
fails, the HID core and devres will destroy the device and free hdev.
Because the work is still pending and never canceled, when the timer expires,
will cfg_setup() execute and dereference the freed drvdata.hdev?


[Severity: High]
This is a pre-existing issue, but can an uninitialized rgb_profile lead to
an out-of-bounds firmware command configuration payload?

drivers/hid/hid-lenovo-go.c:rgb_effect_store() {
    ...
    index = drvdata.rgb_profile + 3;
    u8 rgb_profile[6] = { effect,
                  mc_cdev->subled_info[0].intensity,
    ...
    ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
    ...
}

drvdata.rgb_profile is statically initialized to 0. If a user writes to the
rgb_effect or rgb_speed sysfs attributes before setting or reading
rgb_profile, index evaluates to 3 (corresponding to LIGHT_PROFILE_SEL).

This executes rgb_cfg_call() with a 6-byte payload targeting
LIGHT_PROFILE_SEL. If the firmware natively expects LIGHT_PROFILE_SEL to
receive a 1-byte payload, could sending a 6-byte profile array cause an
out-of-bounds write that crashes or corrupts the device firmware?

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