Re: [PATCH] ASoC: codecs: wcd-mbhc-v2: cancel button long-press work on deinit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] ASoC: codecs: wcd-mbhc-v2: missing Fixes tag and inaccurate
  history in commit
- [Low] ASoC: codecs: wcd-mbhc-v2: open-coded work cancel in
  wcd_mbhc_deinit()
--

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

> ASoC: codecs: wcd-mbhc-v2: cancel button long-press work on deinit
>
> wcd_mbhc_deinit() drains two of the three works embedded in struct
> wcd_mbhc before freeing it - correct_plug_swch (via
> wcd_cancel_hs_detect_plug()) and mbhc_plug_detect_work - but not the
> button long-press delayed work mbhc_btn_dwork:
>
>         mutex_lock(&mbhc->lock);
>         wcd_cancel_hs_detect_plug(mbhc, &mbhc->correct_plug_swch);
>         cancel_work_sync(&mbhc->mbhc_plug_detect_work);
>         mutex_unlock(&mbhc->lock);
>
>         kfree(mbhc);
>
> mbhc_btn_dwork is armed from the button-press interrupt handler
> wcd_mbhc_btn_press_handler() with a 400 ms delay:
>
>         schedule_delayed_work(&mbhc->mbhc_btn_dwork, msecs_to_jiffies(400));
>
> and its handler wcd_btn_long_press_fn() dereferences @mbhc
> (container_of(), mbhc->current_plug, mbhc->jack).
>
> wcd_mbhc_deinit() frees the MBHC interrupts first, which stops new work
> from being queued, but free_irq() does not cancel a delayed work that was
> already scheduled. If a button was pressed within ~400 ms of component
> removal, mbhc_btn_dwork is still pending when kfree(mbhc) runs, so
> wcd_btn_long_press_fn() later executes against freed memory
> (use-after-free).
>
> The asymmetry dates from the teardown rework in commit (CVE-2023-53842),
> which added the correct_plug_swch/mbhc_plug_detect_work cancellation but
> left mbhc_btn_dwork undrained.
>
> Cancel mbhc_btn_dwork alongside the other works. Its handler does not take
> mbhc->lock, so cancel_delayed_work_sync() is safe to call under the lock.
>
> Found by code review; not runtime-reproduced (requires WCD codec hardware
> and a button-press event racing component removal).
>
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
Since this fixes a use-after-free on teardown, should it include a Fixes:
tag such as:

  Fixes: a5475829adcc ("ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove")

Also, the reference to CVE-2023-53842 in the commit description omits the
commit SHA and subject above, and misstates its changes:
wcd_cancel_hs_detect_plug() was present since 0e5c9e7ff899 ("ASoC: codecs:
wcd: add multi button Headset detection support"), a5475829adcc ("ASoC:
codecs: wcd-mbhc-v2: fix resource leaks on component remove") switched mbhc
to kzalloc() and added kfree(mbhc), and 0c105997eefd ("ASoC: codec:
wcd-mbhc-v2: add support when connected behind an USB-C audio mux") added
mbhc_plug_detect_work.

> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
> index fd40c1686d66a..3649ab1eba421 100644
> --- a/sound/soc/codecs/wcd-mbhc-v2.c
> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
> @@ -1625,6 +1625,7 @@ void wcd_mbhc_deinit(struct wcd_mbhc *mbhc)
>  	mutex_lock(&mbhc->lock);
>  	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
>  	cancel_work_sync(&mbhc->mbhc_plug_detect_work);
> +	cancel_delayed_work_sync(&mbhc->mbhc_btn_dwork);
>  	mutex_unlock(&mbhc->lock);
>  
>  	kfree(mbhc);

[Severity: Low]
This isn't a bug, but could wcd_mbhc_deinit() call
wcd_mbhc_cancel_pending_work() instead of open-coding
cancel_delayed_work_sync(&mbhc->mbhc_btn_dwork) after
wcd_cancel_hs_detect_plug()?

sound/soc/codecs/wcd-mbhc-v2.c:wcd_mbhc_cancel_pending_work() {
	/* cancel pending button press */
	wcd_cancel_btn_work(mbhc);
	/* cancel correct work function */
	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
}

Reusing wcd_mbhc_cancel_pending_work() matches the other cancellation sites
in this file and cancels mbhc_btn_dwork before wcd_cancel_hs_detect_plug()
temporarily drops mbhc->lock.

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