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 [off-list ref]

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.

[ ... ]

+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