[PATCH v2] HID: microsoft: fix use-after-free of ff_worker on teardown

[PATCH v2] HID: microsoft: fix use-after-free of ff_worker on teardown

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

ms_play_effect() schedules ms->ff_worker to send the force-feedback
report; ms (and ms->output_report_dmabuf) is devm-allocated, so it is
freed by devres after ms_remove() returns. The work was never cancelled
on teardown, so ms_ff_worker() could run against freed memory.

Draining the work is not quite enough on its own:

 - hid_hw_stop() in ms_remove() unregisters the input device, whose
   teardown (input_ff_flush() -> ms_play_effect()) can re-queue ff_worker,
   so the work must be cancelled *after* hid_hw_stop(), not before;
 - ff_worker was only INIT_WORK()'d in ms_input_configured() for devices
   with force-feedback, so cancelling it unconditionally would hit the
   WARN_ON(!work->func) in __flush_work() for the other MS devices; and
 - if hid_hw_start() fails in ms_probe() after force-feedback was
   registered, its unwind can likewise schedule ff_worker, which was then
   left pending when ms_probe() returned.

Initialise ff_worker unconditionally in ms_probe() (so it is always valid
to cancel), cancel it on the hid_hw_start() error path, and cancel it in
ms_remove() after hid_hw_stop().

Not runtime-tested (requires MS force-feedback hardware).

Fixes: 73c5b254c365 ("HID: microsoft: Add rumble support for Xbox One S controller")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2: Address the review of v1 (which just added cancel_work_sync before
    hid_hw_stop):
    - Cancel ff_worker AFTER hid_hw_stop(), since the input-device teardown
      in hid_hw_stop() can re-queue it via input_ff_flush() ->
      ms_play_effect().
    - Initialise ff_worker unconditionally in ms_probe() so cancelling it
      does not hit WARN_ON(!work->func) for non-FF MS devices (it was only
      INIT_WORK()'d for FF devices in ms_input_configured()).
    - Also cancel it on the ms_probe() hid_hw_start() error path, where a
      failed start can likewise leave the work queued.

    v1: https://lore.kernel.org/linux-input/20261108133821.535139-1-hj351016@gmail.com/
 drivers/hid/hid-microsoft.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index a7d3493..1042a52 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -334,9 +334,6 @@ static int ms_input_configured(struct hid_device *hdev, struct hid_input *hidinp
 	if (!list_is_first(&hidinput->list, &hdev->inputs))
 		return 0;
 
-	ms->hdev = hdev;
-	INIT_WORK(&ms->ff_worker, ms_ff_worker);
-
 	ms->output_report_dmabuf = devm_kzalloc(&hdev->dev,
 						sizeof(struct xb1s_ff_report),
 						GFP_KERNEL);
@@ -361,6 +358,14 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	hid_set_drvdata(hdev, ms);
 
+	/*
+	 * Initialise ff_worker unconditionally so it is always valid to
+	 * cancel on the probe-error and remove paths, even for devices
+	 * without force-feedback (where it is never scheduled).
+	 */
+	ms->hdev = hdev;
+	INIT_WORK(&ms->ff_worker, ms_ff_worker);
+
 	if (quirks & MS_NOGET)
 		hdev->quirks |= HID_QUIRK_NOGET;
 
@@ -377,6 +382,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
 				HID_CONNECT_HIDINPUT_FORCE : 0));
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
+		cancel_work_sync(&ms->ff_worker);
 		return ret;
 	}
 
@@ -384,7 +390,16 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
 }
 static void ms_remove(struct hid_device *hdev)
 {
+	struct ms_data *ms = hid_get_drvdata(hdev);
+
+	/*
+	 * hid_hw_stop() unregisters the input device, whose teardown can
+	 * re-queue ff_worker via input_ff_flush() -> ms_play_effect(). Drain
+	 * the work after hid_hw_stop() so it cannot run against the
+	 * devm-freed ms once ms_remove() returns.
+	 */
 	hid_hw_stop(hdev);
+	cancel_work_sync(&ms->ff_worker);
 }
 
 static const struct hid_device_id ms_devices[] = {
-- 
2.43.0

Re: [PATCH v2] HID: microsoft: fix use-after-free of ff_worker on teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: microsoft: cancelling ff_worker after hid_hw_stop()
  races with stop()
- [Low] HID: microsoft: Fixes trailer cites 73c5b254c365 instead of
  1cfc77a64b71
--

commit 4c6868d5ba6d04af120ee3895542c5a22d96e529
Author: Hyunjung Ko <hj351016@gmail.com>

> HID: microsoft: fix use-after-free of ff_worker on teardown
>
> ms_play_effect() schedules ms->ff_worker to send the force-feedback
> report; ms (and ms->output_report_dmabuf) is devm-allocated, so it is
> freed by devres after ms_remove() returns. The work was never cancelled
> on teardown, so ms_ff_worker() could run against freed memory.
>
> Draining the work is not quite enough on its own:
>
>  - hid_hw_stop() in ms_remove() unregisters the input device, whose
>    teardown (input_ff_flush() -> ms_play_effect()) can re-queue ff_worker,
>    so the work must be cancelled *after* hid_hw_stop(), not before;
>  - ff_worker was only INIT_WORK()'d in ms_input_configured() for devices
>    with force-feedback, so cancelling it unconditionally would hit the
>    WARN_ON(!work->func) in __flush_work() for the other MS devices; and
>  - if hid_hw_start() fails in ms_probe() after force-feedback was
>    registered, its unwind can likewise schedule ff_worker, which was then
>    left pending when ms_probe() returned.
>
> Initialise ff_worker unconditionally in ms_probe() (so it is always valid
> to cancel), cancel it on the hid_hw_start() error path, and cancel it in
> ms_remove() after hid_hw_stop().
>
> Not runtime-tested (requires MS force-feedback hardware).
>
> Fixes: 73c5b254c365 ("HID: microsoft: Add rumble support for Xbox One S controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
Should the Fixes tag reference 1cfc77a64b71 ("HID: microsoft: move FF
initialization to .input_configured()") instead?

73c5b254c365 ("HID: microsoft: Add rumble support for Xbox One S
controller") defined ms_remove_ff() to call cancel_work_sync() after
hid_hw_stop() in ms_remove(), and only initialized force-feedback after
hid_hw_start() succeeded.  1cfc77a64b71 ("HID: microsoft: move FF
initialization to .input_configured()") deleted ms_remove_ff() and moved
INIT_WORK() and input_ff_create_memless() into ms_input_configured().

> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index a7d3493a6141e..1042a5247c4e5 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c

[ ... ]

> @@ -377,6 +382,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  				HID_CONNECT_HIDINPUT_FORCE : 0));
>  	if (ret) {
>  		hid_err(hdev, "hw start failed\n");
> +		cancel_work_sync(&ms->ff_worker);
>  		return ret;
>  	}
>  
> @@ -384,7 +390,16 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  }
>  static void ms_remove(struct hid_device *hdev)
>  {
> +	struct ms_data *ms = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * hid_hw_stop() unregisters the input device, whose teardown can
> +	 * re-queue ff_worker via input_ff_flush() -> ms_play_effect(). Drain
> +	 * the work after hid_hw_stop() so it cannot run against the
> +	 * devm-freed ms once ms_remove() returns.
> +	 */
>  	hid_hw_stop(hdev);
> +	cancel_work_sync(&ms->ff_worker);
>  }

[Severity: Medium]
Does waiting until after hid_hw_stop() to cancel ms->ff_worker allow
ms_ff_worker() to call hid_hw_output_report() concurrently with or after
hdev->ll_driver->stop()?

CPU0                                  kworker
ms_remove()
  hid_hw_stop(hdev)
    hid_disconnect(hdev)
      ...
        input_ff_flush()
          ...
            ms_play_effect()
              schedule_work(&ms->ff_worker)
    hdev->ll_driver->stop(hdev)
                                      ms_ff_worker()
                                        hid_hw_output_report(hdev, ...)
                                          hdev->ll_driver->output_report(...)
                                          <-- runs during or after stop()
  cancel_work_sync(&ms->ff_worker)

hid_hw_stop() invokes hdev->ll_driver->stop(hdev) immediately after
hid_disconnect(hdev) before returning to ms_remove() (and similarly when
hid_hw_start() unwinds on failure in ms_probe()).  For Bluetooth and UHID
transports, this queues an output report after hidp_stop() has purged the
transmit queue or after uhid_hid_stop() has queued UHID_STOP.

Calling disable_work_sync(&ms->ff_worker) before hid_hw_stop() would drain
any in-flight work while the transport is still active and prevent
input_ff_flush() from re-queueing it during input device teardown.

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