Re: [PATCH v2 6/6] HID: hid-lenovo-go: clear calibration status on start

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing hid_hw_stop() and hid_hw_close() in hid_go_probe() error paths.
- [High] Missing hid_hw_stop() in hid_go_remove() on early return.
- [High] Sysfs attribute groups are leaked if hid_go_cfg_probe() fails midway.
- [High] Singleton driver state (`drvdata`) causes state corruption, lock re-initialization, and UAF when multiple devices are probed.
- [High] Deadlock in driver removal path between sysfs_remove_groups and mcu_property_out.
- [High] UAF vulnerability due to devm-managed LED class device unregistration ordering.
--

commit 6b546e272fefac54ad5bf113ee4d2d91e487fb03
Author: Aditya Dash <mradityadash@gmail.com>

HID: hid-lenovo-go: clear calibration status on start
    
A completed calibration leaves success or failure in its status file. If
the next calibration has the same result, userspace sees no change and
cannot tell when the new attempt starts or finishes.

Link each calibration action attribute to its status field. Set only that
field to unknown before submitting Start. Stop leaves the status unchanged,
and the next firmware result replaces unknown with success or failure.

Fixes: 995887a10da1 ("HID: hid-lenovo-go: Add Calibration Settings")

> diff --git a/drivers/hid/hid-lenovo-go.c b/drivers/hid/hid-lenovo-go.c
> index a4231d188b7af..2ed367abd4ccc 100644
> --- a/drivers/hid/hid-lenovo-go.c
> +++ b/drivers/hid/hid-lenovo-go.c

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but does this error path cause a
use-after-free?

If hid_go_cfg_probe() fails, hid_go_probe() returns the error code without
cleaning up. By omitting hid_hw_close() and hid_hw_stop(), the input and hidraw
interfaces remain registered and active while the driver binding fails and
devres resources are freed. Userspace interaction can then trigger a UAF.

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

	if (ep <= 0)
		return;
    ...
}

[Severity: High]
This is a pre-existing issue, but can this early return cause a use-after-free
if a malicious device with no endpoints is unplugged?

When a device is unplugged or unbound and get_endpoint_address() returns
-ENODEV, this early return completely bypasses the hid_hw_stop() call, leaving
input interfaces registered while driver devres resources are freed.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	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 do the sysfs attribute groups get leaked
here?

If devm_led_classdev_multicolor_register() fails, the manually created sysfs
groups are never removed because sysfs_remove_groups() is missing in the error
path. This leaves dangling sysfs files that can be accessed by userspace after
the driver fails to bind.

drivers/hid/hid-lenovo-go.c:hid_go_cfg_probe() {
    ...
	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: High]
This isn't a bug introduced by this patch, but does the use of a single global
static drvdata struct cause state corruption if multiple physical devices are
connected?

Probing a second device unconditionally overwrites drvdata.hdev and
re-initializes cfg_mutex and cmd.lock while they might be held, which could
lead to deadlocks or a use-after-free.

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

[Severity: High]
This is a pre-existing issue, but could this lock ordering cause an AB-BA
deadlock during device removal?

hid_go_cfg_remove() acquires drvdata.cfg_mutex and calls sysfs_remove_groups(),
which waits to drain active kernfs references. Meanwhile, an active sysfs
callback holds a kernfs reference and attempts to acquire drvdata.cfg_mutex
inside mcu_property_out(), which would cause a deadlock.

drivers/hid/hid-lenovo-go.c:hid_go_brightness_set() {
    ...
	ret = rgb_cfg_call(drvdata.hdev, SET_RGB_CFG, index, rgb_profile, 6);
    ...
}

drivers/hid/hid-lenovo-go.c:hid_go_cfg_remove() {
    ...
	hid_hw_stop(hdev);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free window
here during device removal?

Devres cleans up resources (unregistering the LED class device) after the
driver's remove() callback returns. Inside hid_go_cfg_remove(), hid_hw_stop()
is called.

During the window before devres runs, userspace can still invoke
hid_go_brightness_set(), which accesses drvdata.hdev and calls
hid_hw_output_report() on a stopped hardware device.

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