Re: [PATCH v30 4/7] firmware: imx: device context dedicated to priv

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The newly introduced `priv_dev_ctx` and its `devname` string are dynamically allocated but never freed on probe failure or device unbind, leading to a permanent memory leak.

Pre-existing issues:
- [Critical] DMA buffer is freed unconditionally on timeout while the hardware may still be processing the command, leading to use-after-free and DMA memory corruption.
--

--- Patch [4]: [PATCH v30 4/7] firmware: imx: device context dedicated to priv ---
commit 12d4f3d474262e1d23dd156bdd3a71ae735f6977
Author: Pankaj Gupta <[email protected]>

firmware: imx: device context dedicated to priv

Add priv_dev_ctx to prepare enabling misc-device context based send-receive
path, to communicate with FW.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index a2ede9c01691d..524ebc0f697e6 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -66,8 +66,9 @@ int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
>  	tx_msg->data[0] = upper_32_bits(get_info_addr);
>  	tx_msg->data[1] = lower_32_bits(get_info_addr);
>  	tx_msg->data[2] = sizeof(*s_info);
> -	ret = ele_msg_send_rcv(priv, tx_msg, ELE_GET_INFO_REQ_MSG_SZ, rx_msg,
> -			       ELE_GET_INFO_RSP_MSG_SZ);
> +
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_GET_INFO_REQ_MSG_SZ,
> +			       rx_msg, ELE_GET_INFO_RSP_MSG_SZ);
>  	if (ret < 0) {
>  		ele_get_info_cleanup(priv, get_info_data, get_info_addr, ELE_GET_INFO_BUFF_SZ);
>  		return ret;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does this error path free the DMA buffer
while the hardware might still be active?

If ele_msg_send_rcv() times out, it returns -ETIMEDOUT and trips the fw_busy
circuit breaker. However, this error path unconditionally calls
ele_get_info_cleanup(), which releases the DMA buffer back to the system:

static void ele_get_info_cleanup(...)
{
	if (priv->mem_pool)
		gen_pool_free(priv->mem_pool, (unsigned long)buf, size);
	else
		dma_free_coherent(priv->dev, size, buf, d_addr);
}

If the firmware is merely slow and eventually processes the command, won't it
execute a delayed DMA write to the freed physical address, potentially
corrupting whatever kernel memory is subsequently mapped there?

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index e9e2f48f4f71f..b053dd00286c8 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
> @@ -411,6 +441,12 @@ static int se_if_probe(struct platform_device *pdev)
>  		load_fw->is_fw_tobe_loaded = true;
>  	}
>  
> +	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed[0x%x] to create device contexts.",
> +				     ret);
> +

[Severity: Medium]
Will this leak the newly allocated priv->priv_dev_ctx and its devname string
on probe failure or device unbind?

The init_misc_device_context() function dynamically allocates these resources
using non-devm kzalloc_obj() and kasprintf(). However, if probe fails at a
later step or the device is unbound, the devres-managed cleanup function
se_if_probe_cleanup() only frees priv and misses the context:

static void se_if_probe_cleanup(void *context)
{
	...
	if (priv->tx_chan)
		mbox_free_channel(priv->tx_chan);

	dev_set_drvdata(dev, NULL);

	kfree(priv);
}

Should the cleanup function be updated to also free priv->priv_dev_ctx and
its devname to avoid a permanent memory leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4