Re: [PATCH v5 8/9] 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] Unsynchronized access to the sibling interface's HID device and driver data in `steelseries_get_sibling_sd()` leads to Use-After-Free vulnerabilities during concurrent unbinding.
- [High] Unsafe retrieval of `intfdata` from a sibling USB interface without verifying the bound USB driver leads to type confusion.
--

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

HID: steelseries: Add async status interface support

This patch adds support for an async status interface on some Steelseries
headsets. The driver binds both sync and async interfaces, using the async
interface to push status events and delay its probe until the sync interface
is fully initialized.

> 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

[ ... ]

> +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;
> +
> +	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)

[Severity: High]
Can a malicious USB device trigger an out-of-bounds read here?

If the device advertises the sync interface as a non-HID class (like Mass
Storage) but the async interface as HID, the USB core might bind a different
driver to the sync interface. In that case, usb_get_intfdata() would return
a driver-specific structure (like struct us_data) rather than a
struct hid_device.

Would it be safer to verify the bound USB driver on the sibling interface
before casting the intfdata and dereferencing sibling_hdev->driver?

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

[Severity: High]
Is there a race condition with concurrent unbinding here?

If the sync interface is concurrently unbound (e.g., via sysfs or physical
disconnection) while the async interface is probing, usb_get_intfdata()
and hid_get_drvdata() are called without holding device_lock() for the
sibling interface's device.

Could this allow the hid_device to be freed before checking
sibling_hdev->driver, or allow the steelseries_device (sd) to be freed
before kref_get() is called?

> +
> +	return sd;
> +}

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