Re: [PATCH v2 5/6] 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:

New issues:
- [Low] The patch leaves behind dead code (`if (!val) size = 0;`) after explicitly rejecting `CAL_UNKNOWN`.

Pre-existing issues:
- [Critical] The driver incorrectly uses static global structs to store per-device state.
- [High] Missing hid_hw_close() and hid_hw_stop() in hid_go_probe() error paths.
- [High] Early return in hid_go_remove() bypasses hid_hw_stop() cleanup.
- [High] Missing sysfs_remove_groups() on the error paths in hid_go_cfg_probe().
- [High] AB-BA deadlock in hid_go_cfg_remove() with sysfs operations.
- [High] LED brightness_set callback performs sleeping operations.
--

commit a78ed68c3272ef95327340abb6446f933e2d3711
Author: Aditya Dash <mradityadash@gmail.com>
HID: hid-lenovo-go: reject unknown calibration action

This patch explicitly rejects CAL_UNKNOWN in the store handler for the
calibration configuration. It prevents requests without an action byte from
being sent by returning -EINVAL before building the request.

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index c7a2e621a4ade..a4231d188b7af 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c
> @@ -1419,6 +1419,8 @@ static ssize_t calibrate_config_store(struct device *dev,
>  	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;

[Severity: Low]
Does this change leave behind dead code? Since CAL_UNKNOWN maps to 0 and is now
explicitly rejected above, the subsequent assignment val = ret guarantees that
val is non-zero.

Because of this, if (!val) will evaluate to false under all possible
conditions, making size = 0 dead code.

[ ... ]

> 	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 regression introduced by this patch, but can this error path lead
to a resource leak and potential use-after-free?

In hid_go_probe(), if hid_go_cfg_probe() returns an error, the function
immediately returns the error without rolling back prior successful calls to
hid_hw_start() and hid_hw_open(). This leaves the device in an active hardware
state with input interfaces registered, violating HID subsystem lifecycle rules.

[ ... ]

> 	int ep = get_endpoint_address(hdev);
> 
> 	if (ep <= 0)
> 		return;

[Severity: High]
This isn't a regression introduced by this patch, but does this early return
bypass the default case needed to clean up generic HID devices?

If a device without the expected endpoint is unbound, get_endpoint_address()
returns an error (< 0) and this early return in hid_go_remove() triggers.
Omitting hid_hw_stop() here leaves the HID hardware, input devices, and hidraw
nodes active and registered after the driver has unbound.

When userspace interacts with these lingering nodes, it will access freed devres
data, causing a use-after-free.

[ ... ]

> static struct hid_go_cfg {
> ... 
> } drvdata;

[ ... ]

> static struct led_classdev_mc go_cdev_rgb = {

[ ... ]

> 	mutex_init(&drvdata.cfg_mutex);
> 	init_completion(&drvdata.cmd.done);
> 	spin_lock_init(&drvdata.cmd.lock);
> 	drvdata.cmd.pending = false;
> 	hid_set_drvdata(hdev, &drvdata);
> 	drvdata.hdev = hdev;

[Severity: Critical]
This is a pre-existing issue, but does this incorrectly use static global
structs to store per-device state?

If multiple supported devices are plugged in simultaneously, hid_go_cfg_probe()
is called multiple times and unconditionally re-initializes the global
drvdata.cfg_mutex, drvdata.cmd.done, and drvdata.cmd.lock, and overwrites
drvdata.hdev.

Re-initializing active synchronization primitives corrupts lockdep and
waitqueues, leading to kernel panics. Additionally, registering the same static
global led_classdev_mc multiple times corrupts the LED subsystem's linked
lists, and sharing drvdata.hdev means one device's interrupts or sysfs
operations will randomly operate on the other device.

[ ... ]

> 	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 isn't a regression introduced by this patch, but does this error path
permanently leak sysfs attributes?

Earlier in hid_go_cfg_probe(), sysfs_create_groups() is called to create
attributes. If devm_led_classdev_multicolor_register() fails here, the
function returns the error directly without calling sysfs_remove_groups().

This leaves orphaned sysfs files on the device's kobject. If accessed, they
will reference uninitialized or invalid driver state, and reloading or re-binding
the driver will fail because the files already exist.

[ ... ]

> 	guard(mutex)(&drvdata.cfg_mutex);
> 	sysfs_remove_groups(&hdev->dev.kobj, top_level_attr_groups);

[Severity: High]
This is a pre-existing issue, but can this cause an AB-BA deadlock?

In hid_go_cfg_remove(), the driver acquires drvdata.cfg_mutex and then calls
sysfs_remove_groups(), which blocks waiting for the kernfs active reference
to drain.

Concurrently, any sysfs read/write handler (like feature_status_show()) first
acquires the kernfs active reference, and then calls mcu_property_out(), which
blocks trying to acquire drvdata.cfg_mutex. This forms an AB-BA deadlock that
will hang driver removal and userspace sysfs access.

[ ... ]

> 		.brightness_set = hid_go_brightness_set,

[ ... ]

> 	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);

[Severity: High]
This is a pre-existing issue, but does this callback perform sleeping operations
in an atomic context?

The driver assigns hid_go_brightness_set to the .brightness_set field. The LED
subsystem invokes this callback directly from atomic contexts, such as timer
interrupts for the timer trigger.

However, hid_go_brightness_set() calls mcu_property_out(), which acquires a
mutex, allocates memory with GFP_KERNEL, and sleeps on a completion timeout.
Triggering this callback from an atomic context will cause a panic. The driver
must use .brightness_set_blocking instead.

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