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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Freeing the DMA buffer in ele_get_info_cleanup() after a timeout causes a DMA-after-free vulnerability, as the firmware may still be executing and could write to the freed memory.
- [High] Using an interruptible wait in internal kernel transactions causes false probe and suspend failures when signals are pending.
--

--- Patch [3]: [PATCH v33 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit 8655b5144179916d5552e9b237b931d4b3001788
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave.
The secure enclave FW communicates with Linux over single or multiple
dedicated messaging unit(MU) based interface(s). Existing on i.MX SoC(s)
like i.MX8ULP, i.MX93, i.MX95 etc.

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

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-imx-se-if-v33-0-212e32ff0295@nxp.com?part=3