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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `rgb_queue` delayed work accesses a freed device pointer `led_cdev.dev` on its error path due to incorrect teardown order in `claw_remove`.
- [High] Missing cancellation of `cfg_setup` and `cfg_resume` delayed works in `claw_remove()`.
- [Medium] RGB configuration states and LED class properties are modified and read concurrently without proper synchronization.
--

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

HID: hid-msi: Add RGB control interface

Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface. It has 9 total zones (4 per joystick ring
and 1 for the ABXY buttons), and supports up to 8 sequential frames of
RGB zone data.

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 9d3249fa50291..ddb64adb08b9c 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
> @@ -406,6 +406,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;
> +		rgb_addr = drvdata->rgb_addr;
> +		read_addr = be16_to_cpu(frame->read_addr);
> +
> +		if (read_addr < drvdata->rgb_addr)
> +			return -EAGAIN;
> +
> +		frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> +		if (frame_calc >= CLAW_RGB_MAX_FRAMES) {
> +			dev_err(&drvdata->hdev->dev, "Got unsupported frame index: %x\n",
> +				frame_calc);
> +			return -EAGAIN;
> +		}
> +		f_idx = frame_calc;
> +
> +		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;

[Severity: Medium]
Are there missing locks around rgb_speed and led_cdev.brightness?

Here in claw_profile_event(), drvdata->rgb_speed and
drvdata->led_mc.led_cdev.brightness are written to under the frame_lock.
However, they appear to be read locklessly in claw_write_rgb_state() outside
of frame_lock. Similarly, speed_store() modifies drvdata->rgb_speed locklessly
from userspace.

Could this inconsistent locking lead to torn reads or data races between
sysfs operations, hardware events, and the workqueue?

[ ... ]

> +static void claw_rgb_queue_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, rgb_queue);
> +	int ret;
> +
> +	if (!drvdata)
> +		return;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +		if (!smp_load_acquire(&drvdata->rgb_registered))
> +			return;
> +	}
> +
> +	ret = claw_apply_rgb_state(drvdata);
> +	if (ret)
> +		dev_err(drvdata->led_mc.led_cdev.dev,
> +			"Failed to apply RGB state: %d\n", ret);

[Severity: High]
Could this dev_err() access a freed device pointer?

If claw_rgb_queue_fn() starts executing and passes the rgb_registered check,
but the device is concurrently removed, claw_remove() will set rgb_registered
to false and call led_classdev_multicolor_unregister(). This drops the final
reference and frees led_cdev.dev.

If claw_apply_rgb_state() subsequently fails, this error path will log using
drvdata->led_mc.led_cdev.dev, which might trigger a use-after-free since the
work was not canceled prior to unregistration.

[ ... ]

> @@ -1084,6 +1721,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;
> @@ -1096,11 +1734,21 @@ static void claw_remove(struct hid_device *hdev)
>  		gp_registered = smp_load_acquire(&drvdata->gp_registered);
>  		/* Pairs with smp_load_acquire in attribute show/store functions */
>  		smp_store_release(&drvdata->gp_registered, false);
> +		/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +		rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
> +		/* Pairs with smp_load_acquire in attribute show/store functions */
> +		smp_store_release(&drvdata->rgb_registered, false);
>  	}
>  
>  	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);

[Severity: High]
Should cancel_delayed_work_sync() for rgb_queue happen before
led_classdev_multicolor_unregister()?

Canceling the work after unregistering the LED device creates a window
where the executing work can access freed led_cdev.dev memory (as noted
in claw_rgb_queue_fn).

>  	hid_hw_close(hdev);
>  }

[Severity: High]
Are we missing the cancellation of the cfg_setup and cfg_resume delayed works?

The driver schedules cfg_setup and cfg_resume, but does not seem to explicitly
cancel them here in claw_remove(). If the device is disconnected while either
work is pending, could they execute after drvdata has been freed by devres,
resulting in a use-after-free?

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