Re: [PATCH v2 2/6] HID: hid-lenovo-go: use the right controller selectors

[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 probe error paths.
- [High] Missing hardware cleanup in remove() due to early return.
- [High] Global singleton used for per-device state, causing corruption and Use-After-Free with multiple devices.
- [High] Leaked sysfs attribute groups on probe failure.
- [High] Deadlock during device unbind due to lock ordering with sysfs_remove_groups.
--

commit c2d6b2210c5ca6d1fa356349f3cbde71b00ce595
Author: Aditya Dash <mradityadash@gmail.com>
Subject: HID: hid-lenovo-go: use the right controller selectors

This commit fixes the attribute selectors for the right controller on Lenovo
Go gamepads. It updates the imu_enabled_right attribute to select
FEATURE_IMU_ENABLE and ensures the reset_right attribute targets the right
controller.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index b8248bf8965c6..b937d11dd6f6a 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1999,13 +1999,13 @@ LEGO_DEVICE_ATTR_RW(imu_bypass_right, "imu_bypass_enabled", RIGHT_CONTROLLER,

[ ... ]

> 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 global singleton for instance-
specific state break the driver model if multiple devices are plugged in?

Since drvdata is globally allocated in drivers/hid/hid-lenovo-go.c, setting
drvdata.hdev here in hid_go_cfg_probe() will clobber the pointer from any
previously probed device.

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

[Severity: High]
This isn't a bug introduced by this patch, but does this corrupt the LED
subsystem lists?

Since go_cdev_rgb is a statically allocated global struct, registering the
same object in hid_go_cfg_probe() for every device instance could corrupt
the lists if multiple devices are present.

> 	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 this error path in hid_go_cfg_probe()
leave sysfs attributes behind?

If devm_led_classdev_multicolor_register() fails, this returns immediately
without calling sysfs_remove_groups(). While devres will free the driver's
memory, the sysfs attributes remain active and reference freed memory,
which could cause a use-after-free if userspace interacts with them.

> 	}

[ ... ]

> static void hid_go_cfg_remove(struct hid_device *hdev)
> {
> 	/*
> 	 * cfg_setup is scheduled from hid_go_cfg_probe() with a 2 ms delay
> 	 * and dereferences drvdata.hdev.  Drain it here before tearing
> 	 * down so the workqueue cannot run after hid_destroy_device()'s
> 	 * put_device() has released the underlying hdev and dereference
> 	 * a stale drvdata.hdev pointer.
> 	 */
> 	cancel_delayed_work_sync(&drvdata.go_cfg_setup);
> 
> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

[Severity: High]
This isn't a bug introduced by this patch, but can this lock ordering in
hid_go_cfg_remove() cause a deadlock during device unbind?

This holds drvdata.cfg_mutex while synchronously waiting for
sysfs_remove_groups() to finish. If userspace is concurrently writing to a
sysfs attribute, that write path (e.g., mcu_property_out()) will attempt
to acquire drvdata.cfg_mutex, leading to an ABBA deadlock that freezes the
kernel thread.

> 	hid_hw_close(hdev);
> 	hid_hw_stop(hdev);
> }

[ ... ]

> 	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;
> 	}
> 
> 	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 is a pre-existing issue, but does this error path in hid_go_probe()
fail to tear down the hardware?

If hid_go_cfg_probe() fails, the error code is immediately returned without
calling hid_hw_stop() or hid_hw_close(). The hardware is left running while
driver private data is freed by devres, which will trigger a use-after-free
when subsequent input events arrive.

> }
> 
> static void hid_go_remove(struct hid_device *hdev)
> {
> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

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

If get_endpoint_address() fails during device unplug, it hits this early
return and bypasses hid_hw_close() and hid_hw_stop(). Input interrupts
will keep firing into freed driver structures.

> 
> 	switch (ep) {
> 	case GO_GP_INTF_IN:
> 		hid_go_cfg_remove(hdev);
> 		break;

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