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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Out-of-bounds write in `claw_profile_event` due to an off-by-one bounds check on `frame_calc`.
- [High] Sleeping in atomic context due to `mutex` acquisition in the HID `raw_event` callback.
- [High] Use-After-Free and data race due to devm LED unregistration happening after `rgb_queue` is cancelled.
- [High] Teardown order race condition causes leaked sysfs attributes and Use-After-Free.
--

commit 56238f43183718f9b78311c16ef55fefd8acc6e2
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add RGB control interface

This commit adds an RGB control interface for MSI Claw devices, managing
up to 8 sequential frames of RGB zone data. It mirrors Windows effects
and adds sysfs/LED class interfaces for configuration.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 6ccb9666daedf..06dc4290d2bb5 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -308,15 +369,46 @@ 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 > CLAW_RGB_MAX_FRAMES) {

Can this result in an out-of-bounds write?

Since CLAW_RGB_MAX_FRAMES is 8 and the rgb_frames array is sized to 8, a
frame_calc value of 8 would pass this check but write past the end of the
array.

Should this check be >= instead?

> +			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;
> +
> +		scoped_guard(mutex, &drvdata->frame_mutex) {

Could this mutex acquisition cause a sleep-in-atomic bug?

This function is called synchronously from claw_raw_event(), which is the
HID raw_event callback and typically runs in atomic context (softirq or
hardirq).

Should this use a spinlock or defer the work to a workqueue instead?

> +			memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
> +			       sizeof(struct rgb_frame));
> +
[ ... ]
> @@ -839,12 +1355,28 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
[ ... ]
> +	INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
> +
> +	ret = devm_led_classdev_multicolor_register(&hdev->dev, &drvdata->led_mc);
> +	if (ret)
> +		return ret;
[ ... ]
> @@ -906,7 +1438,11 @@ static void claw_remove(struct hid_device *hdev)
>  		return;
>  	}
>  
> +	/* Block writes to brightness/multi_intensity during teardown */
> +	drvdata->led_mc.led_cdev.brightness_set = NULL;
> +	device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
>  	device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);

Does the order here create a race condition with the delayed work?

If cfg_setup executes concurrently, it might call device_add_group()
immediately after device_remove_group() finishes. By the time
cancel_delayed_work_sync() runs, the setup might have already re-added
the sysfs attributes.

Would it be safer to cancel the delayed work before removing the sysfs
groups?

Additionally, because the LED class device is registered via devm in the
probe function, it won't be unregistered until after claw_remove() returns.

Since brightness_set is checked by the LED core without locks, could a
concurrent write to the brightness sysfs attribute slip past the NULL
check and rearm rgb_queue after it has been cancelled?

When devres later frees drvdata, could this rearmed delayed work cause
a use-after-free?

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