Re: [PATCH] HID: microsoft: cancel ff_worker on remove to avoid use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] HID: microsoft: UAF in ms_remove() as hid_hw_stop() reschedules
  ff_worker
- [Medium] HID: microsoft: WARN_ON in ms_remove() on uninitialized
  ff_worker
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] HID: microsoft: use-after-free in ms_probe() [1]
--

commit 7c761c76a759b981259737d50e309dc8e6d24ed9
Author: Hyunjung Ko <hj351016@gmail.com>

HID: microsoft: cancel ff_worker on remove to avoid use-after-free

This commit cancels ms->ff_worker in ms_remove() before stopping the HID
hardware to prevent the worker from running after device removal.

> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index a7d3493a6141e..462fdaf9c65cb 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -384,6 +384,9 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failed
hid_hw_start() in ms_probe() leave ms->ff_worker queued and cause a
use-after-free [1]?

If hid_hw_start() fails after ms_input_configured() has registered force
feedback, unwinding the input device runs input_ff_flush() ->
ms_play_effect() and schedules ms->ff_worker.  ms_probe() then returns the
error without cancelling ms->ff_worker, so ms_ff_worker() can run after
devres frees ms and ms->output_report_dmabuf.

>  static void ms_remove(struct hid_device *hdev)
>  {
> +	struct ms_data *ms = hid_get_drvdata(hdev);
> +
> +	cancel_work_sync(&ms->ff_worker);

[Severity: Medium]
Does calling cancel_work_sync(&ms->ff_worker) unconditionally here trigger
a warning in __flush_work() when removing a device that never initialized
ms->ff_worker?

ms->ff_worker is only initialized in ms_input_configured() for devices with
force-feedback support:

drivers/hid/hid-microsoft.c:ms_input_configured() {
	if (!(ms->quirks & MS_QUIRK_FF))
		return 0;

	if (!list_is_first(&hidinput->list, &hdev->inputs))
		return 0;

	ms->hdev = hdev;
	INIT_WORK(&ms->ff_worker, ms_ff_worker);
...
}

For the Microsoft devices in ms_devices[] without that quirk,
ms->ff_worker.func remains zero, and cancel_work_sync() ->
__cancel_work_sync() unconditionally calls __flush_work(), which warns on a
uninitialized work item:

kernel/workqueue.c:__flush_work() {
	if (WARN_ON(!work->func))
		return false;
...
}

>  	hid_hw_stop(hdev);
>  }

[Severity: High]
Can hid_hw_stop(hdev) reschedule ms->ff_worker after cancel_work_sync() has
already returned, still leaving a use-after-free once ms_remove() finishes?

cancel_work_sync() leaves the work item enabled, and the input device is
still registered until hid_hw_stop() disconnects it:

ms_remove()
  cancel_work_sync(&ms->ff_worker)
  hid_hw_stop(hdev)
    hid_disconnect(hdev)
      ...
        input_unregister_device(...)
          evdev_cleanup()
            input_flush_device()
              input_ff_flush()
                erase_effect()
                  ml_ff_playback()
                    ms_play_effect()
                      schedule_work(&ms->ff_worker)
  /* devres frees ms and ms->output_report_dmabuf */
  ms_ff_worker()                                    <-- use-after-free

When an open evdev handle has an active effect during disconnect (or if the
ff-memless timer fires before input_unregister_device() stops it),
ms_play_effect() queues ms->ff_worker again after cancel_work_sync(), and
ms_ff_worker() can run after devres frees ms.  Using disable_work_sync() or
cancelling the work after input unregistration would prevent it from being
requeued.

[1] https://sashiko.dev/#/bug/linux-0e0c2d3d-cc31-46f2-bb9c-fdbd250adb84

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