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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Converting a successful firmware transaction to `-ERESTARTSYS` (and subsequently `-EINTR`) causes loss of response data and duplicate execution of non-idempotent commands.
- [High] Missing return value check for `dma_set_mask_and_coherent()` allows silent DMA address truncation.
- [Medium] Buffer over-read in `se_if_rx_callback` due to trusting unauthenticated message size.
--

commit a66c01dd1d5f945431fe3bf45a2b25282c5a4077
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave.

> 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

[ ... ]

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

[Severity: High]
Does this conversion of a successful hardware transaction to -ERESTARTSYS 
cause the loss of response data?

If userspace retries the operation as expected for an interrupted system
call, wouldn't it re-execute a command the hardware has already processed,
breaking exactly-once semantics and state synchronization?

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

[ ... ]

> +void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> +{
> +	struct se_clbk_handle *se_clbk_hdl;
> +	struct device *dev = mbox_cl->dev;
> +	struct se_msg_hdr *header;
> +	bool sz_mismatch = false;
> +	struct se_if_priv *priv;
> +	unsigned long flags;
> +	u32 rx_msg_sz;
> +
> +	priv = dev_get_drvdata(dev);
> +	if (!priv)
> +		return;
> +
> +	/* The function can be called with NULL msg */
> +	if (IS_ERR_OR_NULL(msg)) {
> +		dev_err(dev, "Message is invalid\n");
> +		return;
> +	}
> +
> +	header = msg;
> +	rx_msg_sz = header->size << 2;

[ ... ]

> +	} 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: Medium]
Can this untrusted header size calculation cause an out-of-bounds read?

The rx_msg_sz is calculated directly from the unauthenticated message header.
If the underlying mailbox driver provides a pointer to a smaller fixed-size
payload, couldn't the memcpy read past the end of the msg buffer?

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

[ ... ]

> +static int se_if_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	if (if_node->reserved_dma_ranges) {
> +		ret = of_reserved_mem_device_init(dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to init reserved memory region.\n");
> +	}
> +
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));

[Severity: High]
Could omitting the return value check for dma_set_mask_and_coherent() allow
silent DMA address truncation?

If the DMA mask is not successfully set, the system might provide 64-bit DMA
addresses. Because the hardware appears to only support 32-bit DMA addresses,
wouldn't truncation to 32 bits instruct the secure enclave to access the wrong
physical memory addresses and cause memory corruption?

> +
> +	/*
> +	 * Initialize load_fw_lock before registering the misc device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-imx-se-if-v43-0-a3deadbda4ef@nxp.com?part=3