Re: [PATCH v3 1/7] HID: hid-lenovo-go: use the correct calibration commands

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in hid_go_probe() error path.
- [High] Sysfs attribute groups are leaked on error paths in hid_go_cfg_probe(), leading to an Execute-After-Free panic.
- [High] hid_go_remove fails to stop and close the HID hardware if get_endpoint_address returns a negative value.
- [High] Accessing the USB endpoint array without verifying the endpoint count can lead to an out-of-bounds read.
- [High] The driver uses a statically allocated global variable for device private data, breaking support for multiple connected devices.
--

commit 0f59f6a616b3448c35bd0ae030452c8605182e3d
Author: Aditya Dash <mradityadash@gmail.com>

    HID: hid-lenovo-go: use the correct calibration commands
    
    The calibration attributes pass the calibration operation as the command
    and the configuration command as the sub-command. This sends each request
    with the two fields reversed.
    
    Swap the command and sub-command values for all six calibration attributes.
    For example, the malformed left joystick Start request is:
    
      05 00 04 0c 03 01 ...
    
    The corrected request is:
    
      05 00 0c 04 03 01 ...
    
    The bytes identify the output report, configuration group, command,
    sub-command, left controller, and Start action, in that order.
    
    Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")
    Assisted-by: Pi:gpt-5.6-sol
    Signed-off-by: Aditya Dash <mradityadash@gmail.com>
    Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
    Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index 318b1152ff8ba..b8248bf8965c6 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1911,18 +1911,18 @@ LEGO_DEVICE_ATTR_RW(rumble_notification_left, "rumble_notification",
>  static DEVICE_ATTR_RO_NAMED(rumble_notification_left_index,
>  			    "rumble_notification_index");
>  
> -static struct go_cfg_attr cal_trigg_left = { TRIGGER_CALIBRATE };
> -LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", SET_TRIGGER_CFG,
> +static struct go_cfg_attr cal_trigg_left = { SET_TRIGGER_CFG };
> +LEGO_CAL_DEVICE_ATTR(cal_trigg_left, "calibrate_trigger", TRIGGER_CALIBRATE,
>  		     LEFT_CONTROLLER, index);

[ ... ]

While reviewing this code, I noticed a few pre-existing issues elsewhere in
the driver.

> static int get_endpoint_address(struct hid_device *hdev)
> {
> 	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> 	struct usb_host_endpoint *ep;
> 
> 	ep = intf->cur_altsetting->endpoint;
> 	if (!ep)
> 		return -ENODEV;
> 
> 	return ep->desc.bEndpointAddress;
> }
        ^^^^
[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
malicious USB device to provide an interface with zero endpoints?

If so, this code reads ep->desc.bEndpointAddress without verifying the
intf->cur_altsetting->desc.bNumEndpoints count. Does this need a check
to prevent an out-of-bounds read of the endpoint array?


> static int hid_go_cfg_probe(struct hid_device *hdev,
> 			    const struct hid_device_id *_id)
> {
> 	unsigned char *buf;
> 	int ret;
> 
> 	buf = devm_kzalloc(&hdev->dev, GO_PACKET_SIZE, GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;
> 	mutex_init(&drvdata.cfg_mutex);
        ^^^^
[Severity: High]
This is a pre-existing issue, but does using a single static 'drvdata'
struct break support for multiple connected devices?

If two devices are connected (or one exposes multiple interfaces), the
second probe will unconditionally overwrite the global drvdata.hdev
pointer and re-initialize the mutex. Would dynamically allocating this
state (e.g., via devm_kzalloc) prevent cross-device command routing
issues and deadlocks?

> 	ret = sysfs_create_groups(&hdev->dev.kobj, top_level_attr_groups);
> 	if (ret) {
> 		dev_err_probe(&hdev->dev, ret,
> 			      "Failed to create gamepad configuration attributes\n");
> 		return 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;
> 	}
        ^^^^
[Severity: High]
This is a pre-existing issue, but does the error path leak sysfs attributes?

If devm_led_classdev_multicolor_register() fails, the function returns
without calling sysfs_remove_groups(). If the driver unbinds, could reading
or writing the dangling sysfs files trigger an execute-after-free panic?


> static int hid_go_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> 	int ret, ep;
> 
> 	if (!hid_is_usb(hdev))
> 		return -EINVAL;
[ ... ]
> 	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;
> }
        ^^^^
[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave
the hardware and input nodes running?

If hid_go_cfg_probe() fails, the error code is returned directly without
calling hid_hw_close() and hid_hw_stop() to tear down what was started
earlier.

Since the HID core doesn't automatically stop the hardware if a custom probe
fails after hid_hw_start() succeeds, could this cause a use-after-free
if the module is unloaded while callbacks remain active?


> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;
        ^^^^
[Severity: High]
This is a pre-existing issue, but can this skip the required hardware
teardown when the driver is unbound?

If get_endpoint_address() returns a negative error code during unbind, the
function returns early. This skips hid_hw_close() and hid_hw_stop(), leaving
the hardware active.

Could this result in a use-after-free since the input_dev and hidraw
interfaces would remain active after the driver's resources are freed?

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