Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Tearing down the `rgb_queue` delayed work before unregistering the sysfs attributes in `claw_remove()` allows the work to be re-armed, leading to a Use-After-Free.
--
commit e775f842567caa50d4c5a9bd87b14e6d6c8ffff6
Author: Derek J. Clark [off-list ref]
Subject: HID: hid-msi: Add RGB control interface
This commit adds an RGB control interface for MSI Claw devices. It supports
configuring up to 8 sequential frames of RGB zone data across 9 zones,
mirroring effects available in the official Windows driver.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
index 5f20102eae69d..96710d80543d4 100644
--- a/drivers/hid/hid-msi.c
+++ b/drivers/hid/hid-msi.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1043,11 +1675,21 @@ static void claw_remove(struct hid_device *hdev)
gp_registered = smp_load_acquire(&drvdata->gp_registered);
/* Pairs with smp_load_acquire in attribute show/store functions */
smp_store_release(&drvdata->gp_registered, false);
+ /* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
+ rgb_registered = smp_load_acquire(&drvdata->rgb_registered);
+ /* Pairs with smp_load_acquire in attribute show/store functions */
+ smp_store_release(&drvdata->rgb_registered, false);
}
if (gp_registered)
device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
+ cancel_delayed_work_sync(&drvdata->rgb_queue);
[Severity: High]
Could this lead to a use-after-free if the rgb_queue delayed work is re-armed
while claw_remove() is running?
If a sysfs writer (like effect_store()) reads rgb_registered as true under
registration_lock and gets preempted, claw_remove() can clear rgb_registered,
call cancel_delayed_work_sync(), and then block in device_remove_group()
waiting for the sysfs writer to drop its active kernfs reference.
The sysfs writer could then resume, call mod_delayed_work() to re-arm the
workqueue, and finish:
sysfs writer
scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
if (!smp_load_acquire(&drvdata->rgb_registered))
return -ENODEV;
}
// Preempted here
claw_remove()
smp_store_release(&drvdata->rgb_registered, false);
cancel_delayed_work_sync(&drvdata->rgb_queue);
device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
// Blocks waiting for sysfs writer
sysfs writer (resumes)
mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));
// Completes
claw_remove()
// Unblocks and completes
After claw_remove() finishes, drvdata is freed by devres. When the re-armed
delayed work executes 50ms later, it would dereference the freed drvdata,
causing a use-after-free crash.
Should cancel_delayed_work_sync() be called after the sysfs attributes and LED
class device are fully unregistered?
quoted hunk ↗ jump to hunk
+ 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);
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260525020543.519082-1-derekjohn.clark@gmail.com?part=3