Re: [PATCH] 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:
- [High] Type Confusion / Out-of-Bounds Read due to unsafe cast to `usb_interface`
- [High] `steelseries_remove()` unconditionally calls `cancel_delayed_work_sync()` on an uninitialized `delayed_work` struct.
- [Medium] Missing `hid_hw_stop()` and `hid_hw_close()` on probe error path.
- [Medium] Missing runtime PM management during direct USB control transfer, breaking LED updates when the device is idle.
--

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

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

This patch adds support for MSI Raider A18 HX A9WJG internal RGB devices
(KLC and ALC) to the steelseries HID driver. It exposes them as multicolor
LED class devices and sets uniform RGB values across zones.

> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index f98435631aa18..0492814791753 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c

[ ... ]

> @@ -528,6 +542,140 @@ static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t us
>  		hdev->rdesc[2] == 0xff;
>  }
>  
> +static const struct dmi_system_id steelseries_msi_rgb_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star International Co., Ltd."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Raider A18 HX A9WJG"),
> +			DMI_MATCH(DMI_BOARD_NAME, "MS-182L"),
> +		},
> +	},
> +	{ }
> +};
> +
> +static struct usb_device *steelseries_hid_to_usb_dev(struct hid_device *hdev)
> +{
> +	return interface_to_usbdev(to_usb_interface(hdev->dev.parent));
> +}

[Severity: High]
Could this cause an out-of-bounds read if the device is not a USB device?

If a local user injects a virtual HID device with the MSI RGB vendor and
product IDs via the uhid subsystem, the device's parent will be the uhid
device, not a usb_interface.

Calling to_usb_interface() and interface_to_usbdev() without first verifying
the transport using hid_is_usb(hdev) could interpret the uhid parent memory
as a usb_interface, leading to wild pointer dereferences and a panic.

> +
> +static bool steelseries_msi_rgb_is_interface0(struct hid_device *hdev)
> +{
> +	return to_usb_interface(hdev->dev.parent) ==
> +	       usb_ifnum_to_if(steelseries_hid_to_usb_dev(hdev), 0);
> +}
> +
> +#if STEELSERIES_HAS_LEDS_MULTICOLOR
> +
> +static int steelseries_msi_rgb_set_blocking(struct led_classdev *led_cdev,
> +					    enum led_brightness brightness)
> +{

[ ... ]

> +	if (hdev->product == USB_DEVICE_ID_STEELSERIES_MSI_KLC) {
> +		sd->rgb_buf[2] = 0x66;
> +		for (i = 0; i < ARRAY_SIZE(keys); i++) {
> +			sd->rgb_buf[4 + i * 4] = keys[i];
> +			sd->rgb_buf[5 + i * 4] = r;
> +			sd->rgb_buf[6 + i * 4] = g;
> +			sd->rgb_buf[7 + i * 4] = b;
> +		}
> +	} else {
> +		sd->rgb_buf[2] = 0x06;
> +		for (i = 0; i < ARRAY_SIZE(alc_zones); i++) {
> +			sd->rgb_buf[4 + i * 4] = alc_zones[i];
> +			sd->rgb_buf[5 + i * 4] = r;
> +			sd->rgb_buf[6 + i * 4] = g;
> +			sd->rgb_buf[7 + i * 4] = b;
> +		}
> +	}
> +
> +	ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +			      HID_REQ_SET_REPORT,
> +			      USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +			      0x0300, 0,
> +			      sd->rgb_buf, 524, USB_CTRL_SET_TIMEOUT);

[Severity: Medium]
Will this USB control message fail if the device has entered runtime suspend?

Because this directly uses usb_control_msg() and bypasses the HID transport
layer, it doesn't appear to wake the device. Without explicitly waking the
device via hid_hw_power(hdev, PM_HINT_FULLON) or usb_autopm_get_interface(),
the transfer might drop LED updates when the keyboard is idle.

> +
> +	mutex_unlock(&sd->rgb_lock);
> +	return ret < 0 ? ret : 0;
> +}

[ ... ]

> @@ -567,7 +721,15 @@ 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);
> +		return ret;
> +	}

[Severity: Medium]
Does this error path leak the hardware state initialized by hid_hw_start()
and hid_hw_open() called earlier in steelseries_probe()?

Returning directly here skips calling hid_hw_stop() and hid_hw_close(),
which could leave active URBs and orphaned hidinput/hidraw device nodes
exposed to userspace.

> +
> +	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]
Since steelseries_headset_battery_register() and INIT_DELAYED_WORK() are
skipped for MSI RGB devices here, what happens when the device is removed?

If steelseries_remove() unconditionally calls cancel_delayed_work_sync() on
sd->battery_work, passing a zero-initialized work struct will trigger a
debug objects (ODEBUG) warning and splat on device removal.

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