Re: [PATCH v4 09/10] HID: steelseries: Add async status interface support

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The driver blindly casts the sibling interface's driver data to a `struct hid_device` without verifying the bound driver, leading to type confusion and memory corruption.
- [Critical] The sync interface publishes `sd` to `drvdata` before initialization is complete, allowing the async interface to crash the kernel by scheduling an uninitialized work struct.
- [High] Lockless retrieval and access of the sibling interface's driver data leads to a Use-After-Free if the sync interface is unbound concurrently.
- [High] If the sync interface probe fails after the async interface binds to it, the async interface will access a freed `sd->hdev` pointer.

Pre-existing issues:
- [High] An ERR_PTR temporarily assigned to `sd->battery` during initialization failure can be concurrently dereferenced by the raw event handler.
--

commit 002dd61306f7255151ab901886b17a9e612226ad
Author: Sriman Achanta <srimanachanta@gmail.com>
Subject: HID: steelseries: Add async status interface support

This commit adds support for a second HID interface that sends battery
and connection updates asynchronously. This allows the driver to stop
polling the sync interface by watching the async interface instead.

> diff --git a/drivers/hid/hid-steelseries-arctis.c b/drivers/hid/hid-steelseries-arctis.c
> index 734cf1eb87894..2208d0e4cd2ad 100644
> --- a/drivers/hid/hid-steelseries-arctis.c
> +++ b/drivers/hid/hid-steelseries-arctis.c
[ ... ]
> @@ -316,6 +318,23 @@ static int steelseries_battery_register(struct steelseries_device *sd)
>  	return 0;
>  }
>  
> +static struct hid_device *steelseries_get_sibling_hdev(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;
> +
> +	sibling_intf = usb_ifnum_to_if(usb_dev, interface_num);
> +	if (!sibling_intf)
> +		return NULL;
> +
> +	sibling_hdev = usb_get_intfdata(sibling_intf);

[Severity: High]
Does this code safely retrieve the sibling interface's driver data? Because
no reference is taken on the USB interface and usb_driver_claim_interface()
is not used, could a concurrent sysfs unbind destroy the interface and the
retrieved data while we are accessing it?

[Severity: Critical]
Is it safe to assume usb_get_intfdata() returns a struct hid_device pointer
here? If the sync interface is bound to a different driver (such as
snd-usb-audio), its driver data would be a completely different structure.
Could this lead to type confusion and memory corruption when the pointer is
dereferenced later in the probe function?

> +
> +	return sibling_hdev;
> +}
> +
>  static int steelseries_arctis_probe(struct hid_device *hdev,
>  				    const struct hid_device_id *id)
>  {
> @@ -337,39 +356,76 @@ 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))
>  		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  
> -	sd = kzalloc_obj(*sd, GFP_KERNEL);
> -	if (!sd)
> -		return -ENOMEM;
> +	if (interface_num == info->sync_interface) {
> +		sd = kzalloc_obj(*sd, GFP_KERNEL);
> +		if (!sd)
> +			return -ENOMEM;
>  
> -	kref_init(&sd->refcnt);
> -	sd->hdev = hdev;
> -	sd->info = info;
> -	spin_lock_init(&sd->lock);
> +		kref_init(&sd->refcnt);
> +		sd->hdev = hdev;
> +		sd->info = info;
> +		spin_lock_init(&sd->lock);
>  
> -	hid_set_drvdata(hdev, sd);
> +		hid_set_drvdata(hdev, sd);

[Severity: Critical]
By calling hid_set_drvdata() here, sd is globally published before
INIT_DELAYED_WORK() is called below. If the async interface probes concurrently
and immediately receives an event, could it call schedule_delayed_work() on
an uninitialized work struct?

>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> -	if (ret)
> -		goto err_put;
> +		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +		if (ret)
> +			goto err_put;
>  
> -	ret = hid_hw_open(hdev);
> -	if (ret)
> -		goto err_stop;
> +		ret = hid_hw_open(hdev);
> +		if (ret)
> +			goto err_stop;

[Severity: High]
If the sync interface probe fails after this point (for example, if hid_hw_open()
or steelseries_battery_register() fails) and returns an error, but the async
interface has already bound and incremented sd->refcnt:

Since steelseries_arctis_remove() is never called for the failed sync probe,
sd->removed remains false. If the driver core clears the sync interface's driver
data and the sibling interface is unbound, sd->hdev is freed.

Could the still-active async interface then process raw events, bypass the
sd->removed check, and access the freed sd->hdev pointer via
steelseries_headset_set_wireless_status()?

> +
> +		if (info->capabilities & SS_CAP_BATTERY) {
> +			ret = steelseries_battery_register(sd);
> +			if (ret < 0)
> +				hid_warn(hdev, "Failed to register battery: %d\n", ret);
> +		}
>  
> -	if (info->capabilities & SS_CAP_BATTERY) {
> -		ret = steelseries_battery_register(sd);
> -		if (ret < 0)
> -			hid_warn(hdev, "Failed to register battery: %d\n", ret);
> +		INIT_DELAYED_WORK(&sd->status_work, steelseries_status_timer_work_handler);
> +		schedule_delayed_work(&sd->status_work, msecs_to_jiffies(100));
> +
> +		return 0;
>  	}
[ ... ]
> @@ -442,6 +509,10 @@ static int steelseries_arctis_raw_event(struct hid_device *hdev,
>  			old_connected ? "" : "not ",
>  			sd->headset_connected ? "" : "not ");
>  
> +		if (sd->headset_connected && !old_connected &&
> +		    sd->info->async_interface && is_async_interface)
> +			schedule_delayed_work(&sd->status_work, 0);
> +
>  		if (sd->battery) {
>  			steelseries_headset_set_wireless_status(sd->hdev,
>  							       sd->headset_connected);
>  			power_supply_changed(sd->battery);

[Severity: High]
This is a pre-existing issue, but is there a data race with sd->battery here?

During initialization in steelseries_battery_register():

	sd->battery = power_supply_register(&sd->hdev->dev,
			&sd->battery_desc, &battery_cfg);
	if (IS_ERR(sd->battery)) {
		ret = PTR_ERR(sd->battery);
		sd->battery = NULL;

Since hardware has already been enabled via hid_hw_start(), interrupts can
arrive concurrently. If an event arrives during the window where sd->battery
holds an ERR_PTR, if (sd->battery) will evaluate to true.

Could this cause power_supply_changed(sd->battery) to dereference the error
pointer?

>  		}
>  
> @@ -465,6 +536,8 @@ static int steelseries_arctis_raw_event(struct hid_device *hdev,
>  			power_supply_changed(sd->battery);
>  	}
>  
> +	spin_unlock_irqrestore(&sd->lock, flags);
> +
>  	return 0;
>  }

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