Re: [PATCH v3 1/4] HID: hid-msi: Add MSI Claw configuration driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect teardown order in `claw_remove()` creates a race condition that leaks sysfs attributes and leads to a Use-After-Free.
- [High] Unreasonably short 8ms timeout causes spurious `-EBUSY` failures on `HZ=100` systems.
- [Medium] Read-modify-write race condition in sysfs configuration handlers causes silent state corruption.
- [Medium] Missing `.suspend` callback allows hardware-interacting delayed works to execute during system sleep.
--

commit ce9aa923d32bac6eb56e73d413c94aa753ea00cf
Author: Derek J. Clark <derekjohn.clark@gmail.com>
Subject: HID: hid-msi: Add MSI Claw configuration driver

Adds configuration HID driver for the MSI Claw series of handheld PC's.
In this initial patch add the initial driver outline and attributes for
changing the gamepad mode, M-key behavior, and add a WO reset function.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..89bb32f00bfc3
> --- /dev/null
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> +static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	u8 data[2] = { 0x00, drvdata->mkeys_function };

Could this lead to a read-modify-write race condition if two threads write to
the sysfs attributes simultaneously?

They read drvdata->mkeys_function locklessly to construct the command payload.
After the payload is prepared, claw_hw_output_report() is called, which then
acquires cfg_mutex to send the command. If two threads write concurrently,
the second one could overwrite the first one's change because it read a stale
state.

Should the payload construction be performed inside the cfg_mutex critical
section?

[ ... ]

> +static ssize_t gamepad_mode_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret, i;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_READ_GAMEPAD_MODE, NULL, 0, 8);

Will this 8 millisecond timeout cause spurious failures on systems configured
with a 100 hz timer tick?

On such systems, msecs_to_jiffies(8) evaluates to exactly 1 jiffy. A 1-jiffy
timeout using wait_for_completion_interruptible_timeout() can expire in under
1 millisecond if the timer tick fires immediately after the wait is scheduled,
which might not provide enough time for the interrupt to complete and will
return -EBUSY.

[ ... ]

> +static void claw_remove(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata) {
> +		hid_hw_stop(hdev);
> +		return;
> +	}
> +
> +	device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);

Does this teardown sequence leak sysfs attributes and cause a use-after-free?

If cfg_setup_fn() is executing concurrently, device_remove_group() will do
nothing because the sysfs group has not been added yet. Then
cancel_delayed_work_sync() will wait for the work to finish.

Meanwhile, cfg_setup_fn() proceeds to call device_add_group(), adding the
sysfs attributes. When the work finishes, claw_remove() completes and drvdata
is freed. The sysfs attributes remain registered to the device, and any
subsequent read or write from userspace will trigger a use-after-free when
accessing the freed drvdata pointer.

Should cancel_delayed_work_sync() happen before device_remove_group() to
ensure correct teardown order?

[ ... ]

> +static struct hid_driver msi_driver = {
> +	.name		= "hid-msi",
> +	.id_table	= msi_devices,
> +	.raw_event	= msi_raw_event,
> +	.probe		= msi_probe,
> +	.remove		= msi_remove,
> +	.resume		= msi_resume,
> +};

Does this driver need a suspend callback?

The cfg_setup and cfg_resume delayed works are scheduled on the standard
non-freezable system workqueue. Without a suspend callback to cancel them,
these works can execute while the system is asleep.

When they fire, they will call hid_hw_output_report() on a suspended
usb device, which could cause input/output timeouts, lockups, or inadvertently
wake up the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515033622.2095277-1-derekjohn.clark@gmail.com?part=1