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

[sashiko-bot@kernel.org]

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