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

[sashiko-bot@kernel.org]

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 <derekjohn.clark@gmail.com>
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.

> 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

[ ... ]

> @@ -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?

> +	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