Re: [PATCH v11 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] Mixing `goto`-based error handling with scope-based cleanup (`__free`/`guard`).
- [High] TOCTOU race condition causing lost device configuration updates.
--

commit 5c893bbc0296560ff4fc46c1a17db05f45daeaf6
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add MSI Claw configuration driver

This commit adds a configuration HID driver for the MSI Claw series of
handheld PCs. It introduces sysfs attributes to change the gamepad mode,
M-key behavior, and perform a device reset.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> --- /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)
> +{
> +	unsigned char *dmabuf __free(kfree) = NULL;
> +	u8 header[] = { CLAW_OUTPUT_REPORT_ID, 0, 0, 0x3c, index };
[ ... ]
> +	guard(mutex)(&drvdata->cfg_mutex);
> +	if (timeout) {
[ ... ]
> +	ret = hid_hw_output_report(hdev, dmabuf, CLAW_PACKET_SIZE);
> +	if (ret < 0)
> +		goto err_clear_state;

[Severity: High]
Is it intended to mix goto-based error handling with scope-based cleanup
in this function?

The cleanup subsystem guidelines mention that functions should either
convert all resources that need cleanup to scope-based cleanup, or convert
none of them. Using guard and __free alongside a goto label can create
confusing ownership and lock lifetime semantics. 

Could this be converted entirely to scope-based cleanup?

> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret)
> +		goto err_clear_state;
[ ... ]
> +err_clear_state:
> +	if (timeout) {
> +		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)
> +{
[ ... ]
> +	data[0] = ret;
> +	scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
> +		data[1] = drvdata->mkeys_function;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);

[Severity: High]
Does this sequence introduce a time-of-check to time-of-use race condition?

The current state of mkeys_function is read while holding mode_lock, but the
lock is released before claw_hw_output_report() acquires cfg_mutex. If two
threads concurrently write to gamepad_mode and mkeys_function, both could
read the other's state before either acquires cfg_mutex.

This could result in the second thread sending a command that uses stale
data, silently reverting the first thread's update when the device
acknowledges the command. 

Should the read of the current state and the device update be atomic under
a single lock?

> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
[ ... ]
> +static ssize_t mkeys_function_store(struct device *dev, struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
[ ... ]
> +	scoped_guard(spinlock_irqsave, &drvdata->mode_lock)
> +		data[0] = drvdata->gamepad_mode;
> +	data[1] = ret;
> +
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data, ARRAY_SIZE(data), 0);

[Severity: High]
A similar sequence happens here. Can concurrent writes to these attributes
lead to lost device configuration updates due to the locks being acquired and
released separately?

> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}

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