Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A race condition in `claw_remove()` allows the `rgb_queue` delayed work to be re-armed after it has been cancelled, leading to a Use-After-Free of the `drvdata` structure.
- [Medium] A hardcoded LED class device name prevents supporting multiple connected devices.
--
commit 151b50cdc17e18af52c3d79eb6a71e3b5e4583d8
Author: Derek J. Clark [off-list ref]
HID: hid-msi: Add RGB control interface
Adds RGB control interface for MSI Claw devices using a unique frame-based
RGB protocol. Mirrors Windows effects using a delayed work queue to apply
state to the device.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -954,6 +1572,7 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
[ ... ]
quoted hunk ↗ jump to hunk
if (!drvdata->bmap_support)
dev_dbg(&hdev->dev, "M-Key mapping is not supported. Update firmware to enable.\n");
+ drvdata->led_mc.led_cdev.name = "msi_claw:rgb:joystick_rings";
[Severity: Medium]
Does hardcoding the LED class device name prevent multiple MSI Claw devices
from being connected at the same time?
If two devices are connected, it seems the second one will fail to register
its LED class device due to a sysfs name collision. Would it be safer to
include a unique identifier like dev_name(&hdev->dev) in the string?
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1030,6 +1662,7 @@ static void claw_remove(struct hid_device *hdev)
[ ... ]
quoted hunk ↗ jump to hunk
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]
Can a race condition here lead to a use-after-free of drvdata?
In sysfs store functions like effect_store(), rgb_registered is checked
under the registration_lock, but mod_delayed_work() is called after dropping
the lock.
If a sysfs thread is preempted right after dropping the lock, claw_remove()
could set rgb_registered to false, execute cancel_delayed_work_sync(), and
block on device_remove_group() waiting for the sysfs operation to finish.
Once the sysfs thread resumes, it would call mod_delayed_work(), effectively
re-arming the workqueue after it was supposed to be cancelled. When
claw_remove() completes, drvdata is freed, and the delayed work timer firing
later would result in a use-after-free.
Would moving cancel_delayed_work_sync() after the device_remove_group() and
led_classdev_multicolor_unregister() calls prevent this race?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529072111.7565-1-derekjohn.clark@gmail.com?part=3