[PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[Hyunjung Ko]

The mailbox rx callback schedules priv->rproc_work (imx_dsp_rproc_vq_work),
whose handler takes rproc->lock and calls rproc_vq_interrupt(). The work is
never cancelled: imx_dsp_rproc_free_mbox() frees the mailbox channels (which
stops new rx callbacks) but does not drain a work the callback has already
queued.

free_irq()-like teardown of the mailbox does not wait for an already-queued
work item, so a rproc_work still pending when the rproc is torn down runs
after the rproc has been freed and dereferences freed memory (rproc->lock) --
a use-after-free.

Cancel the work in imx_dsp_rproc_free_mbox(), after the channels are freed so
it cannot be re-queued, before the rproc it dereferences is released.

Found by static analysis; not runtime-reproduced (needs an i.MX DSP with a
mailbox message racing teardown).

Fixes: ec0e5549f358 ("remoteproc: imx_dsp_rproc: Add remoteproc driver for DSP on i.MX")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 96d619d..89c72d9 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -642,6 +642,13 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
 	mbox_free_channel(priv->tx_ch);
 	mbox_free_channel(priv->rx_ch);
 	mbox_free_channel(priv->rxdb_ch);
+
+	/*
+	 * The mailbox rx callback schedules rproc_work; the channels are now
+	 * freed, so no new work can be queued. Drain any pending work before
+	 * the rproc (which the work dereferences) can be freed.
+	 */
+	cancel_work_sync(&priv->rproc_work);
 }

 /**
--
2.43.0

Re: [PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] remoteproc: imx_dsp_rproc: AB-BA deadlock in
  imx_dsp_rproc_free_mbox()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] remoteproc: imx_dsp_rproc: use-after-free and resource
  leaks on unbind [1]
--

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

remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

This commit calls cancel_work_sync(&priv->rproc_work) in
imx_dsp_rproc_free_mbox() after freeing the mailbox channels to drain any
pending virtqueue work item.

> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 96d619daf3f5b..89c72d916b1e4 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -642,6 +642,13 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
>  	mbox_free_channel(priv->tx_ch);
>  	mbox_free_channel(priv->rx_ch);
>  	mbox_free_channel(priv->rxdb_ch);
> +
> +	/*
> +	 * The mailbox rx callback schedules rproc_work; the channels are now
> +	 * freed, so no new work can be queued. Drain any pending work before
> +	 * the rproc (which the work dereferences) can be freed.
> +	 */
> +	cancel_work_sync(&priv->rproc_work);
>  }

[Severity: High]
Can calling cancel_work_sync(&priv->rproc_work) in imx_dsp_rproc_free_mbox()
cause an ABBA deadlock with rproc->lock?

CPU0                              CPU1
----                              ----
mutex_lock(&rproc->lock)
                                  imx_dsp_rproc_vq_work()
cancel_work_sync(&priv->rproc_work)
                                    mutex_lock(&rproc->lock)

 *** DEADLOCK ***

CPU0: __rproc_shutdown() -> rproc_unprepare_device() ->
      imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
      imx_dsp_runtime_suspend() -> imx_dsp_rproc_free_mbox()
CPU1: process_one_work() -> imx_dsp_rproc_vq_work()

Because priv->rx_ch remains active until mbox_free_channel(priv->rx_ch) runs
inside imx_dsp_rproc_free_mbox(), an inbound mailbox interrupt before or
during shutdown can queue priv->rproc_work while __rproc_shutdown() holds
rproc->lock.  imx_dsp_rproc_vq_work() then blocks on rproc->lock while
imx_dsp_rproc_free_mbox() waits in cancel_work_sync(&priv->rproc_work).

[Severity: Critical]
This problem wasn't introduced by this patch, but can unbinding an active
DSP remoteproc device skip imx_dsp_rproc_free_mbox() entirely [1] and
leave both the mailbox channels and priv->rproc_work active when the rproc is
freed?

drivers/remoteproc/imx_dsp_rproc.c:imx_dsp_rproc_probe() {
	...
	ret = devm_rproc_add(dev, rproc);
	if (ret)
		return dev_err_probe(dev, ret, "rproc_add failed\n");

	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);

	return devm_pm_runtime_enable(dev);
}

Because devm_rproc_add() is registered before devm_pm_runtime_enable(),
devres teardown disables runtime PM before devm_rproc_remove() calls
rproc_del() and __rproc_shutdown().  When imx_dsp_rproc_unprepare() then
calls pm_runtime_put_sync(), rpm_suspend() fails with -EACCES without
calling imx_dsp_runtime_suspend() or imx_dsp_rproc_free_mbox(), leaving
priv->clks enabled and the mailbox channels and priv->rproc_work able to
dereference the freed rproc and imx_dsp_rproc structures.

[1] https://sashiko.dev/#/bug/linux-a6e902ee-9958-4d44-96cc-d854c6256124

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

Re: [PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[khj]

Thanks -- both are correct, and my patch is wrong.

[High] The AB-BA deadlock is real. imx_dsp_rproc_free_mbox() runs from the
pm_runtime suspend path (__rproc_shutdown() -> rproc_unprepare_device() ->
imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
imx_dsp_runtime_suspend()),
and __rproc_shutdown() holds rproc->lock across that. imx_dsp_rproc_vq_work()
also takes rproc->lock, so cancel_work_sync() there waits for the work while the
work waits for the lock -- a deadlock. The cancel must not run under
rproc->lock,
so it cannot live in free_mbox().

[Critical] You're right that the unbind path can skip free_mbox() entirely:
devm_rproc_add() is registered before devm_pm_runtime_enable(), so on unbind
devres disables runtime PM before devm_rproc_remove() runs rproc_del() /
__rproc_shutdown(). pm_runtime_put_sync() then returns -EACCES without calling
imx_dsp_runtime_suspend(), so free_mbox() never runs -- the mailbox channels and
rproc_work stay live while the rproc / imx_dsp_rproc are freed (UAF + the clk
leak you note). So my one-liner does not even fix the unbind UAF it targeted.

So this needs a teardown rework rather than a cancel in free_mbox(): e.g.
reorder devm_pm_runtime_enable() before devm_rproc_add() so the suspend path
(and mailbox free) actually runs on unbind, and drain rproc_work from a context
that does not hold rproc->lock. Since that touches the driver's probe/teardown
ordering, I'd rather not guess at the shape -- Mathieu, Frank, how would you
prefer this handled? I'm happy to send a v2 once the preferred
direction is clear.

Thanks,
Hyunjung

2026년 11월 10일 (화) 오후 8:08, <sashiko-bot@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] remoteproc: imx_dsp_rproc: AB-BA deadlock in
>   imx_dsp_rproc_free_mbox()
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [Critical] remoteproc: imx_dsp_rproc: use-after-free and resource
>   leaks on unbind [1]
> --
>
> commit 17e3fec9a8c6e436afd532f653405b5a78e316af
> Author: Hyunjung Ko <hj351016@gmail.com>
>
> remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox
>
> This commit calls cancel_work_sync(&priv->rproc_work) in
> imx_dsp_rproc_free_mbox() after freeing the mailbox channels to drain any
> pending virtqueue work item.
>
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > index 96d619daf3f5b..89c72d916b1e4 100644
> > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > @@ -642,6 +642,13 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
> >       mbox_free_channel(priv->tx_ch);
> >       mbox_free_channel(priv->rx_ch);
> >       mbox_free_channel(priv->rxdb_ch);
> > +
> > +     /*
> > +      * The mailbox rx callback schedules rproc_work; the channels are now
> > +      * freed, so no new work can be queued. Drain any pending work before
> > +      * the rproc (which the work dereferences) can be freed.
> > +      */
> > +     cancel_work_sync(&priv->rproc_work);
> >  }
>
> [Severity: High]
> Can calling cancel_work_sync(&priv->rproc_work) in imx_dsp_rproc_free_mbox()
> cause an ABBA deadlock with rproc->lock?
>
> CPU0                              CPU1
> ----                              ----
> mutex_lock(&rproc->lock)
>                                   imx_dsp_rproc_vq_work()
> cancel_work_sync(&priv->rproc_work)
>                                     mutex_lock(&rproc->lock)
>
>  *** DEADLOCK ***
>
> CPU0: __rproc_shutdown() -> rproc_unprepare_device() ->
>       imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
>       imx_dsp_runtime_suspend() -> imx_dsp_rproc_free_mbox()
> CPU1: process_one_work() -> imx_dsp_rproc_vq_work()
>
> Because priv->rx_ch remains active until mbox_free_channel(priv->rx_ch) runs
> inside imx_dsp_rproc_free_mbox(), an inbound mailbox interrupt before or
> during shutdown can queue priv->rproc_work while __rproc_shutdown() holds
> rproc->lock.  imx_dsp_rproc_vq_work() then blocks on rproc->lock while
> imx_dsp_rproc_free_mbox() waits in cancel_work_sync(&priv->rproc_work).
>
> [Severity: Critical]
> This problem wasn't introduced by this patch, but can unbinding an active
> DSP remoteproc device skip imx_dsp_rproc_free_mbox() entirely [1] and
> leave both the mailbox channels and priv->rproc_work active when the rproc is
> freed?
>
> drivers/remoteproc/imx_dsp_rproc.c:imx_dsp_rproc_probe() {
>         ...
>         ret = devm_rproc_add(dev, rproc);
>         if (ret)
>                 return dev_err_probe(dev, ret, "rproc_add failed\n");
>
>         rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
>
>         return devm_pm_runtime_enable(dev);
> }
>
> Because devm_rproc_add() is registered before devm_pm_runtime_enable(),
> devres teardown disables runtime PM before devm_rproc_remove() calls
> rproc_del() and __rproc_shutdown().  When imx_dsp_rproc_unprepare() then
> calls pm_runtime_put_sync(), rpm_suspend() fails with -EACCES without
> calling imx_dsp_runtime_suspend() or imx_dsp_rproc_free_mbox(), leaving
> priv->clks enabled and the mailbox channels and priv->rproc_work able to
> dereference the freed rproc and imx_dsp_rproc structures.
>
> [1] https://sashiko.dev/#/bug/linux-a6e902ee-9958-4d44-96cc-d854c6256124
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261110105436.2487822-1-hj351016@gmail.com?part=1

Re: [PATCH] remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox

[Peng Fan]

Add back remoteproc list.

Peng

On Tue, Nov 10, 2026 at 08:40:44PM +0900, khj wrote:
>Thanks -- both are correct, and my patch is wrong.
>
>[High] The AB-BA deadlock is real. imx_dsp_rproc_free_mbox() runs from the
>pm_runtime suspend path (__rproc_shutdown() -> rproc_unprepare_device() ->
>imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
>imx_dsp_runtime_suspend()),
>and __rproc_shutdown() holds rproc->lock across that. imx_dsp_rproc_vq_work()
>also takes rproc->lock, so cancel_work_sync() there waits for the work while the
>work waits for the lock -- a deadlock. The cancel must not run under
>rproc->lock,
>so it cannot live in free_mbox().
>
>[Critical] You're right that the unbind path can skip free_mbox() entirely:
>devm_rproc_add() is registered before devm_pm_runtime_enable(), so on unbind
>devres disables runtime PM before devm_rproc_remove() runs rproc_del() /
>__rproc_shutdown(). pm_runtime_put_sync() then returns -EACCES without calling
>imx_dsp_runtime_suspend(), so free_mbox() never runs -- the mailbox channels and
>rproc_work stay live while the rproc / imx_dsp_rproc are freed (UAF + the clk
>leak you note). So my one-liner does not even fix the unbind UAF it targeted.
>
>So this needs a teardown rework rather than a cancel in free_mbox(): e.g.
>reorder devm_pm_runtime_enable() before devm_rproc_add() so the suspend path
>(and mailbox free) actually runs on unbind, and drain rproc_work from a context
>that does not hold rproc->lock. Since that touches the driver's probe/teardown
>ordering, I'd rather not guess at the shape -- Mathieu, Frank, how would you
>prefer this handled? I'm happy to send a v2 once the preferred
>direction is clear.
>
>Thanks,
>Hyunjung
>
>2026년 11월 10일 (화) 오후 8:08, <sashiko-bot@kernel.org>님이 작성:
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential
>> issue to consider:
>> - [High] remoteproc: imx_dsp_rproc: AB-BA deadlock in
>>   imx_dsp_rproc_free_mbox()
>> It also noticed 1 related pre-existing issue this patch does not
>> introduce:
>> - [Critical] remoteproc: imx_dsp_rproc: use-after-free and resource
>>   leaks on unbind [1]
>> --
>>
>> commit 17e3fec9a8c6e436afd532f653405b5a78e316af
>> Author: Hyunjung Ko <hj351016@gmail.com>
>>
>> remoteproc: imx_dsp_rproc: drain the vq work before freeing the mailbox
>>
>> This commit calls cancel_work_sync(&priv->rproc_work) in
>> imx_dsp_rproc_free_mbox() after freeing the mailbox channels to drain any
>> pending virtqueue work item.
>>
>> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
>> > index 96d619daf3f5b..89c72d916b1e4 100644
>> > --- a/drivers/remoteproc/imx_dsp_rproc.c
>> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
>> > @@ -642,6 +642,13 @@ static void imx_dsp_rproc_free_mbox(struct imx_dsp_rproc *priv)
>> >       mbox_free_channel(priv->tx_ch);
>> >       mbox_free_channel(priv->rx_ch);
>> >       mbox_free_channel(priv->rxdb_ch);
>> > +
>> > +     /*
>> > +      * The mailbox rx callback schedules rproc_work; the channels are now
>> > +      * freed, so no new work can be queued. Drain any pending work before
>> > +      * the rproc (which the work dereferences) can be freed.
>> > +      */
>> > +     cancel_work_sync(&priv->rproc_work);
>> >  }
>>
>> [Severity: High]
>> Can calling cancel_work_sync(&priv->rproc_work) in imx_dsp_rproc_free_mbox()
>> cause an ABBA deadlock with rproc->lock?
>>
>> CPU0                              CPU1
>> ----                              ----
>> mutex_lock(&rproc->lock)
>>                                   imx_dsp_rproc_vq_work()
>> cancel_work_sync(&priv->rproc_work)
>>                                     mutex_lock(&rproc->lock)
>>
>>  *** DEADLOCK ***
>>
>> CPU0: __rproc_shutdown() -> rproc_unprepare_device() ->
>>       imx_dsp_rproc_unprepare() -> pm_runtime_put_sync() ->
>>       imx_dsp_runtime_suspend() -> imx_dsp_rproc_free_mbox()
>> CPU1: process_one_work() -> imx_dsp_rproc_vq_work()
>>
>> Because priv->rx_ch remains active until mbox_free_channel(priv->rx_ch) runs
>> inside imx_dsp_rproc_free_mbox(), an inbound mailbox interrupt before or
>> during shutdown can queue priv->rproc_work while __rproc_shutdown() holds
>> rproc->lock.  imx_dsp_rproc_vq_work() then blocks on rproc->lock while
>> imx_dsp_rproc_free_mbox() waits in cancel_work_sync(&priv->rproc_work).
>>
>> [Severity: Critical]
>> This problem wasn't introduced by this patch, but can unbinding an active
>> DSP remoteproc device skip imx_dsp_rproc_free_mbox() entirely [1] and
>> leave both the mailbox channels and priv->rproc_work active when the rproc is
>> freed?
>>
>> drivers/remoteproc/imx_dsp_rproc.c:imx_dsp_rproc_probe() {
>>         ...
>>         ret = devm_rproc_add(dev, rproc);
>>         if (ret)
>>                 return dev_err_probe(dev, ret, "rproc_add failed\n");
>>
>>         rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
>>
>>         return devm_pm_runtime_enable(dev);
>> }
>>
>> Because devm_rproc_add() is registered before devm_pm_runtime_enable(),
>> devres teardown disables runtime PM before devm_rproc_remove() calls
>> rproc_del() and __rproc_shutdown().  When imx_dsp_rproc_unprepare() then
>> calls pm_runtime_put_sync(), rpm_suspend() fails with -EACCES without
>> calling imx_dsp_runtime_suspend() or imx_dsp_rproc_free_mbox(), leaving
>> priv->clks enabled and the mailbox channels and priv->rproc_work able to
>> dereference the freed rproc and imx_dsp_rproc structures.
>>
>> [1] https://sashiko.dev/#/bug/linux-a6e902ee-9958-4d44-96cc-d854c6256124
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20261110105436.2487822-1-hj351016@gmail.com?part=1
>
>