Re: [PATCH v4 10/10] HID: steelseries: Add support for Arctis Nova 7 Gen2 family

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The unconditional minimum size check in the Gen2 parsing function may silently drop valid unsolicited status updates.

Pre-existing issues:
- [High] Unsafe fetching of the sibling interface's private data lacks driver validation and synchronization, leading to Type Confusion and Use-After-Free.
--

commit ca415cb624472143e783db3389a867c4e203e6c8
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Add support for Arctis Nova 7 Gen2 family

This commit adds support for the Arctis Nova 7 Gen2 headsets.
It introduces a new parsing helper for the Gen2 status format and uses an
async interface to handle unsolicited battery and connection updates without
polling.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c
[ ... ]
> @@ -163,6 +170,30 @@ static void steelseries_arctis_9_parse_status(struct steelseries_device *sd,
>  	}
>  }
>  
> +static void steelseries_arctis_nova_7_gen2_parse_status(struct steelseries_device *sd,
> +							u8 *data, int size)
> +{
> +	if (size < 4)
> +		return;

[Severity: Medium]
Will this strict minimum size check silently drop valid short packets?

Since opcodes like 0xb7, 0xb9, and 0xbb only appear to require 2 bytes of
payload (e.g., data[0] and data[1]), if the hardware sends an unsolicited
report as a 2 or 3 byte packet, it seems it would be dropped before
processing.

> +
> +	switch (data[0]) {
> +	case 0xb0:
> +		sd->headset_connected = (data[1] == 0x03);
> +		sd->battery_capacity = data[2];
> +		sd->battery_charging = (data[3] == 0x01);
> +		break;
> +	case 0xb7:
> +		sd->battery_capacity = data[1];
> +		break;
> +	case 0xb9:
> +		sd->headset_connected = (data[1] == 0x03);
> +		break;
> +	case 0xbb:
> +		sd->battery_charging = (data[1] == 0x01);
> +		break;
> +	}
> +}
> +
>  /*
>   * Device info definitions
>   */
> @@ -181,6 +212,14 @@ static const struct steelseries_device_info arctis_9_info = {
>  	.parse_status = steelseries_arctis_9_parse_status,
>  };
>  
> +static const struct steelseries_device_info arctis_nova_7_gen2_info = {
> +	.sync_interface = 3,
> +	.async_interface = 5,

[Severity: High]
This isn't a bug introduced by this patch, but since these devices use an
async interface, does the sibling interface lookup in 
steelseries_get_sibling_hdev() have adequate locking and validation?

When a device probes the async interface, it appears
steelseries_get_sibling_hdev() fetches intfdata from the sibling interface
via usb_get_intfdata() locklessly.

If the sibling interface (interface 3) is unbound via sysfs and bound to
another driver (like usbfs), or physically unplugged concurrently, could
this return a foreign private context or a soon-to-be-freed pointer?

Then in steelseries_arctis_probe(), this unvalidated pointer is cast and
dereferenced to get the driver data:

    sd = hid_get_drvdata(master_hdev);
    kref_get(&sd->refcnt);

Could this lead to type confusion or a use-after-free if the sibling driver
is no longer the expected usbhid driver?

> +	.capabilities = SS_CAP_BATTERY,
> +	.request_status = steelseries_arctis_nova_request_status,
> +	.parse_status = steelseries_arctis_nova_7_gen2_parse_status,
> +};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623172310.272708-1-srimanachanta@gmail.com?part=10