Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Data races and locking mismatches when accessing RGB configuration variables and LED core state.
Pre-existing issues:
- [High] Use-after-free on device teardown due to missing cancellation of delayed works.
--
--- Patch [3]: [PATCH v13 3/4] HID: hid-msi: Add RGB control interface ---
commit d67de53a026c42fe9260a66fca23392240a52c26
Author: Derek J. Clark [off-list ref]
HID: hid-msi: Add RGB control interface
This commit adds an RGB control interface for MSI Claw devices. It provides
support for configuring up to 9 LED zones via an MCU memory interface and
sets up the corresponding sysfs attributes and LED class device.
Link: https://patch.msgid.link/20260529072111.7565-4-derekjohn.clark@gmail.com (local)
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 023d63cee6b06..5cb85fc2f0534 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -349,6 +410,38 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
for (i = 0; i < CLAW_KEYS_MAX; i++)
codes[i] = (mkeys->codes[i]);
break;
+ case CLAW_RGB_PENDING:
+ frame = (struct claw_rgb_report *)cmd_rep->data;
[ ... ]
quoted hunk ↗ jump to hunk
+ scoped_guard(spinlock_irqsave, &drvdata->frame_lock) {
+ memcpy(&drvdata->rgb_frames[f_idx], &frame->zone_data,
+ sizeof(struct rgb_frame));
+
+ /* Only use frame 0 for remaining variable assignment */
+ if (f_idx != 0)
+ break;
+
+ 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;
+ }
+
+ break;
[Severity: Medium]
Is there a locking mismatch for rgb_speed? Here in claw_profile_event(),
rgb_speed is updated while holding frame_lock. However, in speed_store(),
it is updated while holding profile_lock.
Also, does this direct modification of the LED class device internal state
need synchronization? Updating brightness and subled_info[x].intensity here
is done without holding the standard led_access lock used by the LED core,
which could cause data races with concurrent sysfs accesses.
[ ... ]
quoted hunk ↗ jump to hunk
+/* Send RGB configuration to device */
+static int claw_write_rgb_state(struct claw_drvdata *drvdata)
+{
+ struct claw_rgb_report report = { {0x01, 0}, CLAW_RGB_FRAME_OFFSET, 0x00,
+ drvdata->rgb_frame_count, 0x09, drvdata->rgb_speed,
+ drvdata->led_mc.led_cdev.brightness };
[Severity: Medium]
Does claw_write_rgb_state() need to hold a lock when reading rgb_speed?
It appears to be read locklessly here, but is updated with a lock elsewhere.
This also applies to other variables like rgb_effect and rgb_enabled read
locklessly in claw_apply_rgb_state() and sysfs show functions.
[ ... ]
quoted hunk ↗ jump to hunk
+static ssize_t speed_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ /* 0 is fastest, invert value for intuitive userspace speed */
+ speed = 20 - val;
+
+ scoped_guard(spinlock_irqsave, &drvdata->profile_lock)
+ drvdata->rgb_speed = speed;
+
+ mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
+
+ return count;
+}
[Severity: Medium]
This is where rgb_speed is written using profile_lock instead of the
frame_lock used in claw_profile_event().
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1088,6 +1727,7 @@ static void claw_remove(struct hid_device *hdev)
{
struct claw_drvdata *drvdata = hid_get_drvdata(hdev);
bool gp_registered;
+ bool rgb_registered;
if (!drvdata)
return;
[ ... ]
quoted hunk ↗ jump to hunk
if (gp_registered)
device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
+ if (rgb_registered) {
+ device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
+ led_classdev_multicolor_unregister(&drvdata->led_mc);
+ }
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
+
hid_hw_close(hdev);
}
[Severity: High]
This is a pre-existing issue, but does this teardown path leak delayed works?
The driver initializes the cfg_setup and cfg_resume delayed works during
probe, but they don't appear to be cancelled here in claw_remove().
If the device is removed while either of those works is pending, could they
execute after the devres-allocated drvdata is freed, causing a
use-after-free crash?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720031549.2272658-1-derekjohn.clark@gmail.com?part=3