Re: [PATCH v7 7/8] HID: steelseries: Add async status interface support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `steelseries_get_sibling_sd` function uses `usb_get_intfdata` on a sibling USB interface and unconditionally casts the result to `struct hid_device *`, which leads to type confusion if a different driver binds to it.
- [High] A concurrent sysfs unbind of the HID driver from the sync interface can cause `steelseries_get_sibling_sd` to return a freed `steelseries_device` (sd) and call `kref_get` on it, leading to a Use-After-Free.
--

--- Patch [7]: [PATCH v7 7/8] HID: steelseries: Add async status interface support ---
commit 816a244e480f8271ebf1dbc58fbdbe5e0bdbafab
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Add async status interface support

Some headsets expose a second HID interface that sends battery and
connection updates on its own. Watching that interface lets the driver
stop polling the sync interface.

> 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
[ ... ]
> @@ -318,6 +320,53 @@ static int steelseries_battery_register(struct steelseries_device *sd)
> +static struct steelseries_device *
> +steelseries_get_sibling_sd(struct hid_device *hdev, int interface_num)
> +{
> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct usb_interface *sibling_intf;
> +	struct hid_device *sibling_hdev;
> +	struct steelseries_device *sd = NULL;
> +
> +	sibling_intf = usb_ifnum_to_if(usb_dev, interface_num);
> +	if (!sibling_intf)
> +		return NULL;
[ ... ]
> +	if (!device_trylock(&sibling_intf->dev))
> +		return NULL;
> +	if (sibling_intf->dev.driver) {
> +		sibling_hdev = usb_get_intfdata(sibling_intf);
> +		if (sibling_hdev &&
> +		    sibling_hdev->driver == &steelseries_arctis_driver) {

[Severity: High]
Is it safe to assume sibling_intf is bound to usbhid here?

If an attacker plugs in a crafted USB device, the async interface could be
bound to a different driver (like usbfs or usbmouse). Since
usb_get_intfdata() is unconditionally treated as a struct hid_device
pointer, checking sibling_hdev->driver could result in an out-of-bounds
read if the bound driver's data structure is smaller.

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

[Severity: High]
Could a concurrent unbind of the HID driver cause a use-after-free on sd?

While the USB interface lock (&sibling_intf->dev) is held here, this
doesn't serialize against a sysfs unbind of the HID driver. If a user
unbinds the HID driver from the sibling interface via sysfs, the HID core's
hid_device_remove() calls the driver's remove callback (which drops the
refcount and frees sd), and only clears hdev->driver afterwards.

This could allow hid_get_drvdata() to return a pointer to a freed
steelseries_device, leading to a kref_get() on freed memory.

> +		}
> +	}
> +	device_unlock(&sibling_intf->dev);
> +
> +	return sd;
> +}

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