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

[sashiko-bot@kernel.org]

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