Re: [PATCH v7 4/4] HID: hid-msi: Add Rumble Intensity Attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent execution of sysfs attributes can lead to premature command completion and stale reads because untracked generic ACKs from zero-timeout commands hijack pending wait completions.
- [High] A race condition during module removal can lead to a Use-After-Free of `drvdata` due to improper teardown ordering of the `rgb_queue` delayed work.
--

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

HID: hid-msi: Add Rumble Intensity Attributes

Adds intensity adjustment for the left and right rumble motors.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 61917902e38d3..4adc5588618df 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -919,6 +951,154 @@ static ssize_t button_mapping_options_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(button_mapping_options);
>  
> +static ssize_t rumble_intensity_left_store(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count)
> +{

[ ... ]

> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +				    (u8 *)&report, sizeof(report), 25);
> +	if (ret)
> +		return ret;
> +
> +	/* MCU will not send ACK until the USB transaction completes. ACK is sent
> +	 * immediately after and will hit the stale state machine, before the next
> +	 * command re-arms the state machine. Timeout 0 ensures no deadlock waiting
> +	 * for ACK that ill never come.
> +	 */
> +	ret = claw_hw_output_report(hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 0);

[Severity: High]
This is a pre-existing issue, but using timeout 0 here appears to leave an
untracked generic ACK in the hardware pipeline.

If another thread concurrently triggers a hardware command (like a sysfs
show/store) that waits for an ACK, could the claw_raw_event() handler
unconditionally consume this delayed generic ACK to complete the new
command's wait?

Since the generic ACK handler doesn't verify drvdata->waiting_cmd, this
might cause the new command to return prematurely before its actual data
arrives, leading to stale sysfs reads or dropped configuration writes.

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

[ ... ]

> @@ -1557,6 +1748,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
>  	spin_lock_init(&drvdata->mode_lock);
>  	spin_lock_init(&drvdata->profile_lock);
>  	spin_lock_init(&drvdata->frame_lock);
> +	spin_lock_init(&drvdata->rumble_lock);
>  	init_completion(&drvdata->send_cmd_complete);
>  	INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
>  	INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);

[Severity: High]
This is a pre-existing issue not introduced by this patch, but while
reviewing the driver's workqueue lifecycle, I noticed a potential race
condition during module removal.

In claw_remove(), if cancel_delayed_work_sync(&drvdata->rgb_queue) is called
before setting drvdata->rgb_registered to false, could a concurrent sysfs
write to the LED brightness file cause a use-after-free?

Since the LED class device is managed via devres, it remains accessible. A
write could invoke claw_led_brightness_set(), check that rgb_registered is
still true, and re-arm the delayed work timer after it was cancelled. When
devres later frees drvdata, the timer would fire and access freed memory.

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