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

> > diff --git a/drivers/firmware/imx/ele_base_msg.c
> > b/drivers/firmware/imx/ele_base_msg.c
> > new file mode 100644
> > index 0000000000000..724f6e913ce79
> > --- /dev/null
> > +++ b/drivers/firmware/imx/ele_base_msg.c
> > @@ -0,0 +1,282 @@
> 
> [ ... ]
> 
> > +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); }
> > +
> > +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info
> > +*s_info) {
> 
> [ ... ]
> 
> > +	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);
> 
> [Severity: High]
> If ele_msg_send_rcv() times out and returns -ETIMEDOUT, is it safe to
> immediately free the DMA buffer by calling ele_get_info_cleanup()?
> 
> Since the external EdgeLock Enclave firmware is not halted on a timeout,
> couldn't it eventually complete the transaction and write the response
> payload into this freed memory, causing a DMA-after-free memory
> corruption?
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = se_val_rsp_hdr_n_status(priv, rx_msg, ELE_GET_INFO_REQ,
> > +				      ELE_GET_INFO_RSP_MSG_SZ, true);
> > +	if (ret < 0) {
> > +		ele_get_info_cleanup(priv, get_info_data, get_info_addr,
> get_info_len);
> > +		return ret;
> > +	}
> 
[Already taken care in 5/7]
At 3/7 the cleanup frees unconditionally (gen_pool_free/dma_free_coherent) and the timeout leg only does a bare atomic_set(\&priv->fw_busy, 1), which cannot fence an in-flight enclave DMA. The symbols that fix it (se_is_fw_busy_ctx/se_mark_fw_busy, priv_dev_ctx) do not exist at 3/7. It is resolved by PATCH v33 5/7 (fb107010a07b, "firmware: imx: adds miscdev"), which adds per-context fw_busy ownership and rewrites ele_get_info_cleanup() to quarantine (return without freeing) the buffer while the owning context is still busy.


> [ ... ]
> 
> > diff --git a/drivers/firmware/imx/ele_common.c
> > b/drivers/firmware/imx/ele_common.c
> > new file mode 100644
> > index 0000000000000..bc08ea2dcd87d
> > --- /dev/null
> > +++ b/drivers/firmware/imx/ele_common.c
> > @@ -0,0 +1,492 @@
> 
> [ ... ]
> 
> > +int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle
> > +*se_clbk_hdl) {
> > +	bool is_rsp_wait_with_timeout = false;
> > +	bool wait_uninterruptible = false;
> > +	unsigned long remaining_jiffies;
> > +	unsigned long deadline_jiffies;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	remaining_jiffies =
> msecs_to_jiffies(SE_RCV_MSG_DEFAULT_TIMEOUT_MS);
> > +	if (se_clbk_hdl == &priv->waiting_rsp_clbk_hdl) {
> > +		is_rsp_wait_with_timeout = true;
> > +		deadline_jiffies = jiffies + remaining_jiffies;
> > +	}
> > +
> > +	do {
> > +		if (is_rsp_wait_with_timeout) {
> > +			unsigned long now = jiffies;
> > +
> > +			if (time_after_eq(now, deadline_jiffies)) {
> > +				/* Deadline hit: fence hung FW, like the
> ret==0 path. */
> > +				spin_lock_irqsave(&se_clbk_hdl-
> >clbk_rx_lock, flags);
> > +				se_clbk_hdl->rx_msg = NULL;
> > +				if (!completion_done(&se_clbk_hdl->done))
> > +					atomic_set(&priv->fw_busy, 1);
> > +				spin_unlock_irqrestore(&se_clbk_hdl-
> >clbk_rx_lock, flags);
> > +				ret = -ETIMEDOUT;
> > +				break;
> > +			}
> > +			remaining_jiffies = deadline_jiffies - now;
> > +		}
> > +
> > +		if (wait_uninterruptible)
> > +			ret = wait_for_completion_timeout(&se_clbk_hdl-
> >done,
> > +							  remaining_jiffies);
> > +		else
> > +			ret =
> wait_for_completion_interruptible_timeout(&se_clbk_hdl->done,
> > +
> 	remaining_jiffies);
> 
> [Severity: High]
> Since ele_msg_rcv() uses an interruptible wait by default for all transactions,
> what happens to internal kernel transactions if a signal is pending?
> 
> If a fake signal is sent by the PM freezer during suspend, or a user sends
> SIGINT during modprobe, won't this abort the wait with -ERESTARTSYS?
> 
> > +		if (ret == -ERESTARTSYS) {
> > +			/*
> > +			 * Record that a signal was observed, then continue
> waiting non-
> > +			 * interruptibly until the response arrives or the
> timeout
> > +			 * expires. The caller can surface the interruption to
> userspace
> > +			 * after the protocol transaction is brought back to a
> > +			 * synchronized state.
> > +			 */
> > +			if (is_rsp_wait_with_timeout &&
> > +			    READ_ONCE(se_clbk_hdl->rx_msg)) {
> > +				WRITE_ONCE(se_clbk_hdl->signal_rcvd, true);
> > +				wait_uninterruptible = true;
> > +				continue;
> > +			}
> > +			break;
> > +		}
> 
> [ ... ]
> 
> > +int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
> > +		     void *rx_msg, int exp_rx_msg_sz) {
> 
> [ ... ]
> 
> > +	err = ele_msg_rcv(priv, &priv->waiting_rsp_clbk_hdl);
> > +
> > +	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
> > +		/*
> > +		 * Signal was deferred until the FW/kernel protocol
> resynchronized.
> > +		 * On success report -ERESTARTSYS for the interrupted wait;
> the
> > +		 * command is not re-sent. Keep real errors like -ETIMEDOUT.
> > +		 */
> > +		if (err > 0)
> > +			err = -ERESTARTSYS;
> > +		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> > +		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
> > +	}
> 
> Since ele_msg_send_rcv() explicitly converts the successful return back to -
> ERESTARTSYS when a signal was received, could this cause spurious device
> initialization or power management failures for callers that are not tied to a
> restartable syscall?

[Already taken care in 5/7]
The `wait_uninterruptible = true` for `priv->priv_dev_ctx` is introduced in 5/7 as well (it depends on `priv_dev_ctx`, added in 4/7 ).
At 3/7 only the `signal_rcvd` deferred-signal mechanism exists, not the internal-transaction fix.

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