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

You are right, and I need to correct my earlier disposition. 
Your finding is valid: the buffer really is freed unconditionally on timeout, and the "DMA free on timeout.
The code is correct by 5/7, and the only actually-wrong item is the 3/7 disposition claim.
No code changes are needed,

> > 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.
> 

Not at this commit. Agreed on the hardware model: fw_busy is a software atomic
and cannot cancel an in-flight enclave DMA. In 3/7 fw_busy is only a bare global
atomic set on timeout; nothing prevents ele_get_info_cleanup() from returning the
buffer to the allocator while the enclave still holds its physical address.
The justification should be removed/corrected from the comment disposition.
No code changes are needed.


> > 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;
> > +	}
>

Yes, at 3/7 it does, and that is the bug. It must not return the buffer to the
allocator while a timed-out transaction has fenced the FW. Minimal in-place fix
for 3/7 (which only has the bare fw_busy atomic at this point):

	static void ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf,
					 dma_addr_t d_addr, size_t size)
	{
		/*
		 * On timeout the FW is fenced via fw_busy but the enclave may
		 * still DMA into this buffer. Do not release it to the allocator;
		 * leak it deliberately to avoid a DMA-after-free. It is a fixed
		 * probe-time buffer and is reclaimed with the device on unbind.
		 */
		if (atomic_read(&priv->fw_busy))
			return;

		if (priv->mem_pool)
			gen_pool_free(priv->mem_pool, (unsigned long)buf, size);
		else
			dma_free_coherent(priv->dev, size, buf, d_addr);
	}

This is fixed in the follow-up commit 5/7.

> [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?
> 

At 3/7, yes. With the guard above the -ETIMEDOUT path sees fw_busy set and
quarantines (leaks) the buffer instead of freeing it, closing the window.

> > 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?
> 

At 3/7 this is also unsafe and must change. Hence, this is changed in 5/7 with the richer per-context
quarantine (se_mark_fw_busy/fw_busy_dev_ctx, se_is_fw_busy_ctx(), mem_pool_buf_list, deferred fw_busy_work),
which is the final form.

Summary of what to do in (3/7):

Nothing, as the comments came due to the richer implementation is part of 5/7.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260729-imx-se-if-v31-0-
> [email protected]?part=3