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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in the sysfs attribute handlers allows hardware access after device teardown.
- [Medium] State machine is not reset if hid_hw_output_report() returns a negative error code.
--

commit 4222880100fe04d05b88fc8c07f6d4c9f2dcb27c
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add MSI Claw configuration driver

Adds a configuration HID driver for the MSI Claw series of handheld PCs
to support gamepad mode switching, M-key behavior adjustments, and device
reset functions.

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

[ ... ]

> +static int claw_hw_output_report(struct hid_device *hdev, u8 index, u8 *data,
> +				 size_t len, unsigned int timeout)
> +{

[ ... ]

> +	guard(mutex)(&drvdata->cfg_mutex);
> +	if (timeout) {
> +		reinit_completion(&drvdata->send_cmd_complete);
> +		scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
> +			drvdata->waiting_cmd = index;
> +			drvdata->waiting_for_ack = true;
> +			drvdata->cmd_status = -ETIMEDOUT;
> +		}
> +	}
> +
> +	dev_dbg(&hdev->dev, "Send data as raw output report: [%*ph]\n",
> +		CLAW_PACKET_SIZE, dmabuf);
> +
> +	ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Does this leave the state machine armed if hid_hw_output_report() returns a
negative error code (like -EPIPE)?

If the early return is taken, waiting_for_ack and waiting_cmd remain set.
If a subsequent command is sent with timeout == 0, and an unexpected input
report arrives that matches the stale waiting_cmd, could it improperly
trigger a completion and corrupt the internal state?

Would it be safer to clear waiting_cmd and waiting_for_ack on all error paths
before returning?

> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret) {
> +		scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
> +			drvdata->waiting_cmd = CLAW_COMMAND_TYPE_NONE;
> +			drvdata->waiting_for_ack = false;
> +		}
> +		return ret;
> +	}

[ ... ]

> +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);
> +	int i, ret = -EINVAL;
> +	u8 data[2];
> +
> +	/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +	if (!smp_load_acquire(&drvdata->gamepad_registered))
> +		return -ENODEV;

[Severity: High]
Can a sysfs thread race with device removal here and cause a use-after-free?

The lockless check of gamepad_registered does not appear to prevent
msi_remove() from running concurrently once the check passes. If the USB
device is disconnected, a sysfs thread could evaluate the flag as true
and then get preempted.

Concurrently, msi_remove() could run, set the flag to false, and call
hid_hw_stop(), which tears down the lower-level USB HID resources.

When the sysfs thread resumes, it would eventually call:
claw_hw_output_report() -> hid_hw_output_report() -> usbhid_output_report()
on the torn-down hdev structure.

Should a mutex or other synchronization mechanism be used to safely serialize
the sysfs execution against hardware teardown?

> +
> +	for (i = 0; i < ARRAY_SIZE(claw_gamepad_mode_text); i++) {
> +		if (claw_gamepad_mode_text[i] && sysfs_streq(buf, claw_gamepad_mode_text[i])) {
> +			ret = i;
> +			break;
> +		}
> +	}

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