Re: [PATCH v6 2/4] HID: hid-msi: Add M-key mapping attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race and state desynchronization in `profile_pending` allows delayed ACKs to corrupt profile data.
- [High] Race window in `claw_hw_output_report()` causes spurious command timeouts.
- [High] Use-After-Free during module teardown due to incorrect delayed work cancellation order.
--

commit d45c51fc226fdd946c2d70a93c254be199300dc7
Author: Derek J. Clark <derekjohn.clark@gmail.com>
Subject: HID: hid-msi: Add M-key mapping attributes

This commit introduces M-key mapping attributes for the MSI Claw device.
It adds the ability to read and write profile mappings for two back buttons
(M1 and M2) via sysfs, using custom HID commands to communicate with the
hardware.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index c79eb0bfeb776..e58d35dba5b40 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

>  struct claw_drvdata {
>  	/* MCU General Variables */
> +	enum claw_profile_ack_pending profile_pending;
>  	struct completion send_cmd_complete;
>  	struct delayed_work cfg_resume;
>  	struct delayed_work cfg_setup;

This problem wasn't introduced by this patch, but does the cancellation order
of these delayed works cause a use-after-free regression during module
teardown?

In cfg_resume_fn(), cfg_setup is unconditionally queued via
schedule_delayed_work(). During module removal, if
cancel_delayed_work_sync(&drvdata->cfg_setup) is executed before
cancel_delayed_work_sync(&drvdata->cfg_resume), and cfg_resume_fn is
actively executing:

The first cancellation would return immediately because cfg_setup is not
yet pending. cfg_resume_fn would then queue cfg_setup. The second
cancellation would wait for cfg_resume_fn to finish.

Once the removal completes and drvdata is freed by devres, could the
newly queued cfg_setup fire 500ms later and dereference the freed
drvdata pointer?

[ ... ]

> +static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_report *cmd_rep)
> +{
> +	u8 *codes;
> +	int i;
> +
> +	switch (drvdata->profile_pending) {
> +	case CLAW_M1_PENDING:
> +	case CLAW_M2_PENDING:
> +		codes = (drvdata->profile_pending == CLAW_M1_PENDING) ?
> +			drvdata->m1_codes : drvdata->m2_codes;
> +		for (i = 0; i < CLAW_KEYS_MAX; i++)
> +			codes[i] = (cmd_rep->data[6 + i]);
> +		break;
> +	default:
> +		dev_dbg(&drvdata->hdev->dev,
> +			"Got profile event without changes pending from command: %x\n",
> +			cmd_rep->cmd);
> +		return -EINVAL;
> +	}
> +	drvdata->profile_pending = CLAW_NO_PENDING;
> +
> +	return 0;
> +}

Is there a data race and potential state desynchronization with
profile_pending here?

drvdata->profile_pending is written under profile_mutex in process context
in claw_buttons_show(), but it is read and cleared locklessly here in the
atomic HID event handler.

If a request (like an M1 read) times out, profile_pending is cleared by the
sysfs thread. If a subsequent request then sets profile_pending to
CLAW_M2_PENDING, could a concurrently arriving delayed ACK for the original
M1 read cause this lockless handler to mistakenly parse the M1 payload into
drvdata->m2_codes?

This would reset profile_pending and prematurely wake the waiting thread,
corrupting the profile data.

[ ... ]

> +static int claw_buttons_store(struct device *dev, const char *buf, u8 mkey_idx)
> +{

[ ... ]

> +set_buttons:
> +	scoped_guard(mutex, &drvdata->rom_mutex) {
> +		ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    data, len, 25);

This problem wasn't introduced by this patch, but is there a race window in
claw_hw_output_report() that could cause spurious command timeouts?

Looking at claw_hw_output_report(), drvdata->waiting_for_ack is set to
true inside a cmd_lock critical section. Immediately after releasing the
spinlock, it calls reinit_completion(&drvdata->send_cmd_complete).

If the hardware responds exceptionally quickly, or if a delayed ACK from a
previous timed-out command arrives in the small window between the spinlock
release and the reinit_completion() call:

Could the interrupt handler clear waiting_for_ack and call complete(),
only for the main thread to immediately erase the completion event with
reinit_completion()?

This would cause wait_for_completion_interruptible_timeout() to block and
incorrectly return -ETIMEDOUT to userspace.

Should reinit_completion() be called before setting waiting_for_ack?

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