Re: [PATCH v2 4/5] HID: UNCTRL: keep a single battery when a mouse has multiple connections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: unctrl: use-after-free via concurrent battery handover to a stopped device
- [High] hid: unctrl: UAF in usbhid_output_report due to delayed worker cancel order
--

commit e4a86ecd29d9dc9de2cb5ac805fe6b509cef727d
Author: Bishwa Bhusan Saha <contact@bishwasaha.com>

HID: UNCTRL: keep a single battery when a mouse has multiple connections

The UNCTRL RAGE mouse can be connected over its 2.4 GHz dongle and its
wired connection at the same time. This patch avoids reporting two
separate batteries by registering the power supply only on the active
connection, handing it over dynamically when that connection goes away.

> diff --git a/drivers/hid/hid-unctrl-m1.c b/drivers/hid/hid-unctrl-m1.c
> --- a/drivers/hid/hid-unctrl-m1.c
> +++ b/drivers/hid/hid-unctrl-m1.c

[ ... ]

> @@ -302,20 +364,41 @@ static int unctrl_raw_event(struct hid_device *hdev, struct hid_report *report,
>  static void unctrl_remove(struct hid_device *hdev)
>  {
>  	struct unctrl_dev *dev = hid_get_drvdata(hdev);
> +	struct unctrl_dev *other;
>  
>  	/* Stop polling before the transport goes down. */
> -	if (dev) {
> -		cancel_delayed_work_sync(&dev->battery_work);
> +	if (dev)
>  		hid_hw_close(hdev);
> -	}
>  
>  	hid_hw_stop(hdev);
>  
>  	if (!dev)
>  		return;
>  
> -	if (dev->battery)
> -		power_supply_unregister(dev->battery);
> +	/*
> +	 * Pass the battery to a capable connection without one, so that it
> +	 * keeps being reported when the owner disappears.
> +	 */
> +	mutex_lock(&unctrl_lock);
> +	list_del(&dev->list);
> +	/* out of the list, nothing can reach this device anymore */
> +	cancel_delayed_work_sync(&dev->battery_work);

[Severity: High]
Can this cause a use-after-free in the transport driver? By moving the
cancellation of the delayed worker battery_work to after hid_hw_stop(),
the self-rescheduling worker might run concurrently with or after the
transport is torn down.

For example, hid_hw_stop() invokes the transport driver's stop routine
like usbhid_stop(), which frees resources such as usbhid->urbout. If the
un-cancelled worker executes unctrl_battery_timer_tick() and calls
hid_hw_output_report(), the transport driver might evaluate its urbout
pointer without locking, creating a time-of-check to time-of-use race
that dereferences the freed pointer.

> +	if (dev->battery) {
> +		struct power_supply *battery;
> +
> +		scoped_guard(spinlock_irqsave, &dev->lock) {
> +			battery = dev->battery;
> +			dev->battery = NULL;
> +		}
> +		power_supply_unregister(battery);
> +		list_for_each_entry(other, &unctrl_connections, list) {
> +			if (!other->vendor || other->battery)
> +				continue;
> +			unctrl_battery_register(other);

[Severity: High]
Can this trigger a use-after-free if two devices are concurrently removed?

hid_hw_stop() is called before the device is removed from the
unctrl_connections list under unctrl_lock.

If Thread A is executing unctrl_remove() for Device A, it will call
hid_hw_stop(A->hdev).

If Thread B is concurrently executing unctrl_remove() for Device B, it might
acquire unctrl_lock before Thread A. Thread B will iterate the list, find
Device A, and hand the battery to it by calling unctrl_battery_register().

This triggers unctrl_fetch_battery(), which issues a hardware query via
hid_hw_output_report(). Since Device A's transport has already been stopped
by Thread A, accessing the hardware can trigger a use-after-free.

> +			break;
> +		}
> +	}
> +	mutex_unlock(&unctrl_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-b4-unctrl-m1-battery-v2-0-bd6752021753@bishwasaha.com?part=4