Re: [PATCH v10 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] Race condition during device teardown allows the `rgb_queue` delayed work to be queued after cancellation, leading to a Use-After-Free.
- [High] Early return in `claw_profile_event()` skips state machine reset, leaving the device stuck in `CLAW_RGB_PENDING` state.
- [Medium] Incorrect suspend handling for the LED class device leads to dropped 'off' events and potential re-queuing of work during suspend.
- [Low] RGB configuration state variables are read and modified without synchronization, leading to data races.
--

commit 4395753635f4bb8533afa93923c64af621829712
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add RGB control interface

This patch adds an RGB control interface for MSI Claw devices. It configures
a 9-zone multicolor LED device with up to 8 animation frames and exposes sysfs
attributes to control the effects, speed, and enabled state.

diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
[ ... ]
@@ -342,6 +403,34 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
[ ... ]
> +	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);
> +		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 -EINVAL;

[Severity: High]
Does this early return skip resetting the state machine?

By returning directly here, we bypass the drvdata->profile_pending assignment
at the bottom of the function. This appears to leave the device permanently
locked in the CLAW_RGB_PENDING state.

> +		}
[ ... ]
@@ -882,32 +971,553 @@ static const struct attribute_group claw_gamepad_attr_group = {
[ ... ]
> +static ssize_t effect_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
[ ... ]
> +	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;
> +}

[Severity: Low]
Can this modification of drvdata->rgb_effect race with reads from
claw_apply_rgb_state()?

It looks like rgb_effect, rgb_enabled, and rgb_speed are being read and
written without synchronization, which could lead to an inconsistent
configuration state if multiple sysfs operations happen concurrently.

[ ... ]
@@ -1030,6 +1662,7 @@ static void claw_remove(struct hid_device *hdev)
[ ... ]
>  	if (gp_registered)
>  		device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
>  
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
> +	if (rgb_registered) {
> +		device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> +		led_classdev_multicolor_unregister(&drvdata->led_mc);
> +	}
> +
>  	hid_hw_close(hdev);
>  }

[Severity: High]
Could this ordering cause a use-after-free during device teardown?

If a sysfs operation like effect_store() is preempted, it could complete
after cancel_delayed_work_sync() finishes. Because device_remove_group() and
led_classdev_multicolor_unregister() haven't removed the interfaces yet,
the sysfs handler can still successfully call mod_delayed_work(), which will
re-queue the work to run after drvdata has been freed.

[ ... ]
@@ -1106,6 +1749,7 @@ static int claw_suspend(struct hid_device *hdev)
>  
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
>  
>  	return 0;
>  }

[Severity: Medium]
Is this cancellation sufficient to safely suspend the LED device?

Without setting LED_CORE_SUSPENDRESUME in the LED class device flags during
probe, the LED subsystem might not know to properly coordinate the suspend.
This could potentially lead to dropped 'off' events or concurrent timers
re-queuing this work after it has been cancelled here.

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