Re: [PATCH v13 3/4] HID: hid-msi: Add RGB control interface

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Data races and locking mismatches when accessing RGB configuration variables and LED core state.

Pre-existing issues:
- [High] Use-after-free on device teardown due to missing cancellation of delayed works.
--

--- Patch [3]: [PATCH v13 3/4] HID: hid-msi: Add RGB control interface ---
commit d67de53a026c42fe9260a66fca23392240a52c26
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add RGB control interface

This commit adds an RGB control interface for MSI Claw devices. It provides
support for configuring up to 9 LED zones via an MCU memory interface and
sets up the corresponding sysfs attributes and LED class device.

Link: https://patch.msgid.link/20260529072111.7565-4-derekjohn.clark@gmail.com

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 023d63cee6b06..5cb85fc2f0534 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -349,6 +410,38 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
>  		for (i = 0; i < CLAW_KEYS_MAX; i++)
>  			codes[i] = (mkeys->codes[i]);
>  		break;
> +	case CLAW_RGB_PENDING:
> +		frame = (struct claw_rgb_report *)cmd_rep->data;

[ ... ]

> +		scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
> +			memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> +			       sizeof(struct rgb_frame));
> +
> +			/* Only use frame 0 for remaining variable assignment */
> +			if (f_idx != 0)
> +				break;
> +
> +			drvdata->rgb_speed = frame->speed;
> +			drvdata->led_mc.led_cdev.brightness = frame->brightness;
> +			drvdata->led_mc.subled_info[0].intensity = frame->zone_data.zone[0].red;
> +			drvdata->led_mc.subled_info[1].intensity = frame->zone_data.zone[0].green;
> +			drvdata->led_mc.subled_info[2].intensity = frame->zone_data.zone[0].blue;
> +		}
> +
> +		break;

[Severity: Medium]
Is there a locking mismatch for rgb_speed? Here in claw_profile_event(),
rgb_speed is updated while holding frame_lock. However, in speed_store(),
it is updated while holding profile_lock.

Also, does this direct modification of the LED class device internal state
need synchronization? Updating brightness and subled_info[x].intensity here
is done without holding the standard led_access lock used by the LED core,
which could cause data races with concurrent sysfs accesses.

[ ... ]

> +/* Send RGB configuration to device */
> +static int claw_write_rgb_state(struct claw_drvdata *drvdata)
> +{
> +	struct claw_rgb_report report = { {0x01, 0}, CLAW_RGB_FRAME_OFFSET, 0x00,
> +			drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
> +			drvdata->led_mc.led_cdev.brightness };

[Severity: Medium]
Does claw_write_rgb_state() need to hold a lock when reading rgb_speed?
It appears to be read locklessly here, but is updated with a lock elsewhere.
This also applies to other variables like rgb_effect and rgb_enabled read
locklessly in claw_apply_rgb_state() and sysfs show functions.

[ ... ]

> +static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{

[ ... ]

> +	/* 0 is fastest, invert value for intuitive userspace speed */
> +	speed = 20 - val;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
> +		drvdata->rgb_speed = speed;
> +
> +	mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
> +
> +	return count;
> +}

[Severity: Medium]
This is where rgb_speed is written using profile_lock instead of the
frame_lock used in claw_profile_event().

[ ... ]

> @@ -1088,6 +1727,7 @@ static void claw_remove(struct hid_device *hdev)
>  {
>  	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
>  	bool gp_registered;
> +	bool rgb_registered;
>  
>  	if (!drvdata)
>  		return;

[ ... ]

>  	if (gp_registered)
>  		device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
>  
> +	if (rgb_registered) {
> +		device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> +		led_classdev_multicolor_unregister(&drvdata->led_mc);
> +	}
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
> +
>  	hid_hw_close(hdev);
>  }

[Severity: High]
This is a pre-existing issue, but does this teardown path leak delayed works?
The driver initializes the cfg_setup and cfg_resume delayed works during
probe, but they don't appear to be cancelled here in claw_remove().

If the device is removed while either of those works is pending, could they
execute after the devres-allocated drvdata is freed, causing a
use-after-free crash?

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