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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] ABBA deadlock in `claw_remove` during sysfs group removal.
- [High] Dynamic endpoint querying in `msi_remove` can bypass cleanup on alternate setting changes, leading to a Use-After-Free.
- [High] Unbalanced `hid_hw_close` call in `claw_remove` causes reference count underflow if `drvdata` is NULL.
- [High] Late ACKs from timed-out commands incorrectly fulfill the completion wait of subsequent synchronous commands.
- [Medium] The 8ms timeout for hardware output reports is too short and will randomly fail due to jiffy timer granularity.
- [Medium] System suspend during initial probe permanently prevents sysfs attribute registration.
- [Medium] Kconfig dependency mismatch for LEDS_CLASS causes unmet dependency warnings.
- [Medium] cfg_resume_fn unconditionally forces a destructive USB disconnect on every system resume.
- [Medium] Unprotected data race on `drvdata->gamepad_mode` and `drvdata->mkeys_function` between interrupt and process contexts.
--

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

This commit adds a configuration driver for the MSI Claw series of handhelds.
It supports gamepad mode switching, M-key behavior, and a device reset via
sysfs attributes.

> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index ff2f580b660ba..ec412bba1e1ed 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -485,6 +485,18 @@ config HID_GT683R
>  	Currently the following devices are know to be supported:
>  	  - MSI GT683R
>  
> +config HID_MSI
> +	tristate "MSI Claw Gamepad Support"
> +	depends on USB_HID
> +	select LEDS_CLASS
> +	select LEDS_CLASS_MULTICOLOR

Is it possible this will cause an unmet dependency Kconfig warning if NEW_LEDS
is not enabled?

LEDS_CLASS depends on NEW_LEDS, so selecting it without inheriting the
dependency can cause build issues on some configurations.

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

[ ... ]

> +static int claw_gamepad_mode_event(struct claw_drvdata *drvdata,
> +				   struct claw_command_report *cmd_rep)
> +{
> +	if (cmd_rep->data[0] >= ARRAY_SIZE(claw_gamepad_mode_text) ||
> +	    !claw_gamepad_mode_text[cmd_rep->data[0]] ||
> +	    cmd_rep->data[1] >= ARRAY_SIZE(claw_mkeys_function_text))
> +		return -EINVAL;
> +
> +	drvdata->gamepad_mode = cmd_rep->data[0];
> +	drvdata->mkeys_function = cmd_rep->data[1];

Is there a potential data race here?

These fields are written from the raw event callback (which runs in interrupt
context), but they are read in process context by functions like cfg_resume_fn
and gamepad_mode_show without acquiring a shared lock.

> +
> +	return 0;
> +}
> +
> +static int claw_raw_event(struct claw_drvdata *drvdata, struct hid_report *report,
> +			  u8 *data, int size)
> +{

[ ... ]

> +	switch (cmd_rep->cmd) {
> +	case CLAW_COMMAND_TYPE_GAMEPAD_MODE_ACK:
> +		ret = claw_gamepad_mode_event(drvdata, cmd_rep);
> +		break;
> +	case CLAW_COMMAND_TYPE_ACK:
> +		break;
> +	default:
> +		dev_dbg(&drvdata->hdev->dev, "Unknown command: %x\n", cmd_rep->cmd);
> +		return 0;
> +	}
> +
> +	complete(&drvdata->send_cmd_complete);

If a previous synchronous command timed out, but the device later sends an ACK,
will this unconditionally complete the completion?

When the next command calls reinit_completion(), an immediate late ACK from the
previous command might falsely complete the new command's wait, causing the
driver to read stale state.

> +
> +	return ret;
> +}

[ ... ]

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

[ ... ]

> +	if (timeout) {
> +		ret = wait_for_completion_interruptible_timeout(&drvdata->send_cmd_complete,
> +								msecs_to_jiffies(timeout));

Could this timeout prematurely?

When the timeout is very short (like 8ms as used in gamepad_mode_show),
msecs_to_jiffies(8) evaluates to 1 jiffy on HZ=100 kernels.

A 1-jiffy timeout can expire almost instantly if the system jiffy tick occurs
immediately after wait_for_completion_interruptible_timeout() is called,
leading to a spurious -EBUSY.

> +
> +		dev_dbg(&hdev->dev, "Remaining timeout: %u\n", ret);
> +		if (ret >= 0) /* preserve errors */
> +			ret = ret == 0 ? -EBUSY : 0; /* timeout occurred : time remained */
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static void cfg_resume_fn(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = container_of(work, struct delayed_work, work);
> +	struct claw_drvdata *drvdata = container_of(dwork, struct claw_drvdata, cfg_resume);
> +	u8 data[2] = { drvdata->gamepad_mode, drvdata->mkeys_function };
> +	int ret;
> +
> +	ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SWITCH_MODE, data,
> +				    ARRAY_SIZE(data), 0);

The commit message mentions that sending the SWITCH_MODE command causes a USB
disconnect in the device.

Will this unconditional SWITCH_MODE call on every system resume forcibly
disconnect and re-enumerate the gamepad, potentially breaking active
applications holding an open file descriptor?

> +	if (ret)
> +		dev_err(&drvdata->hdev->dev, "Failed to set gamepad mode settings: %d\n", ret);
> +}

[ ... ]

> +static void claw_remove(struct hid_device *hdev)
> +{
> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (!drvdata) {
> +		hid_hw_close(hdev);

If drvdata is NULL (meaning claw_probe was never called or failed), will
calling hid_hw_close() underflow the hdev->ll_open_count, since hid_hw_open()
was never successfully called?

> +		return;
> +	}
> +
> +	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +
> +	guard(mutex)(&drvdata->cfg_mutex);
> +	if (drvdata->gamepad_registered)
> +		device_remove_group(&hdev->dev, &claw_gamepad_attr_group);

Will holding the cfg_mutex while removing the sysfs group cause an ABBA
deadlock?

The sysfs core's removal blocks until active references to attributes are
drained. If a user concurrently accesses an attribute, the sysfs callback will
try to acquire the cfg_mutex (via claw_hw_output_report).

The driver would hold the mutex waiting for the sysfs reference to drop, while
the callback holds the sysfs reference waiting for the mutex.

> +
> +	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);

If userspace dynamically changes the USB alternate setting via
USBDEVFS_SETINTERFACE, the endpoint check here could fail and bypass
claw_remove().

If claw_remove() is bypassed, the delayed works (cfg_setup and cfg_resume)
are never cancelled. Could this result in a use-after-free when those works
eventually execute and access the freed drvdata?

> +
> +hw_stop:
> +	hid_hw_stop(hdev);
> +}

[ ... ]

> +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);

If the system suspends before the cfg_setup delayed work fires, the work is
synchronously cancelled here.

Since claw_resume schedules cfg_resume but does not reschedule cfg_setup,
will drvdata->gamepad_registered remain false forever, permanently
preventing sysfs attribute registration?

> +	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +
> +	return 0;
> +}

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