[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