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

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

On May 11, 2026 2:22:29 PM PDT, sashiko-bot@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>- [High] `speed_show()` incorrectly casts the LED class device pointer to a `hid_device`, leading to an out-of-bounds memory read and kernel crash.
>- [High] Deadlock and missing return in `cfg_setup_fn` when handling initialization errors.
>- [High] Sysfs attributes re-arm the cancelled delayed work after device teardown, leading to a Use-After-Free.
>- [High] Racy nullification of `brightness_set` causes a TOCTOU NULL pointer dereference.
>- [Medium] Communication failure in `claw_read_rgb_config` leaves the profile parser in a corrupted state.
>--
>
>commit 9663adbabcba6be0b443e7073133d650a790c871
>Author: Derek J. Clark <derekjohn.clark@gmail.com>
>
>HID: hid-msi-claw: Add RGB control interface
>    
>Adds RGB control interface for MSI Claw devices with 9 total zones and
>supports up to 8 sequential frames of RGB zone data.
>
>> diff --git a/drivers/hid/hid-msi-claw.c b/drivers/hid/hid-msi-claw.c
>> index 60694d075d56f..f4fe74a784c29 100644
>> --- a/drivers/hid/hid-msi-claw.c
>> +++ b/drivers/hid/hid-msi-claw.c
>
>[ ... ]
>
>> @@ -743,6 +836,389 @@ static const struct attribute_group claw_gamepad_attr_group = {
>>  	.is_visible = claw_gamepad_attr_is_visible,
>>  };
>>  
>> +/* Read RGB config from device */
>> +static int claw_read_rgb_config(struct hid_device *hdev)
>> +{
>> +	u8 data[4] = { 0x01, 0x00, 0x00, CLAW_RGB_FRAME_OFFSET };
>> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
>> +	u16 read_addr = drvdata->rgb_addr;
>> +	size_t len = ARRAY_SIZE(data);
>> +	int ret, i;
>> +
>> +	if (!drvdata->rgb_addr)
>> +		return -ENODEV;
>> +
>> +	/* Loop through all 8 pages of RGB data */
>> +	for (i = 0; i < 8; i++) {
>> +		drvdata->profile_pending = CLAW_RGB_PENDING;
>> +		data[1] = (read_addr >> 8) & 0xff;
>> +		data[2] = read_addr & 0x00ff;
>> +		ret = mcu_property_out(hdev, CLAW_COMMAND_TYPE_READ_PROFILE, data, len, 8);
>> +		if (ret)
>> +			return ret;
>
>If mcu_property_out() fails here, does drvdata->profile_pending remain set
>to CLAW_RGB_PENDING? If a subsequent unrelated profile event or stray ACK
>arrives, could claw_profile_event() incorrectly parse its payload as an RGB
>frame?
>
Hi AI bot,

I am responding for the humans that will be reading this.

Valid. Pending should be reset after errors. TBS, I need to find a way to push pending assignment into the mutex, or there will be issues as found in the other message.

>[ ... ]
>
>> +static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
>> +			  char *buf)
>> +{
>> +	struct hid_device *hdev = to_hid_device(dev);
>> +	struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
>
>Since the rgb_attr_group attributes are attached to the LED class device
>rather than the HID device, does to_hid_device(dev) result in an invalid
>pointer dereference here? Other sysfs functions in this patch seem to
>correctly use dev_get_drvdata(dev) to retrieve the led_classdev.
>

Also valid, though it doesn't crash when run it would be more proper to treat the led cdev as the parent.

>
>> @@ -758,6 +1234,13 @@ static void cfg_setup_fn(struct work_struct *work)
>>  		claw_remove(drvdata->hdev);
>>  	}
>>  
>> +	ret = claw_read_rgb_config(drvdata->hdev);
>> +	if (ret) {
>> +		dev_err(drvdata->led_mc.led_cdev.dev,
>> +			"Failed to setup device, can't read RGB config: %d\n", ret);
>> +		claw_remove(drvdata->hdev);
>> +	}
>
>If claw_remove() is called from within the cfg_setup worker thread, and
>claw_remove() calls cancel_delayed_work_sync(&drvdata->cfg_setup), won't
>this deadlock waiting for itself to complete?

Since it's a void function I can't return an error. I suppose it can just dev_err and return without initializing any sysfs attributes.

>Also, since there is no return statement after claw_remove() is called in
>these error blocks, will this proceed to execute the rest of the function on
>a stopped device?
>
>[ ... ]
>
>> @@ -881,6 +1405,9 @@ static void claw_remove(struct hid_device *hdev)
>>  
>>  	if (drvdata->endpoint == CLAW_XINPUT_CFG_INTF_IN ||
>>  	    drvdata->endpoint == CLAW_DINPUT_CFG_INTF_IN) {
>> +		/* Block writes to brightness/multi_intensity during teardown */
>> +		drvdata->led_mc.led_cdev.brightness_set = NULL;
>
>Is it safe to dynamically nullify the brightness_set callback here? If a
>concurrent brightness update in the LED core checks the pointer, sees it as
>valid, and then this nullification happens before the actual function call,
>could it result in a NULL pointer dereference?

This was done specifically to address a timing issue with the LED core and userspace writing to brightness during teardown. Without this we will get a null pointer dereference when the brightness function calls mcu_property_out.

>> +		cancel_delayed_work_sync(&drvdata->rgb_queue);
>
>Since the LED class device is registered with devres
>(devm_led_classdev_multicolor_register), the sysfs attributes like effect,
>speed, and enabled remain accessible until after claw_remove() completes.
>
>If userspace writes to any of those attributes in this window, could
>mod_delayed_work() re-arm rgb_queue, leading to a use-after-free when
>drvdata is subsequently freed?
>

Swapping this and the sysfs remove after should solve it.

- 
>>  		sysfs_remove_group(&hdev->dev.kobj, &claw_gamepad_attr_group);
>>  		cancel_delayed_work_sync(&drvdata->cfg_setup);
>>  		cancel_delayed_work_sync(&drvdata->cfg_resume);
>