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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Inverted registration check in `gamepad_mode_show` breaks functionality and allows access after device removal.
- [High] TOCTOU race in sysfs attributes leading to Use-After-Free during device teardown.
- [High] Race condition with `reinit_completion` and unprotected state on error paths in `claw_hw_output_report`.
- [High] Incorrect cancellation order of interdependent delayed works causes a Use-After-Free.
- [High] Brittle endpoint check in `msi_remove` bypasses teardown and causes unbalanced `hid_hw_close`.
--

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

This commit adds a configuration HID driver for the MSI Claw series of
handheld PC's. It provides an initial driver outline and attributes for
changing the gamepad mode, M-key behavior, and handling resets.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> new file mode 100644
> index 0000000000000..c79eb0bfeb776
> --- /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) {
> +		scoped_guard(spinlock_irqsave, &drvdata->cmd_lock) {
> +			drvdata->waiting_cmd = index;
> +			drvdata->waiting_for_ack = true;
> +			drvdata->cmd_status = -ETIMEDOUT;
> +		}
> +		reinit_completion(&drvdata->send_cmd_complete);

Could this race with an incoming ACK? Since reinit_completion() is called
outside cmd_lock after setting waiting_for_ack = true, an ACK arriving in
this window could call complete(), which would then be overwritten, causing
a command timeout.

> +	}
> +
> +	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;
> +
> +	ret = ret == CLAW_PACKET_SIZE ? 0 : -EIO;
> +	if (ret)
> +		return ret;

If hid_hw_output_report() fails or returns a partial write, the function
returns early without resetting waiting_for_ack to false. Could this corrupt
the state machine and cause stray ACKs to trigger spurious completions later?

[ ... ]

> +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;
> +
> +	if (drvdata->gamepad_registered)
> +		return -ENODEV;

Should this be if (!drvdata->gamepad_registered)? Without the logical NOT,
this prevents reading the mode during normal operation and allows access
during teardown, which could lead to a use-after-free since the sysfs
attribute can be accessed after device removal.

[ ... ]

> +static ssize_t mkeys_function_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];
> +
> +	if (!drvdata->gamepad_registered)
> +		return -ENODEV;

Is there a race condition here during device teardown? The check for
gamepad_registered happens locklessly before cfg_mutex is acquired in
claw_hw_output_report().

If claw_remove() executes concurrently, sets gamepad_registered to false,
and closes the hardware, could this result in calling hid_hw_output_report()
on a stopped device and causing a use-after-free when the sysfs thread
resumes?

[ ... ]

> +static void claw_remove(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata) {
> +		hid_hw_close(hdev);
> +		return;
> +	}
> +
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);

Should cfg_resume be cancelled before cfg_setup? Since cfg_resume_fn() can
schedule cfg_setup, cancelling cfg_setup first might allow cfg_resume_fn()
to queue it again. If this happens, the newly scheduled cfg_setup work could
access freed drvdata memory after claw_remove() finishes.

> +
> +	drvdata->gamepad_registered = false;
> +
> +	hid_hw_close(hdev);
> +}
> +
> +static void msi_remove(struct hid_device *hdev)
> +{
> +	int ret;
> +	u8 ep;
> +
> +	ret = get_endpoint_address(hdev);
> +	if (ret <= 0)
> +		goto hw_stop;
> +
> +	ep = ret;
> +	if (ep == CLAW_XINPUT_CFG_INTF_IN || ep == CLAW_DINPUT_CFG_INTF_IN)
> +		claw_remove(hdev);

Is it safe to rely on get_endpoint_address() during removal?

If userspace modifies the USB alternate setting via usbfs while the driver
is bound, the endpoint could change, causing claw_remove() to be skipped
and leaving the workqueues running, leading to a use-after-free.

Conversely, if the endpoint didn't match during probe but does match during
removal, claw_remove() might be called with a NULL drvdata and execute
hid_hw_close() without a prior hid_hw_open(), underflowing ll_open_count.

[ ... ]

> +static int claw_suspend(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata)
> +		return -ENODEV;
> +
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);

Does this have the same cancellation order issue as claw_remove(), where
cfg_resume_fn() could reschedule cfg_setup after it has been cancelled?

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