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

["Derek J. Clark" <derekjohn.clark@gmail.com>]

On July 14, 2026 7:42:19 PM GMT+09:00, sashiko-bot@kernel.org wrote:
>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?

Humans reading this dumb bot's reply,
No, they are under scoped_guard 9 lines above this...

>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.

Addressed below.

>
>[ ... ]
>
>> @@ -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()?
>

This suggestion will just reintroduce the bug the reorder was meant to fix. device_remove_group is blocking as far as I'm aware. By canceling the work after that it removes the possibility of re-arming the worker after it is freed. The smp_load_acquire ensures any currently queued thread will abort before the call to the led_classdev in the error message. If it is really a problem I'll swap the device over to hdev.dev, but the probability is nearly zero of this ever happening.

>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?
>


cfg_setup & cfg_removed are cancelled in this function above the portion marker cutoff. This is introduced in patch 1/4. Bot is wrong here.

- Derek