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

[sashiko-bot@kernel.org]

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 is cancelled, leading to a Use-After-Free.
- [High] Failing to unregister the LED class device if `device_add_group()` fails leaves it registered but sets `rgb_registered = false`, causing list corruption upon retry.
--

commit 35b79b95f2f2fd70b7faaa568ae55b37643edb83
Author: Derek J. Clark <derekjohn.clark@gmail.com>

HID: hid-msi: Add RGB control interface
    
Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
fairly unique RGB interface. It has 9 total zones (4 per joystick ring
and 1 for the ABXY buttons), and supports up to 8 sequential frames of
RGB zone data.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index 193435ec9386c..61917902e38d3 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -871,31 +960,521 @@ static const struct attribute_group claw_gamepad_attr_group = {

[ ... ]

> +try_rgb:
> +	/* Add and enable RGB interface once we have the device state */
> +	/* Pairs with smp_store_release from below */
> +	if (!smp_load_acquire(&drvdata->rgb_registered) && rgb_ready) {
> +		ret = devm_led_classdev_multicolor_register(&drvdata->hdev->dev, &drvdata->led_mc);
> +		if (ret) {
> +			dev_err(&drvdata->hdev->dev,
> +				"Failed to create led device: %d\n", ret);
> +			goto update_kobjects;
> +		}
> +
> +		ret = device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> +		if (ret) {
> +			dev_err(&drvdata->hdev->dev,
> +				"Failed to create RGB attrs: %d\n", ret);
> +			goto update_kobjects;
> +		}

[Severity: High]
If device_add_group() fails here, we jump to update_kobjects without rolling
back the devm_led_classdev_multicolor_register() call, and rgb_registered
remains false.

On a subsequent system resume, cfg_resume_fn() will see rgb_registered is
false and schedule cfg_setup_fn() again:

    if (!smp_load_acquire(&drvdata->rgb_registered))
        schedule_delayed_work(&drvdata->cfg_setup, msecs_to_jiffies(500));

Will the second call to devm_led_classdev_multicolor_register() with the exact
same drvdata->led_mc structure corrupt the leds_list, since led_cdev->node
is already in the list from the first call?

> +
> +		/* Pairs with smp_load_acquire in attribute show/store functions */
> +		smp_store_release(&drvdata->rgb_registered, true);
> +	}

[ ... ]

> @@ -1021,11 +1622,15 @@ static void claw_remove(struct hid_device *hdev)
>  		return;
>  	}
>  
> +	/* Block writes to brightness/multi_intensity during teardown */
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
>  
>  	/* Pairs with smp_load_acquire in attribute show/store functions */
>  	smp_store_release(&drvdata->gamepad_registered, false);
> +	/* Pairs with smp_load_acquire in attribute show/store functions */
> +	smp_store_release(&drvdata->rgb_registered, false);

[Severity: High]
Could setting rgb_registered to false after cancelling the delayed work
cause a use-after-free here?

Because sysfs attributes are managed by devres, they remain active until
claw_remove() returns. A concurrent sysfs write could check rgb_registered
and then get preempted:

    /* in effect_store() */
    if (!smp_load_acquire(&drvdata->rgb_registered))
        return -ENODEV;
    /* Preempted here */

Meanwhile, claw_remove() runs:

    cancel_delayed_work_sync(&drvdata->rgb_queue);
    smp_store_release(&drvdata->rgb_registered, false);

Then the sysfs writer resumes and queues the work again:

    mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));

After claw_remove() returns, devres will free drvdata. When the queued work
eventually executes claw_rgb_queue_fn(), will it access the freed drvdata?

Would clearing rgb_registered before calling cancel_delayed_work_sync()
prevent the work from being re-queued?

>  
>  	hid_hw_close(hdev);
>  }

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