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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] In `ele_get_info()`, a physical DMA buffer (`get_info_data`) is allocated and mapped for the firmware to write its response. If the firmware takes too long to respond, `ele_msg_send_rcv()` times out and returns `-ETIMEDOUT`, setting the firmware state to busy. However, on this error path, `ele_get_info()` calls `ele_get_info_cleanup()`, which unconditionally frees the DMA buffer (via `dma_free_coherent()` or `gen_pool_free()`). Because the firmware may still be processing the command, it could eventually finish and write its response to the now-freed physical address, leading to a DMA Use-After-Free that corrupts memory. The buffer must be quarantined (leaked) if the firmware is unresponsive.
--

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

firmware: imx: device context dedicated to priv

This patch adds a private device context to se_if_priv to prepare for
misc-device context-based send-receive paths. It updates several firmware
message passing functions to take the new priv_dev_ctx instead of the
top-level priv structure.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -71,8 +71,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, get_info_len);
>  		return ret;

[Severity: High]
If ele_msg_send_rcv() times out and returns -ETIMEDOUT, the firmware state is
marked as busy, but this error path calls ele_get_info_cleanup() which
unconditionally frees the DMA buffer get_info_data:

ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf, dma_addr_t d_addr,
				 size_t size)
{
	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 just slow and later completes the command, will it write
its response into this now-freed physical address, causing a DMA
use-after-free and memory corruption?

Should this buffer be quarantined instead of freed when the firmware is
unresponsive?

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