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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

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>
---
 sound/soc/codecs/wcd-mbhc-v2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
index 0000000..1111111 100644
--- a/sound/soc/codecs/wcd-mbhc-v2.c
+++ b/sound/soc/codecs/wcd-mbhc-v2.c
@@ -1624,6 +1624,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);

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

[sashiko-bot]

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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

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 freeing of mbhc was introduced by commit a5475829adcc ("ASoC: codecs:
wcd-mbhc-v2: fix resource leaks on component remove"), which switched mbhc
to kzalloc() and added the kfree(mbhc) above; the deinit path cancels
correct_plug_swch (added with the multi-button support in 0e5c9e7ff899)
and, later, mbhc_plug_detect_work (added in 0c105997eefd), but
mbhc_btn_dwork has never been cancelled there.

Use the existing wcd_mbhc_cancel_pending_work() helper, which cancels
mbhc_btn_dwork (via wcd_cancel_btn_work()) and then correct_plug_swch,
matching the other cancellation sites in this file and ensuring
mbhc_btn_dwork is cancelled before wcd_cancel_hs_detect_plug() briefly
drops mbhc->lock.

Found by code review; not runtime-reproduced (requires WCD codec hardware
and a button-press event racing component removal).

Fixes: a5475829adcc ("ASoC: codecs: wcd-mbhc-v2: fix resource leaks on component remove")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2:
 - Use the existing wcd_mbhc_cancel_pending_work() helper instead of an
   open-coded cancel_delayed_work_sync(), per review - it cancels
   mbhc_btn_dwork before wcd_cancel_hs_detect_plug() drops the lock and
   matches the other cancellation sites.
 - Add Fixes: tag and correct the commit history in the changelog (the v1
   reference to CVE-2023-53842 was inaccurate), per review.

 sound/soc/codecs/wcd-mbhc-v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
--- a/sound/soc/codecs/wcd-mbhc-v2.c
+++ b/sound/soc/codecs/wcd-mbhc-v2.c
@@ -1623,7 +1623,7 @@ void wcd_mbhc_deinit(struct wcd_mbhc *mbhc)
 	free_irq(mbhc->intr_ids->mbhc_sw_intr, mbhc);

 	mutex_lock(&mbhc->lock);
-	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
+	wcd_mbhc_cancel_pending_work(mbhc);
 	cancel_work_sync(&mbhc->mbhc_plug_detect_work);
 	mutex_unlock(&mbhc->lock);

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Fri, Nov 06, 2026 at 02:44:47PM +0900, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@gmail.com>
> 
> 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:

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]