Re: [PATCH v2] HID: steelseries: Add MSI Raider A18 HX A9WJG RGB support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Type confusion and undefined behavior for SRWS1 devices in steelseries_headset_raw_event().
- [High] DMA API violation: devm_kzalloc buffer passed to usb_control_msg() for DMA.
- [Low] Build failure due to missing 'err_close' label on the error path of steelseries_probe().

Pre-existing issues:
- [High] Kernel crash due to uninitialized workqueue object when battery registration fails.
--

commit a675d7bbdbccedaef6c839f514204764ec470211
Author: David Glushkov <david.glushkov@sntiq.com>

HID: steelseries: Add MSI Raider A18 HX A9WJG RGB support

This commit adds support for MSI Raider A18 HX A9WJG RGB keyboards via the
steelseries driver. It introduces multicolor LED support using a devres
managed buffer and adds device specific quirks mapping.

> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c

[ ... ]

> +static int steelseries_msi_rgb_register(struct steelseries_device *sd)
> +{
> +	struct hid_device *hdev = sd->hdev;
> +	struct led_classdev *led_cdev;
> +
> +	sd->rgb_buf = devm_kzalloc(&hdev->dev, 524, GFP_KERNEL);

[Severity: High]
Memory allocated with devm allocators includes a devres header, which breaks
cacheline alignment. Does passing this sd->rgb_buf to usb_control_msg() in
steelseries_msi_rgb_set_blocking() cause memory corruption on architectures
with non-coherent DMA?

> +	if (!sd->rgb_buf)
> +		return -ENOMEM;

[ ... ]

> @@ -567,7 +731,17 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>  	if (ret)
>  		return ret;
>  
> -	if (steelseries_headset_battery_register(sd) < 0)
> +	if (sd->quirks & STEELSERIES_MSI_RGB) {
> +		ret = steelseries_msi_rgb_register(sd);
> +		if (ret) {
> +			hid_err(hdev, "Failed to register MSI RGB LEDs: %d\n", ret);
> +			goto err_close;

[Severity: Low]
Since there is no err_close label defined in steelseries_probe() or the rest
of the file, does this goto statement cause a build failure?

> +		}
> +		return 0;
> +	}
> +
> +	if (sd->quirks & (STEELSERIES_ARCTIS_1 | STEELSERIES_ARCTIS_9) &&
> +	    steelseries_headset_battery_register(sd) < 0)
>  		hid_err(sd->hdev,
>  			"Failed to register battery for headset\n");

[Severity: High]
This is a pre-existing issue, but if steelseries_headset_battery_register()
fails, the error is logged but probe successfully returns 0. This leaves
sd->battery_work zero-initialized.

Will a subsequent raw input event or device removal cause a kernel panic when
schedule_delayed_work() or cancel_delayed_work_sync() is called on this
uninitialized structure?

[ ... ]

> @@ -635,7 +812,7 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
>  	unsigned long flags;
>  
>  	/* Not a headset */
> -	if (hdev->product == USB_DEVICE_ID_STEELSERIES_SRWS1)
> +	if (!(sd->quirks & (STEELSERIES_ARCTIS_1 | STEELSERIES_ARCTIS_9)))
>  		return 0;

[Severity: High]
For SRWS1 devices, the driver data is a completely different structure, not
a struct steelseries_device. As a result, sd aliases a kernel pointer.

Does reading sd->battery_capacity earlier in the function, and checking
sd->quirks here, lead to type confusion and undefined behavior since it
replaces the safe hdev->product ID check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531222123.179923-1-david.glushkov@sntiq.com?part=1