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

On Mon, Aug 17, 2026 at 08:48:17AM +0000, [email protected] wrote:
> 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?

Please check this also

Frank

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