Re: [PATCH v31 3/7] firmware: imx: add driver for NXP EdgeLock Enclave

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] DMA Use-After-Free / Memory Corruption on command timeout
--

--- Patch [3]: [PATCH v31 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit ae6974582706d865c1232af0bb0e80860d057d5e
Author: Pankaj Gupta <[email protected]>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave.

NXP hardware IP(s) for secure-enclaves like Edgelock Enclave(ELE), are
embedded in the SoC to support the features like HSM, SHE & V2X, using
message based communication interface.

> Two findings need no code change:
> - DMA free on timeout: -ETIMEDOUT means FW is fenced by fw_busy, so the
>   free is safe.

[Severity: High]
Is this description accurate? The fw_busy flag is a software-side atomic variable
that only prevents the kernel from sending subsequent commands. It does nothing
to cancel the in-flight hardware DMA operation. Once the enclave has the physical
address, the kernel cannot safely free the memory until it guarantees the enclave
will no longer write to it.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.c

[ ... ]

> +static void 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);
> +}

[Severity: High]
Does ele_get_info_cleanup() in drivers/firmware/imx/ele_base_msg.c unconditionally
free the DMA buffer back to the allocator even if the firmware is still executing
the command and may write to it?

[ ... ]

> +	ret = ele_msg_send_rcv(priv, 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, won't ele_get_info()
invoke the unsafe free operation while the firmware might still be processing
the request?

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> +	} else if (header->tag == priv->if_defs->rsp_tag) {
> +		bool exception_for_sz_mismatch = check_hdr_exception_for_sz(priv, header);
> +		u32 exp_rx_msg_sz = 0;
> +
> +		/*
> +		 * rx_msg and rx_msg_sz are owned by the sender under clbk_rx_lock.
> +		 * Read both under the lock: drop a late response instead of copying
> +		 * into freed memory, and avoid a stale size. A late response also
> +		 * closes the firmware-busy circuit breaker.
> +		 */
> +		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
> +		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
> +		if (!se_clbk_hdl->rx_msg) {
> +			/* Close circuit breaker on spinlock race */
> +			atomic_set(&priv->fw_busy, 0);
> +			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
> +			dev_info(dev, "ELE responded (late), recovery FW available.");
> +			return;
> +		}

[Severity: High]
When a late response arrives after a timeout, does clearing the software circuit
breaker in se_if_rx_callback() leave the system completely oblivious to the fact
that the hardware just wrote to freed memory?

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