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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Use-After-Free and NULL pointer dereference risks during RGB interface teardown.
- [High] Use-After-Free due to incorrect workqueue cancellation order.
- [Medium] Lifecycle mismatch of sysfs groups causes kernfs warnings on device removal.
- [Medium] Gamepad initialization is fully disabled if the RGB configuration read fails.
--

commit 1d7a0aec4b983c2c64bceb50fe253ae0e8e94c94
Author: Derek J. Clark <derekjohn.clark@gmail.com>
HID: hid-msi: Add RGB control interface

This commit adds an RGB control interface for MSI Claw devices, parsing
profile and effect data to mirror the RGB effects available in Windows.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index e58d35dba5b40..4d267b896da72 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> @@ -846,16 +1372,44 @@ static void cfg_setup_fn(struct work_struct *work)
>  		return;
>  	}
>  
> -	/* Add sysfs attributes after we get the device state */
> -	ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
> +	ret = claw_read_rgb_config(drvdata->hdev);
>  	if (ret) {
>  		dev_err(&drvdata->hdev->dev,
> -			"Failed to setup device, can't create gamepad attrs: %d\n", ret);
> +			"Failed to setup device, can't read RGB config: %d\n", ret);
>  		return;
>  	}

If claw_read_rgb_config() fails, it returns early and skips registering the 
gamepad attributes. Should core gamepad functionality (like button mapping)
break if the cosmetic RGB feature fails to initialize?

> -	drvdata->gamepad_registered = true;
> +
> +	/* Add sysfs attributes after we get the device state */
> +	if (!drvdata->gamepad_registered) {
> +		ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);
> +		if (ret) {
> +			dev_err(&drvdata->hdev->dev,
> +				"Failed to setup device, can't create gamepad attrs: %d\n", ret);
> +			return;
> +		}
> +		drvdata->gamepad_registered = true;
> +	}
> +
> +	/* Add and enable RGB interface once we have the device state */
> +	if (!drvdata->rgb_registered) {
> +		ret = devm_led_classdev_multicolor_register(&drvdata->hdev->dev, &drvdata->led_mc);
> +		if (ret) {
> +			dev_err(&drvdata->hdev->dev,
> +				"Failed to setup device, can't create led device: %d\n", ret);
> +			return;
> +		}
> +
> +		ret = devm_device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);

Will this trigger a kernfs warning on device removal? 

The LED class device is unregistered via devres first, which calls 
device_del() and implicitly removes all sysfs entries. This makes the 
subsequent devres removal of claw_rgb_attr_group fail to find the directory, 
triggering a "can not remove '...', no directory" warning.

Would it be safer to assign this group to drvdata->led_mc.led_cdev.groups
before registration instead of using devm_device_add_group()?

> +		if (ret) {
> +			dev_err(&drvdata->hdev->dev,
> +				"Failed to setup device, can't create led attributes: %d\n", ret);
> +			return;
> +		}
> +		drvdata->rgb_registered = true;
> +	}
>  
>  	kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
> +	kobject_uevent(&drvdata->led_mc.led_cdev.dev->kobj, KOBJ_CHANGE);
>  }

[ ... ]

> @@ -979,10 +1553,14 @@ static void claw_remove(struct hid_device *hdev)
>  		return;
>  	}
>  
> +	/* Block writes to brightness/multi_intensity during teardown */
> +	drvdata->led_mc.led_cdev.brightness_set = NULL;

Can this race with __led_set_brightness() in the LED subsystem? If the LED 
core checks the brightness_set pointer and finds it valid, but gets preempted
while this runs without locking, could it then jump to a NULL pointer?

>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);

Since cfg_resume_fn() schedules cfg_setup, is it possible for cfg_setup to 
be re-armed here? 

If cfg_resume is currently running, cancelling cfg_setup first does nothing, 
and then cfg_resume will queue cfg_setup again before completing. This could 
leave cfg_setup pending after the driver data is freed.

> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
>  
>  	drvdata->gamepad_registered = false;
> +	drvdata->rgb_enabled = false;
>  
>  	hid_hw_close(hdev);
>  }

Because drvdata->rgb_registered remains true here, and the RGB sysfs 
attributes are devres-managed (meaning they persist until after claw_remove
returns), could a concurrent userspace write pass the rgb_registered check 
and call mod_delayed_work()? 

This would re-arm rgb_queue after it was cancelled, leading to a 
use-after-free when drvdata is eventually freed. Should 
drvdata->rgb_registered = false be set before cancelling the workqueue?

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