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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The DMA buffer passed to the enclave firmware is freed unconditionally on timeout, creating a window where delayed firmware operations will corrupt memory.
- [Medium] The core messaging function `ele_msg_send_rcv` incorrectly returns `-ERESTARTSYS` when a firmware command completes successfully, but a signal was received during the wait. This causes non-idempotent operations to be incorrectly reported as interrupted and potentially re-executed.
--

--- Patch [3]: [PATCH v34 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit 33c1a4b0c7dcc5a0ad9f6bb5fbdd591b9371ef2d
Author: Pankaj Gupta <[email protected]>
Subject: 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). Exists on i.MX SoC(s) like i.MX8ULP, i.MX93,
i.MX95 etc.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=3

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 0000000000000..724f6e913ce79
> --- /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);
> +}
> +
> +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: Critical]
If ele_msg_send_rcv() times out and returns -ETIMEDOUT, does this unconditionally
free the DMA buffer while the firmware might still be active?

If the firmware is merely slow and responds after the timeout, it could write
up to 160 bytes into this physical memory after it has been returned to the
page allocator or gen_pool, potentially causing memory corruption. 

Should we avoid freeing this buffer if the firmware transaction has not been safely
aborted?

> +		return ret;
> +	}

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

[ ... ]

> +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)
> +{

[ ... ]

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

[Severity: Medium]
If a signal is received but the firmware successfully completes the command
(err > 0), replacing the result with -ERESTARTSYS discards the hardware
response data. 

Could this cause issues if the operation is exposed to userspace later? The
VFS would restart the syscall and re-execute the firmware operation, which
might not be idempotent. 

Is it intentional to discard a successful response here?

> +		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> +		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
> +	}
> +
> +	ele_msg_send_rcv_cleanup(priv);
> +
> +	return err;
> +}

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