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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sysfs attributes remain active during and after device removal, leading to a Time-of-Check to Time-of-Use (TOCTOU) race condition and Use-After-Free of HID transport structures.
- [High] Calling the sleeping function `hid_hw_close()` while holding a spinlock in `claw_remove()`, causing a kernel panic.
- [Low] Mixing `goto` error handling with scope-based cleanups (`__free()` and `guard()`) in the same function.
--

commit 76e865e5c59094dfdc13a67a7954da1cc82124bc
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add MSI Claw configuration driver

This commit adds a new HID configuration driver for the MSI Claw handheld PC.
It introduces the initial driver structure, including sysfs attributes to
control the gamepad mode, M-key behavior, and device reset commands.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..40b16253abbb3
> --- /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 };
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	size_t header_size = ARRAY_SIZE(header);
> +	int ret;
> +
> +	if (header_size + len > CLAW_PACKET_SIZE)
> +		return -EINVAL;
> +
> +	/* We can't use a devm_alloc reusable buffer without side effects during suspend */
> +	dmabuf = kzalloc(CLAW_PACKET_SIZE, GFP_KERNEL);
> +	if (!dmabuf)
> +		return -ENOMEM;
> +
> +	memcpy(dmabuf, header, header_size);
> +	if (data && len)
> +		memcpy(dmabuf + header_size, data, len);
> +
> +	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)
> +		goto err_clear_state;

[Severity: Low]
Is it okay to mix goto error labels with scope-based cleanups in the same
function?

According to the cleanup subsystem guidelines, functions should either fully
rely on scope-based cleanups or use manual cleanups entirely to avoid
confusing ownership semantics.

> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret)
> +		goto err_clear_state;
> +
> +	if (timeout) {
> +		ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> +								msecs_to_jiffies(timeout));
> +
> +		dev_dbg(&hdev->dev, "Remaining timeout: %u\n", ret);
> +		ret = ret > 0 ? drvdata->cmd_status : ret ?: -EBUSY;
> +	}
> +
> +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)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int i, ret = -EINVAL;
> +	u8 data[2];
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* 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 this lead to a use-after-free during device disconnect?

If a sysfs thread calls gamepad_mode_store() and gets preempted right after
releasing registration_lock here, a concurrent device disconnect could
trigger msi_remove() which clears gamepad_registered.

msi_remove() will then call hid_hw_stop(), destroying the HID transport
structures. When the sysfs thread resumes and continues execution here:

> +
> +	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;
> +		}
> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	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);

It will call claw_hw_output_report() which delegates to hid_hw_output_report()
and accesses the now-destroyed transport structures.

Since devm_device_add_group() is used to register the sysfs attributes, the
sysfs group remains active until after msi_remove() finishes completely.
Should the sysfs group be removed synchronously before hid_hw_stop() is
called to prevent this race?

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

[ ... ]

> +static void claw_remove(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata)
> +		return;
> +
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +
> +	guard(spinlock_irqsave)(&drvdata->registration_lock);
> +	/* Pairs with smp_load_acquire in attribute show/store functions */
> +	smp_store_release(&drvdata->gamepad_registered, false);
> +
> +	hid_hw_close(hdev);

[Severity: High]
Does this call a sleeping function while holding a spinlock?

The guard() macro acquires registration_lock and holds it until the end of
the function. While this spinlock is held and interrupts are disabled,
hid_hw_close() is called, which attempts to acquire the sleeping mutex
ll_open_lock.

I noticed this is fixed in a later patch in the series by changing guard()
to scoped_guard(), but doesn't this break bisectability by introducing a
kernel panic in this commit?

> +}

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