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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Holding a spinlock while calling sleeping functions causes a kernel panic.
- [High] `frame_lock` is acquired in process context without disabling interrupts, leading to a deadlock.
- [High] Re-arming of workqueue during teardown leads to Use-After-Free.
- [High] Early LED registration exposes sysfs attributes before initialization is complete, causing NULL pointer dereferences or Use-After-Free.
- [Medium] The `frame_lock` spinlock is never initialized.
- [Low] Mixing `goto` and scope-based cleanup helpers in the same function violates subsystem guidelines.
--

commit 7eda00d45299494b87bf52e044ab97d5c4a78385
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, supporting up
to 8 frames of sequential RGB zone data. It implements several lighting
effects mirroring the Windows driver functionality.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index bc0169c992ae8..8ea8f551b8e43 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c
[ ... ]
> @@ -309,15 +372,46 @@ static int claw_profile_event(struct claw_drvdata *drvdata, struct claw_command_
>  		for (i = 0; i < CLAW_KEYS_MAX; i++)
>  			codes[i] = (cmd_rep->data[6 + i]);
>  		break;
> +	case CLAW_RGB_PENDING:
> +		frame = (struct rgb_report *)cmd_rep->data;
> +		rgb_addr = drvdata->rgb_addr;
> +		read_addr = be16_to_cpu(frame->read_addr);
> +		frame_calc = (read_addr - rgb_addr) / CLAW_RGB_FRAME_OFFSET;
> +		if (frame_calc >= CLAW_RGB_MAX_FRAMES) {
> +			dev_err(drvdata->led_mc.led_cdev.dev, "Got unsupported frame index: %x\n",
> +				frame_calc);
> +			ret = -EINVAL;
> +			goto err_pending;
> +		}
> +		f_idx = frame_calc;
> +
> +		scoped_guard(spinlock, &drvdata->frame_lock) {

This function mixes a goto-based error path (err_pending) with a
scope-based cleanup helper (scoped_guard). Is it possible to convert the
error handling to use cleanup helpers, or remove the scoped guard? Subsystem
guidelines state that goto and cleanup helpers should not be mixed in the
same function.

[ ... ]
> +/* Apply solid effect (1 frame, all zones same color) */
> +static int claw_apply_monocolor(struct claw_drvdata *drvdata)
> +{
> +	struct mc_subled *subleds = drvdata->led_mc.subled_info;
> +	struct rgb_zone zone = { subleds[0].intensity, subleds[1].intensity,
> +				 subleds[2].intensity };
> +
> +	guard(spinlock)(&drvdata->frame_lock);

Are we acquiring the spinlock without disabling interrupts here? Since
frame_lock is also acquired in claw_profile_event() which runs in the
HID raw event interrupt context, could this lead to a deadlock if an
interrupt fires while this process context holds the lock? Using
guard(spinlock_irqsave) might be necessary to prevent this.

Also, does calling claw_write_rgb_state() while holding the frame_lock
spinlock cause an issue?

Looking at claw_write_rgb_state(), it acquires a mutex, allocates memory
with GFP_KERNEL inside claw_hw_output_report(), and can sleep via
wait_for_completion_interruptible_timeout(). Calling these sleeping functions
while holding a spinlock will trigger a scheduling while atomic panic.

> +	drvdata->rgb_frame_count = 1;
> +	claw_frame_fill_solid(&drvdata->rgb_frames[0], zone);
> +
> +	return claw_write_rgb_state(drvdata);
> +}
[ ... ]
>  static int claw_probe(struct hid_device *hdev, u8 ep)
> @@ -843,12 +1361,28 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
>  	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";
> +	drvdata->led_mc.led_cdev.brightness = 0x50;
> +	drvdata->led_mc.led_cdev.max_brightness = 0x64;
> +	drvdata->led_mc.led_cdev.color = LED_COLOR_ID_RGB;
> +	drvdata->led_mc.led_cdev.brightness_set = claw_led_brightness_set;
> +	drvdata->led_mc.num_colors = 3;
> +	drvdata->led_mc.subled_info = devm_kmemdup(&hdev->dev, claw_rgb_subled_info,
> +						   sizeof(claw_rgb_subled_info), GFP_KERNEL);
> +	if (!drvdata->led_mc.subled_info)
> +		return -ENOMEM;
> +
>  	mutex_init(&drvdata->cfg_mutex);
>  	mutex_init(&drvdata->profile_mutex);
>  	mutex_init(&drvdata->rom_mutex);
>  	init_completion(&drvdata->send_cmd_complete);

Is frame_lock missing an explicit initialization? It doesn't appear to be
initialized with spin_lock_init(). Relying on devm_kzalloc
zero-initialization for spinlocks can lead to kernel panics or lockdep
warnings on debug kernels.

>  	INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
>  	INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> +	INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);
> +
> +	ret = devm_led_classdev_multicolor_register(&hdev->dev, &drvdata->led_mc);
> +	if (ret)
> +		return ret;

By registering the LED device here, the sysfs attributes (like brightness)
are exposed to userspace before hid_set_drvdata() is called later in probe.

If userspace writes to brightness immediately, it will schedule rgb_queue.
When the work executes, will it encounter a NULL pointer dereference, since
claw_hw_output_report() relies on hid_get_drvdata() which hasn't been set
yet?

Additionally, if hid_hw_open() fails, the probe returns an error and devres
cleans up drvdata. Since the queued work isn't canceled on this error path,
could this result in a use-after-free when the work eventually runs?

>  
>  	/* For control interface: open the HID transport for sending commands. */
>  	ret = hid_hw_open(hdev);
[ ... ]
> @@ -910,10 +1444,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;
>  	cancel_delayed_work_sync(&drvdata->cfg_setup);
>  	cancel_delayed_work_sync(&drvdata->cfg_resume);
> +	cancel_delayed_work_sync(&drvdata->rgb_queue);
>  
>  	guard(mutex)(&drvdata->cfg_mutex);
> +	device_remove_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);

Does canceling the delayed work before removing the sysfs attributes leave
a race condition?

Since device_remove_group() is called after cancel_delayed_work_sync(), the
sysfs files (like effect or speed) are still active. If a concurrent write
from userspace occurs, it can call mod_delayed_work() and re-queue
rgb_queue.

Once claw_remove() finishes, devres will free drvdata. When the re-queued
work executes later, will it dereference the freed memory and cause a
use-after-free? Removing the sysfs group before canceling the work would
prevent this.

>  	device_remove_group(&hdev->dev, &claw_gamepad_attr_group);
>  	hid_hw_close(hdev);
>  }

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