Re: [PATCH] remoteproc: imx_rproc: revert channel freeing order in imx_rproc_free_mbox

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] remoteproc: imx_rproc: dangling pointers in
  imx_rproc_xtr_mbox_init() [1]
--

commit 47b24cc41389cd2e5e7dc88e20cee4c1d32a5d30
Author: Richard Leitner <richard.leitner@linux.dev>

remoteproc: imx_rproc: revert channel freeing order in imx_rproc_free_mbox

This commit frees the rx mailbox channel before the tx channel in
imx_rproc_free_mbox() to prevent an rx callback from kicking virtqueues
through a freed tx channel during shutdown.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 581bb07f5893f..232da31a56ebe 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -920,15 +920,16 @@ static void imx_rproc_free_mbox(void *data)
>  	struct rproc *rproc = data;
>  	struct imx_rproc *priv = rproc->priv;
>  
> -	if (priv->tx_ch) {
> -		mbox_free_channel(priv->tx_ch);
> -		priv->tx_ch = NULL;
> -	}
> -
> +	/* First release rx channel as its callback may kick vqs via tx_ch. */
>  	if (priv->rx_ch) {
>  		mbox_free_channel(priv->rx_ch);
>  		priv->rx_ch = NULL;
>  	}
> +
> +	if (priv->tx_ch) {
> +		mbox_free_channel(priv->tx_ch);
> +		priv->tx_ch = NULL;
> +	}
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can imx_rproc_free_mbox()
pass an ERR_PTR and a dangling pointer to mbox_free_channel() [1] if
requesting the rx channel fails in imx_rproc_xtr_mbox_init()?

drivers/remoteproc/imx_rproc.c:imx_rproc_xtr_mbox_init() {
    ...
	if (priv->tx_ch && priv->rx_ch)
		return 0;
    ...
	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
	if (IS_ERR(priv->rx_ch)) {
		mbox_free_channel(priv->tx_ch);
		return dev_err_probe(cl->dev, PTR_ERR(priv->rx_ch),
				     "failed to request rx mailbox channel\n");
	}
}

When mbox_request_channel_byname(cl, "rx") fails during rproc start or
attach, priv->tx_ch is freed without being cleared and priv->rx_ch is left
holding an ERR_PTR.  Because both fields remain non-NULL, a subsequent start
retry passes the (priv->tx_ch && priv->rx_ch) check without initializing the
channels and later causes a use-after-free on priv->tx_ch in
imx_rproc_kick(), while unbinding the device causes the devm action
imx_rproc_free_mbox() to call mbox_free_channel() on both invalid pointers.

[1] https://sashiko.dev/#/bug/linux-ac0f248a-bdfd-41da-95f8-44b8f1f7091d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-imx-rproc-free-mbox-v1-1-3e33ff98570d@linux.dev?part=1