Re: [PATCH v6 8/9] HID: steelseries: Add async status interface support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A concurrent unbinding of the sync interface's HID driver via sysfs can result in a Use-After-Free of the `steelseries_device` object during the async interface's probe.
- [High] Blindly casting `usb_get_intfdata()` to `struct hid_device *` causes a Type Confusion and Out-Of-Bounds Read if another driver is bound to the sibling interface.
- [Medium] The `async_interface` field treats `0` as a sentinel value for 'not present', which will break for devices where the async interface is actually at index 0.
--

--- Patch [8]: [PATCH v6 8/9] HID: steelseries: Add async status interface support ---
commit b3e51c178159f4ed7f8fb3b18ebfac675c1ff9b7
Author: Sriman Achanta <srimanachanta@gmail.com>

HID: steelseries: Add async status interface support

This patch adds support for a second HID interface that asynchronously sends
battery and connection updates. It binds both sync and async interfaces,
deferring the async one until the sync interface is ready.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index e534aa44e70a0..9960c0ec512bf 100644
> --- 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)
>  	return 0;
>  }
>  
> +static struct hid_driver steelseries_arctis_driver;
> +
> +static struct steelseries_device *
> +steelseries_get_sibling_sd(struct hid_device *hdev, int interface_num)
> +{
[ ... ]
> +	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]
Does casting usb_get_intfdata() directly to a hid_device risk a type
confusion and out-of-bounds read?

If another driver (like usbfs via userspace claiming the interface) is
bound to the sync USB interface, usb_get_intfdata() would return that
driver's data (e.g., a much smaller struct usb_dev_state).

Reading sibling_hdev->driver would then read far past the bounds of that
smaller struct.

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

[Severity: High]
Is there a race condition here that could lead to a use-after-free of
the steelseries_device object?

The code acquires the parent USB interface lock
(device_trylock(&sibling_intf->dev)), but not the child hid_device lock.

If the HID driver of the sync interface is unbound via sysfs while the
async interface is actively probing, steelseries_arctis_remove() could be
executed, freeing 'sd'.

Is there a window before the driver core clears sibling_hdev->driver and
drvdata where this probing thread could call kref_get() on the freed 'sd'?

> +		}
> +	}
> +	device_unlock(&sibling_intf->dev);
> +
> +	return sd;
> +}
[ ... ]
> @@ -339,43 +388,81 @@ static int steelseries_arctis_probe(struct hid_device *hdev,
>  	if (ret)
>  		return ret;
>  
> -	/* Let hid-generic handle non-sync interfaces */
> -	if (interface_num != info->sync_interface)
> +	/* Let hid-generic handle non-vendor or unknown interfaces */
> +	if (interface_num != info->sync_interface &&
> +	    (!info->async_interface || interface_num != info->async_interface))

[Severity: Medium]
Will this logic prevent devices from using interface 0 as the async
interface?

USB interfaces are zero-indexed, making 0 a valid interface number, but
evaluating !info->async_interface treats 0 as a sentinel "not present"
value.

>  		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709032316.1908460-1-68172138+srimanachanta@users.noreply.github.com?part=8