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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The global bounds check `if (size < 4)` incorrectly drops valid, shorter unsolicited status reports.

Pre-existing issues:
- [High] Concurrent sysfs unbind leads to Use-After-Free of sibling `hid_device` and `steelseries_device`.
- [High] Type confusion when retrieving the sibling HID device can lead to an out-of-bounds memory read.
--

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

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

This patch adds support for the Arctis Nova 7 Gen2 headsets. It introduces a
new parser for unsolicited battery and connection updates on a second HID
interface, and adds eight new USB product IDs to the supported devices list.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index 689435c60ecbd..31d31f3f4b0e9 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c
[ ... ]
> @@ -162,6 +169,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]
Could this global bounds check prematurely discard valid unsolicited battery
and connection updates?

The new opcodes 0xb7, 0xb9, and 0xbb only require up to data[1] to be valid.
If the headset firmware sends these unsolicited updates as 2-byte short USB
HID packets reflecting their actual content size, this check will drop them.

[ ... ]
> @@ -180,6 +211,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 is a pre-existing issue, but the addition of these new devices using the
async/sync interface mechanism heavily exercises it. Is it possible for a
concurrent sysfs unbind to cause a use-after-free?

During the async interface probe, steelseries_get_sibling_sd() calls
usb_get_intfdata(sibling_intf) without holding the device lock for the sibling
interface. If a user concurrently unbinds the sibling interface (e.g., via
sysfs), usbhid destroys the hid_device. This causes the probe path to
locklessly dereference the freed sibling_hdev->driver:

	sibling_hdev = usb_get_intfdata(sibling_intf);
	if (!sibling_hdev || sibling_hdev->driver != &steelseries_arctis_driver)
		return NULL;

	sd = hid_get_drvdata(sibling_hdev);
	if (sd)
		kref_get(&sd->refcnt);

It then calls kref_get(&sd->refcnt) on the associated private data, modifying
freed memory.

[Severity: High]
This isn't a bug introduced by this patch, but adding these devices expands
the exposure to a pre-existing type confusion issue. Could this lead to an
out-of-bounds memory read?

In steelseries_get_sibling_sd(), the driver extracts the private data via
usb_get_intfdata() and assumes it is a struct hid_device *:

	sibling_intf = usb_ifnum_to_if(usb_dev, interface_num);
	if (!sibling_intf)
		return NULL;

	sibling_hdev = usb_get_intfdata(sibling_intf);
	if (!sibling_hdev || sibling_hdev->driver != &steelseries_arctis_driver)
		return NULL;

If a crafted USB device is plugged in where the sibling interface (e.g.,
interface 0 or 3) is a non-HID class like USB Audio, a different driver will
bind and store its own private structure in intfdata.

The driver field sits hundreds of bytes deep into struct hid_device. Reading
it from a smaller structure belonging to another driver will read out-of-bounds
of the allocated SLUB object.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782840133.git.srimanachanta@gmail.com?part=9