[PATCH] HID: magicmouse: disable the multitouch work on removal

[PATCH] HID: magicmouse: disable the multitouch work on removal

[Myeonghun Pak]

During USB runtime reset-resume, usbhid can call magicmouse_reset_resume()
without serialization against HID driver unbind. The USB interface
remains bound to usbhid, so this callback can queue msc->work after
magicmouse_remove() cancels it. The delayed work can then run after
devres frees msc.

Use disable_delayed_work_sync() to drain the work and reject later
queueing. Probe initializes fresh work on rebind.

Found by a static analysis tool under development.

Fixes: 4253fe22b137 ("HID: magicmouse: re-enable multitouch after reset-resume")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index a01e84a18ded..525ffc64a2f0 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -1032,7 +1032,7 @@ static void magicmouse_remove(struct hid_device *hdev)
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
 
 	if (msc) {
-		cancel_delayed_work_sync(&msc->work);
+		disable_delayed_work_sync(&msc->work);
 		if (is_usb_magicmouse2(hdev->vendor, hdev->product) ||
 		    is_usb_magictrackpad2(hdev->vendor, hdev->product))
 			timer_delete_sync(&msc->battery_timer);

base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
-- 
2.43.0

Re: [PATCH] HID: magicmouse: disable the multitouch work on removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] HID: magicmouse: UAF on msc in magicmouse_reset_resume()
  during unbind
--

commit 6b44c69f3cb297be7e3753fa8262ad83f2724078
Author: Myeonghun Pak <mhun512@gmail.com>

HID: magicmouse: disable the multitouch work on removal

This commit replaces cancel_delayed_work_sync() with
disable_delayed_work_sync() in magicmouse_remove() to prevent
magicmouse_reset_resume() from queueing msc->work during unbind.

> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index a01e84a18dede..525ffc64a2f02 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -1032,7 +1032,7 @@ static void magicmouse_remove(struct hid_device *hdev)
>  	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
>  
>  	if (msc) {
> -		cancel_delayed_work_sync(&msc->work);
> +		disable_delayed_work_sync(&msc->work);
>  		if (is_usb_magicmouse2(hdev->vendor, hdev->product) ||
>  		    is_usb_magictrackpad2(hdev->vendor, hdev->product))
>  			timer_delete_sync(&msc->battery_timer);

[Severity: Medium]
Can magicmouse_reset_resume() still cause a use-after-free on msc after
magicmouse_remove() returns?

disable_delayed_work_sync() records the disabled state in
msc->work.work.data, which lives inside the devm-allocated msc.  In
hid_device_remove(), devres_release_group() frees msc right after
hdrv->remove() returns, before hdev->driver or driver_data is cleared:

drivers/hid/hid-core.c:hid_device_remove() {
	...
		if (hdrv->remove)
			hdrv->remove(hdev);
		else /* default remove */
			hid_hw_stop(hdev);

		/* Release all devres resources allocated by the driver */
		devres_release_group(&hdev->dev, hdev->devres_group_id);

		hid_close_report(hdev);
		hdev->driver = NULL;
	...
}

Because hid_driver_reset_resume() does not serialize against
hid_device_remove(), a concurrent reset-resume can still touch msc->work
after msc is freed:

CPU0                                  CPU1
hid_device_remove()
  magicmouse_remove()
    disable_delayed_work_sync(&msc->work)
                                      hid_driver_reset_resume()
                                        magicmouse_reset_resume()
                                          msc = hid_get_drvdata(hdev)
  devres_release_group() -> frees msc
                                          schedule_delayed_work(&msc->work, ...)
                                            <-- use-after-free on msc->work

queue_delayed_work_on() performs atomic bit operations on
msc->work.work.data to test and clear the pending bit when disabled,
modifying freed or reallocated slab memory (or queueing the work if
reallocation cleared the disable count).

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