Re: [PATCH 2/2] HID: steelseries: Add Rival 3 Wireless battery reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] HID: steelseries: missing suspend and runtime PM handling for battery_work
- [High] HID: steelseries: UAF on probe failure after steelseries_resume() arms work
- [High] HID: steelseries: race between steelseries_remove() and steelseries_resume()
- [Medium] HID: steelseries: deprecated system_wq mismatch in steelseries_resume()
--

commit bd1d30fd1ed7cb8e4a6c8b3c38fe5d24efafb14d
Author: Martin Oberleiter <martin.oberleiter@netixx.it>

HID: steelseries: Add Rival 3 Wireless battery reporting

This commit adds power_supply battery reporting for the SteelSeries Rival 3
Wireless mouse in 2.4 GHz receiver mode by polling vendor interface 3 via a
delayed work item and parsing the asynchronous input report replies.

> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index 7292da0313e11..9fa70d21f49de 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
[ ... ]
> +static void steelseries_rival3_battery_work(struct work_struct *work)
> +{
> +	struct steelseries_device *sd = container_of(work,
> +			struct steelseries_device, battery_work.work);
> +	static const u8 request[] = { 0x00, 0xaa, 0x01 }; /* report ID 0 */
> +	bool was_online, was_charging, online, changed;
> +	unsigned long flags, timeout;
> +	u8 old_capacity;
> +	u8 *buf;
> +	int ret;
> +
> +	spin_lock_irqsave(&sd->battery_lock, flags);
> +	was_online = sd->battery_online;
> +	was_charging = sd->battery_charging;
> +	old_capacity = sd->battery_capacity;
> +	spin_unlock_irqrestore(&sd->battery_lock, flags);
> +
> +	buf = kmemdup(request, sizeof(request), GFP_KERNEL);
> +	if (!buf) {
> +		ret = -ENOMEM;
> +		goto update;
> +	}
> +
> +	spin_lock_irqsave(&sd->battery_lock, flags);
> +	reinit_completion(&sd->battery_reply);
> +	sd->battery_awaiting_reply = true;
> +	spin_unlock_irqrestore(&sd->battery_lock, flags);
> +
> +	ret = hid_hw_raw_request(sd->hdev, 0x00, buf, sizeof(request),
> +				 HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> +	kfree(buf);
> +	if (ret >= 0) {
> +		timeout = msecs_to_jiffies(STEELSERIES_RIVAL3_WIRELESS_REPLY_TIMEOUT_MS);
> +		ret = wait_for_completion_timeout(&sd->battery_reply, timeout) ? 0 : -ETIMEDOUT;
> +	}
> +
> +update:
> +	spin_lock_irqsave(&sd->battery_lock, flags);
> +	/* A reply may still have arrived right after the timeout. */
> +	if (ret && sd->battery_awaiting_reply) {
> +		/* Keep the last level; the mouse is off or asleep. */
> +		sd->battery_awaiting_reply = false;
> +		sd->battery_online = false;
> +	} else {
> +		ret = 0;
> +	}
> +	online = sd->battery_online;
> +	changed = online != was_online ||
> +		  sd->battery_charging != was_charging ||
> +		  sd->battery_capacity != old_capacity;
> +	spin_unlock_irqrestore(&sd->battery_lock, flags);
> +
> +	if (ret)
> +		hid_dbg(sd->hdev, "No battery reply: %d\n", ret);
> +	if (changed)
> +		power_supply_changed(sd->battery);

[Severity: High]
What happens when steelseries_rival3_battery_work() in
drivers/hid/hid-steelseries.c runs during system suspend, suspend-to-idle
(s2idle), or USB runtime autosuspend?

Because steelseries_driver registers .resume and .reset_resume callbacks
without a matching .suspend callback and queues sd->battery_work on a
non-freezable workqueue, steelseries_rival3_battery_work() can fire after
usbhid's hid_suspend() has stopped I/O and usb_suspend() has cleared
udev->can_submit (or while the system is in s2idle).

When hid_hw_raw_request() fails with -EHOSTUNREACH (or
wait_for_completion_timeout() times out with -ETIMEDOUT),
steelseries_rival3_battery_work() sets sd->battery_online = false. If the
mouse was previously online (was_online == true), changed becomes true and
power_supply_changed(sd->battery) is called:

    steelseries_rival3_battery_work()
      power_supply_changed(sd->battery)
        pm_stay_awake(&psy->dev)

Because battery_cfg.no_wakeup_source is left false in
steelseries_rival3_battery_register(), __power_supply_register() initializes
sd->battery->dev as a wakeup source, so pm_stay_awake() will abort system
suspend or wake the system from s2idle while also dropping the poll interval
to 1 minute.

In addition, usbhid enables USB runtime autosuspend and usbhid_open() drops
its usb_autopm reference before returning. Unlike
steelseries_msi_rgb_set_blocking() in the same file,
steelseries_rival3_battery_work() does not wrap the transfer and reply wait
with hid_hw_power(sd->hdev, PM_HINT_FULLON) and hid_hw_power(sd->hdev,
PM_HINT_NORMAL). Consequently, polls while the receiver is runtime-suspended
fail without waking the interface, and every runtime autoresume invokes
hid_resume() -> steelseries_resume(), resetting the 10-minute poll timer to
1 second.

Should steelseries_driver provide a .suspend callback that cancels
sd->battery_work, and should steelseries_rival3_battery_work() hold
hid_hw_power(sd->hdev, PM_HINT_FULLON) across the request and completion
wait?

> +
> +	schedule_delayed_work(&sd->battery_work, msecs_to_jiffies(online ?
> +			STEELSERIES_RIVAL3_WIRELESS_POLL_ONLINE_MS :
> +			STEELSERIES_RIVAL3_WIRELESS_POLL_OFFLINE_MS));
> +}
> +
> +static int steelseries_rival3_battery_register(struct steelseries_device *sd)
> +{
[ ... ]
> +	power_supply_powers(battery, &hdev->dev);
> +	sd->battery = battery;
> +	sd->battery_iface = true;
> +
> +	return 0;
> +}
[ ... ]
> @@ -593,17 +811,38 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>  		}
>  	}
>  
> +	if (sd->quirks & STEELSERIES_RIVAL3_WIRELESS &&
> +	    !steelseries_rival3_is_battery_iface(hdev))
> +		sd->quirks &= ~STEELSERIES_RIVAL3_WIRELESS;
> +
>  	ret = hid_parse(hdev);
>  	if (ret)
>  		return ret;
>  
> +	if (sd->quirks & STEELSERIES_RIVAL3_WIRELESS) {
> +		ret = steelseries_rival3_battery_register(sd);
> +		if (ret) {
> +			hid_warn(hdev,
> +				 "Failed to register battery: %d, continuing without battery reporting\n",
> +				 ret);
> +			sd->quirks &= ~STEELSERIES_RIVAL3_WIRELESS;
> +		}
> +	}
> +
>  	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  	if (ret)
>  		return ret;
>  
> -	ret = hid_hw_open(hdev);
> -	if (ret)
> -		goto err_stop;
> +	/* Only the Rival 3 Wireless battery interface needs to stay open. */
> +	if (hdev->product != USB_DEVICE_ID_STEELSERIES_RIVAL_3_WIRELESS ||
> +	    sd->battery_iface) {
> +		ret = hid_hw_open(hdev);
> +		if (ret)
> +			goto err_stop;
> +	}

[Severity: High]
Can a failed probe in steelseries_probe() in drivers/hid/hid-steelseries.c
leave sd->battery_work armed after devres frees sd?

In __hid_device_probe(), hdev->driver is set to &steelseries_driver before
steelseries_probe() runs. In steelseries_probe(),
steelseries_rival3_battery_register() sets sd->battery_iface = true before
hid_hw_start() and hid_hw_open() are called.

If the parent usb_interface is runtime-suspended when hid_hw_open() is
called, usbhid_open() calls usb_autopm_get_interface(usbhid->intf), which
synchronously runs:

    usbhid_open()
      usb_autopm_get_interface()
        hid_resume()
          hid_driver_resume()
            steelseries_resume()
              mod_delayed_work(system_wq, &sd->battery_work, ...)

Because sd->battery_iface is already true, steelseries_resume() arms
sd->battery_work. If hid_hw_open() then fails (for example, when
hid_start_in() fails and usbhid_open() returns -EBUSY), steelseries_probe()
jumps to err_stop and returns without cancelling sd->battery_work, after
which __hid_device_probe() frees sd via devres_release_group() while the
delayed work timer is still pending.

Would it be safer to register the battery and set sd->battery_iface = true
only after hid_hw_start() and hid_hw_open() have succeeded (or cancel
sd->battery_work in the error path)?

[ ... ]
> @@ -632,10 +873,32 @@ static void steelseries_remove(struct hid_device *hdev)
>  		return;
>  	}
>  
> -	hid_hw_close(hdev);
> +	sd = hid_get_drvdata(hdev);
> +	if (sd->battery_iface)
> +		cancel_delayed_work_sync(&sd->battery_work);
> +
> +	if (hdev->product != USB_DEVICE_ID_STEELSERIES_RIVAL_3_WIRELESS ||
> +	    sd->battery_iface)
> +		hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  }

[Severity: High]
Could steelseries_resume() race with steelseries_remove() in
drivers/hid/hid-steelseries.c and re-queue sd->battery_work during driver
unbind?

In hid_device_remove(), hdrv->remove(hdev) is called first, followed by
devres_release_group(&hdev->dev, hdev->devres_group_id), and hdev->driver is
only set to NULL afterwards. Meanwhile, usbhid's hid_resume() and
hid_reset_resume() invoke steelseries_resume() on the parent usb_interface
without holding device_lock(&hdev->dev) or hdev->driver_input_lock.

Because steelseries_remove() calls cancel_delayed_work_sync() rather than
disable_delayed_work_sync() and leaves sd->battery_iface true while
hid_hw_close(), hid_hw_stop(), and devres_release_group() execute, a
concurrent runtime resume or reset_resume on the USB interface can see
sd && sd->battery_iface and call mod_delayed_work() after
cancel_delayed_work_sync() has already returned, re-arming sd->battery_work
on memory that devres_release_group() is freeing.

Should steelseries_remove() use disable_delayed_work_sync(&sd->battery_work)
to prevent sd->battery_work from being re-queued once teardown begins?

>  
> +static int steelseries_resume(struct hid_device *hdev)
> +{
> +	struct steelseries_device *sd;
> +
> +	if (hdev->product != USB_DEVICE_ID_STEELSERIES_RIVAL_3_WIRELESS)
> +		return 0;
> +
> +	/* Polls during suspend fail, so do not wait for the next one. */
> +	sd = hid_get_drvdata(hdev);
> +	if (sd && sd->battery_iface)
> +		mod_delayed_work(system_wq, &sd->battery_work,
> +				 msecs_to_jiffies(1000));

[Severity: Medium]
In steelseries_resume() in drivers/hid/hid-steelseries.c, mod_delayed_work()
queues sd->battery_work onto system_wq, whereas steelseries_probe() and
steelseries_rival3_battery_work() use schedule_delayed_work(), which queues
onto system_percpu_wq.

In kernel/workqueue.c, system_wq and system_percpu_wq are separate
workqueue_struct instances, and system_wq is marked with __WQ_DEPRECATED,
causing __queue_work() to emit a runtime deprecation warning when the timer
expires:

    workqueue: work func steelseries_rival3_battery_work enqueued on deprecated workqueue. Use system_{percpu|dfl}_wq instead.

Mixing two different workqueue_struct instances for the same delayed_work
also bypasses __queue_work()'s single-workqueue non-reentrancy check
(worker->current_pwq->wq == wq), allowing steelseries_rival3_battery_work()
to execute concurrently on two CPUs and race on
reinit_completion(&sd->battery_reply) and sd->battery_awaiting_reply.

Should steelseries_resume() pass system_percpu_wq to mod_delayed_work() so
all scheduling of sd->battery_work uses the same non-deprecated workqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-rival3-battery-v1-0-6b1feeaf1f52@netixx.it?part=2