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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] DMA Use-After-Free vulnerability in ele_get_info() when freeing a buffer after a command timeout.
- [High] Spurious failures and duplicate executions due to explicitly overwriting successful hardware transactions with -ERESTARTSYS.
- [High] Out-of-bounds read in se_if_rx_callback() due to trusting the firmware-provided header->size.
- [Medium] Permanent probe failure instead of returning -EPROBE_DEFER when the SRAM pool is not yet available.
--

--- Patch [3]: [PATCH v36 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit ba0b4841b861b1ed1d3d0d02b5e6c1bb39bdf404
Author: Pankaj Gupta <[email protected]>
Subject: 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.

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
@@ -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);

[Severity: High]
If ele_msg_send_rcv() times out in ele_get_info(), does releasing this DMA
buffer back to the system allocator while the enclave is still programmed to
write to it cause kernel memory corruption when the delayed firmware
transaction eventually completes?

> +	else
> +		dma_free_coherent(priv->dev, size, buf, d_addr);
> +}

[ ... ]

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
@@ -0,0 +1,501 @@
[ ... ]
> +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: High]
Does explicitly overwriting a successful hardware response (err > 0) with
-ERESTARTSYS cause the VFS to restart the operation, leading to duplicate
executions of non-idempotent hardware commands and spurious failures in
kernel contexts like PM suspend?

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

[ ... ]
> +void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> +{
[ ... ]
> +	} else if (header->tag == priv->if_defs->rsp_tag) {
[ ... ]
> +		se_clbk_hdl->rx_msg_sz = min(rx_msg_sz, exp_rx_msg_sz);
> +		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);

[Severity: High]
If the mailbox controller driver passes a pointer to a small local buffer
on the IRQ stack for msg, but the untrusted firmware-provided header->size
specifies a large size, does this memcpy() read out-of-bounds past the msg
pointer capacity?

> +		complete(&se_clbk_hdl->done);
> +		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);

[ ... ]

diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
--- /dev/null
+++ b/drivers/firmware/imx/se_ctrl.c
@@ -0,0 +1,507 @@
[ ... ]
> +static int se_if_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (if_node->pool_name) {
> +		priv->mem_pool = of_gen_pool_get(dev->of_node, if_node->pool_name, 0);
> +		if (!priv->mem_pool)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "Unable to get sram pool = %s.",
> +					     if_node->pool_name);

[Severity: Medium]
Should this return -EPROBE_DEFER instead of permanently failing with -ENOMEM
when the SRAM pool is not yet available during boot?

> +	}

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