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

[PATCH v3] 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 and freed
after ms_remove() returns, and the work was never cancelled on teardown,
so ms_ff_worker() could run against freed memory.

hid_hw_stop() in ms_remove() unregisters the input device, whose teardown
(input_ff_flush() -> ms_play_effect()) re-queues ff_worker, and a work
left running after ll_driver->stop() would also issue an output report on
an already-stopped transport. Disable the work with disable_work_sync()
*before* hid_hw_stop(), so it is drained while the transport is still live
and cannot be re-queued during teardown.

ff_worker was only INIT_WORK()'d in ms_input_configured() for
force-feedback devices, so initialise it unconditionally in ms_probe()
(making disable_work_sync() always valid) and also disable it on the
hid_hw_start() error path, whose unwind can likewise schedule it.

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

Fixes: 1cfc77a64b71 ("HID: microsoft: move FF initialization to .input_configured()")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index a7d3493a6141..5e4218beec92 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -335,7 +335,6 @@ static int ms_input_configured(struct hid_device *hdev, struct hid_input *hidinp
 		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),
@@ -361,6 +360,13 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	hid_set_drvdata(hdev, ms);
 
+	/* ff_worker can be scheduled from input_ff_flush() during a failed
+	 * hid_hw_start() unwind and on removal, so initialise it
+	 * unconditionally before hid_hw_start() rather than only for
+	 * force-feedback devices in ms_input_configured().
+	 */
+	INIT_WORK(&ms->ff_worker, ms_ff_worker);
+
 	if (quirks & MS_NOGET)
 		hdev->quirks |= HID_QUIRK_NOGET;
 
@@ -377,6 +383,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");
+		disable_work_sync(&ms->ff_worker);
 		return ret;
 	}
 
@@ -384,6 +391,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() tears down the input device, whose input_ff_flush()
+	 * -> ms_play_effect() path re-queues ff_worker, and a work still
+	 * running after ll_driver->stop() would issue an output report on a
+	 * stopped transport.  Disable the work first so it is drained while
+	 * the transport is live and cannot be re-queued during teardown.
+	 */
+	disable_work_sync(&ms->ff_worker);
 	hid_hw_stop(hdev);
 }
 
-- 
2.43.0