Re: [PATCH 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 4 potential issue(s) to consider:
- [High] hid: uaf in unctrl_remove() due to early work cancellation and list exposure
- [High] hid: uaf of power_supply in unctrl_raw_event() racing with cross-device probe
- [High] hid: uaf of dev in unctrl_probe() error path due to concurrent battery transfer
- [Medium] hid: permanent battery loss due to missing rollback in unctrl_battery_register()
--

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

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

    This patch modifies the unctrl_m1 driver to register a single power
    supply across multiple connections (e.g., wired and dongle) of the same
    mouse. It transfers the power supply ownership when a more capable
    connection appears or disappears.

> 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
[ ... ]
> @@ -181,32 +181,32 @@ static int unctrl_battery_register(struct unctrl_dev *dev)
>  	if (owner) {
>  		struct power_supply *owner_battery;
>  
>  		cancel_delayed_work_sync(&owner->battery_work);
>  		/* so a raw_event cannot touch the supply being unregistered */
>  		scoped_guard(spinlock_irqsave, &owner->lock) {
>  			owner_battery = owner->battery;
>  			owner->battery = NULL;
>  		}
>  		power_supply_unregister(owner_battery);
>  	}
>  
>  	battery = power_supply_register(&dev->hdev->dev, &dev->battery_desc,
>  					&config);
>  	if (IS_ERR(battery)) {
>  		ret = PTR_ERR(battery);
>  		hid_err(dev->hdev,
>  			"%s:power_supply_register failed with error %d\n",
>  			__func__, ret);
>  		return ret;

[Severity: Medium]
If power_supply_register() fails here, does the original owner's battery 
remain unregistered forever?

We just cancelled the owner's work and unregistered its battery above, but 
the error path doesn't restore it. Could this leave the active device 
permanently without battery reporting?

>  	}
>  
>  	power_supply_powers(battery, &dev->hdev->dev);
[ ... ]
> @@ -312,11 +312,11 @@ static int unctrl_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (ret)
>  		goto err_del;
>  
>  	return 0;
>  
>  err_del:
>  	mutex_lock(&unctrl_lock);
>  	list_del(&dev->list);
>  	mutex_unlock(&unctrl_lock);

[Severity: High]
Is there a use-after-free risk in this error path due to a concurrent 
battery transfer?

During the time between the failing unctrl_battery_register() returning and 
re-acquiring the lock here at err_del, could a concurrent unctrl_remove() 
on another connection find this dev in unctrl_connections?

If that happens, the other thread would transfer its battery to this failing
dev and schedule dev->battery_work. This work would remain active after 
kfree(dev) is called at the end of this function.

>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  	hid_set_drvdata(hdev, NULL);
>  	kfree(dev);
>  	return ret;
>  }
[ ... ]
> @@ -336,15 +336,15 @@ static int unctrl_raw_event(struct hid_device *hdev, struct hid_report *report,
[ ... ]
>  	scoped_guard(spinlock_irqsave, &dev->lock) {
>  		/* the receiver reports 0% while the mouse is switched off */
>  		dev->battery_capacity = min_t(u8, data[4], 100);
>  		dev->mode = data[5];
>  		dev->online = dev->battery_capacity != 0;
>  		battery = dev->battery;
>  	}
>  
>  	/* only schedules a work, so this is safe from the completion path */
>  	if (battery)
>  		power_supply_changed(battery);

[Severity: High]
Can power_supply_changed() dereference a freed pointer here?

If a new connection probes concurrently, it can call unctrl_battery_register(), 
which acquires this dev->lock, sets dev->battery = NULL, and immediately 
calls power_supply_unregister(). 

Since power_supply_unregister() synchronously frees the battery, and this 
code drops dev->lock before using the cached battery pointer, could this 
lead to a use-after-free?

>  	return 0;
>  }
>  
>  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);

[Severity: High]
Could dev->battery_work be rescheduled after this cancellation, leading to
a use-after-free?

Since dev is still in the global unctrl_connections list here (it isn't 
removed until the mutex_lock(&unctrl_lock) later in the function), could a 
concurrent unctrl_probe() or unctrl_remove() from another device find it, 
transfer a battery to it, and call schedule_delayed_work()?

If so, the rescheduled work might execute after kfree(dev) is called.

>  		hid_hw_close(hdev);
>  	}

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