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

[sashiko-bot@kernel.org]

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?

[ ... ]

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

[ ... ]

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

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?

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

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

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