Re: [PATCH 2/5] HID: hid-lenovo-go: fix the FPS mode DPI request

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

New issues:
- [High] FPS mode DPI read requests and responses use incorrect byte offsets and headers.

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in the hid_go_probe() error path.
- [High] The driver uses a single global static variable drvdata for device-specific state.
- [High] Early return in hid_go_remove() bypasses hid_hw_stop() completely.
- [High] get_endpoint_address() lacks a boundary check for bNumEndpoints.
- [High] AB-BA deadlock between device removal and sysfs file access.
- [High] Passing a NULL pointer to memcpy() when processing GET requests.
- [Medium] Sysfs attribute groups are leaked on hid_go_cfg_probe() error paths.
--

--- Patch [2]: [PATCH 2/5] HID: hid-lenovo-go: fix the FPS mode DPI request ---
commit 9b984e3644e77c41adb68e3622677de0ae7cd145
Author: Aditya Dash <[email protected]>

HID: hid-lenovo-go: fix the FPS mode DPI request

mcu_property_out() adds the five-byte request header used by most
configuration commands. For FPS mode DPI writes, this inserts a device byte
before the four-byte DPI value.

The FPS mode DPI request does not contain a device byte. Its little-endian
DPI value starts immediately after the command and sub-command.

Use the four-byte header only for FPS mode DPI writes. Keep the common
header for all other requests.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..a6e9a130ad417 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -725,6 +725,11 @@ static int mcu_property_out(struct hid_device *hdev, u8 id, u8 command,
>  	int timeout = 50;
>  	int ret;
>  
> +	/* The FPS mode DPI request does not contain a device byte. */
> +	if (id == MCU_CONFIG_DATA && command == SET_DPI_CFG &&
> +	    index == FPS_MODE_DPI)
> +		header_size--;
> +

[Severity: High]
Does this change result in FPS mode DPI read requests and responses using
incorrect byte offsets and headers?

Since the check specifically targets SET_DPI_CFG, a GET_DPI_CFG request from
fps_mode_dpi_show() will still send a 5-byte header. hid_go_fps_dpi_event()
will then read the response from an incorrect byte offset due to asymmetric
header tracking.

>  	if (header_size + len > GO_PACKET_SIZE)
>  		return -EINVAL;
>  

While reviewing this patch, I also noticed a few preexisting issues in this
file.

[Severity: High]
This isn't a bug introduced by this patch, but does this invoke undefined
behavior by passing a NULL pointer to memcpy()?

drivers/hid/hid-lenovo-go.c:mcu_property_out()
    ...
    memcpy(dmabuf, header, header_size);
    memcpy(dmabuf + header_size, data, len);
    ...

When mcu_property_out() is called for GET requests (like GET_DPI_CFG in
fps_mode_dpi_show()), the data pointer is NULL and len is 0.


[Severity: High]
This isn't a bug introduced by this patch, but does using a single global
static variable drvdata cause multiple devices to corrupt each other's state?

drivers/hid/hid-lenovo-go.c
    static struct hid_go_cfg {
        ...
    } drvdata;

The global drvdata is mutated per-device and its mutex is re-initialized on
every probe, which could cause issues if more than one of these devices is
connected.


[Severity: High]
This isn't a bug introduced by this patch, but does the hid_go_probe() error
path miss calling hid_hw_stop() and hid_hw_close()?

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, this returns the error without tearing down the
hardware that was started earlier, which could lead to a resource leak and
use-after-free.


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

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

The function returns directly on error without calling sysfs_remove_groups()
to clean up the groups created earlier by sysfs_create_groups().


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

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

    if (ep <= 0)
        return;

If the endpoint is not found or is less than zero on disconnect, the early
return skips hid_hw_stop() and hid_hw_close(), leaving hardware active.


[Severity: High]
This isn't a bug introduced by this patch, but does get_endpoint_address() lack
a boundary check for bNumEndpoints?

drivers/hid/hid-lenovo-go.c:get_endpoint_address()
    ...
    ep = intf->cur_altsetting->endpoint;
    if (!ep)
        return -ENODEV;

    return ep->desc.bEndpointAddress;

Accessing ep->desc.bEndpointAddress directly without verifying that
intf->cur_altsetting->desc.bNumEndpoints > 0 could result in an out-of-bounds
read on the endpoint descriptor array.


[Severity: High]
This isn't a bug introduced by this patch, but is there an AB-BA deadlock
between device removal and sysfs file access here?

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 before sysfs_remove_groups(). If a
sysfs write is waiting on cfg_mutex while holding the kernfs active
reference, calling sysfs_remove_groups() while holding cfg_mutex will
deadlock.

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