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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] The `speed_show` function incorrectly casts the LED class device to a HID device, resulting in an invalid pointer dereference and a guaranteed kernel panic.
- [Critical] Missing bounds check on `frame->frame_count` from the USB device leads to an out-of-bounds read of kernel memory.
- [High] Re-arming `rgb_queue` from sysfs attributes after it has been explicitly cancelled in `claw_remove` leads to a Use-After-Free.
- [High] The `guard(mutex)` macro in `claw_hw_output_report()` is incorrectly used inside a block scope, providing absolutely no synchronization.
- [High] Concurrent modification of shared RGB state without lock protection causes data races and memory corruption.
- [High] Every RGB state change triggers a `SYNC_TO_ROM` command, causing severe flash memory degradation.
- [Medium] An early return in `cfg_setup_fn` silently drops core gamepad sysfs attributes if RGB initialization fails.
--

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

> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -308,15 +368,52 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
>  		for (i = 0; i < CLAW_KEYS_MAX; i++)
>  			codes[i] = (cmd_rep->data[6 + i] != 0xff) ? cmd_rep->data[6 + i] : 0x00;
>  		break;
> +	case CLAW_RGB_PENDING:
> +		frame = (struct rgb_report *)cmd_rep->data;
> +		rgb_addr = drvdata->rgb_addr;
> +		read_addr = be16_to_cpu(frame->read_addr);
> +		frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> +		if (frame_calc > U8_MAX) {
> +			dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> +				frame_calc);
> +			ret = -EINVAL;
> +			goto err_pending;
> +		}
> +		f_idx = frame_calc;
> +
> +		if (f_idx >= CLAW_RGB_MAX_FRAMES) {
> +			dev_err(drvdata->led_mc.led_cdev.dev, "Got illegal frame index: %x\n",
> +				f_idx);
> +			ret = -EINVAL;
> +			goto err_pending;
> +		}
> +
> +		/* Always treat the first frame as the truth for these constants */
> +		if (f_idx == 0) {
> +			drvdata->rgb_frame_count = frame->frame_count;

drvdata->rgb_frame_count is assigned directly from the hardware packet.
Should there be a bounds check here to ensure it is less than or equal to
CLAW_RGB_MAX_FRAMES?

Later, claw_write_rgb_state() uses drvdata->rgb_frame_count as the upper
bound for a loop reading from drvdata->rgb_frames. If a device sends a
frame count greater than 8, could this read out-of-bounds kernel memory
and send it back to the USB device?

> +			/* Invert device speed (20-0) to sysfs speed (0-20) */
> +			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;
> +		}
> +
> +		memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> +		       sizeof(struct rgb_frame));

claw_profile_event() modifies drvdata->rgb_frames and
drvdata->rgb_frame_count from the HID raw event handler context.
Simultaneously, claw_rgb_queue_fn() runs in a workqueue and calls
functions that overwrite these exact same fields.

Since there are no locks protecting these updates, could a background
worker read torn state or corrupt the frame arrays while they are being
actively updated by the asynchronous device response?

[ ... ]

> @@ -759,6 +856,397 @@ 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)
> +{

[ ... ]

> +/* Send RGB configuration to device */
> +static int claw_write_rgb_state(struct claw_drvdata *drvdata)
> +{
> +	struct rgb_report report = { 0x01, 0x0000, CLAW_RGB_FRAME_OFFSET, 0x00,
> +			drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
> +			drvdata->led_mc.led_cdev.brightness };
> +	u16 write_addr = drvdata->rgb_addr;
> +	size_t len = sizeof(report);
> +	int f, ret;
> +
> +	if (!drvdata->rgb_addr)
> +		return -ENODEV;
> +
> +	if (!drvdata->rgb_frame_count)
> +		return -EINVAL;
> +
> +	guard(mutex)(&drvdata->rom_mutex);
> +	/* Loop through (up to) 8 pages of RGB data */
> +	for (f = 0; f < drvdata->rgb_frame_count; f++) {
> +		report.zone_data = drvdata->rgb_frames[f];
> +
> +		/* Set the MCU address to write the frame data to */
> +		report.read_addr = cpu_to_be16(write_addr);
> +
> +		/* Serialize the rgb_report and write it to MCU */
> +		ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_WRITE_PROFILE_DATA,
> +					    (u8 *)&report, len, 8);
> +		if (ret)
> +			return ret;
> +
> +		/* Increment the write addr by the offset for the next frame */
> +		write_addr += CLAW_RGB_FRAME_OFFSET;
> +	}
> +
> +	ret = claw_hw_output_report(drvdata->hdev, CLAW_COMMAND_TYPE_SYNC_TO_ROM, NULL, 0, 8);

Does sending CLAW_COMMAND_TYPE_SYNC_TO_ROM on every RGB state change
cause excessive flash memory degradation?

Because claw_apply_rgb_state() is triggered by unthrottled sysfs writes
(such as rapidly changing a brightness slider), this might cause hundreds
of NVRAM writes per second and potentially degrade the MCU's flash
endurance.

[ ... ]

> +static ssize_t effect_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> +	struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
> +	int ret;
> +
> +	ret = sysfs_match_string(claw_rgb_effect_text, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	drvdata->rgb_effect = ret;
> +	mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
> +
> +	return count;
> +}

Is there a potential use-after-free here if sysfs attributes are written
after the device is removed?

In claw_remove(), the driver attempts to tear down the worker using
cancel_delayed_work_sync(&drvdata->rgb_queue). However, the sysfs files
and LED endpoints remain active because devm cleanup executes after
claw_remove() completes.

A concurrent write to a sysfs attribute like effect_store could call
mod_delayed_work() and re-arm the work item, causing it to execute after
drvdata has been freed.

[ ... ]

> +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);

Is the dev parameter passed to speed_show actually a hid_device?

The rgb_attr_group sysfs group is attached to the LED class device
(led_cdev.dev), not the parent HID device. The macro to_hid_device(dev)
blindly performs a container_of cast, which will yield an invalid pointer
for hdev.

Dereferencing it with hid_get_drvdata(hdev) will likely cause a kernel
panic when this world-readable attribute is accessed.

[ ... ]

> @@ -772,6 +1260,13 @@ static void cfg_setup_fn(struct work_struct *work)
>  		return;
>  	}
>  
> +	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);
> +		return;
> +	}
> +
>  	/* Add sysfs attributes after we get the device state */
>  	ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);

If claw_read_rgb_config() fails due to a temporary USB error or
unsupported firmware, this early return skips the subsequent
devm_device_add_group call.

Does this unintentionally break the device's core gamepad configuration
interface simply because the RGB initialization failed?

This isn't a new bug introduced by this patch, but does the guard() in
claw_hw_output_report() provide any synchronization?

claw_hw_output_report() {
    if (timeout) {
        guard(mutex)(&drvdata->cfg_mutex);
        reinit_completion(&drvdata->send_cmd_complete);
    }
    ...
}

Because the guard is declared inside the block, the mutex is immediately
released at the closing brace before hid_hw_output_report() and
wait_for_completion_interruptible_timeout() are ever called. This seems
to leave the I/O operations unprotected against concurrent threads, which
this patch introduces via the new RGB workers.

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