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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] DMA Use-After-Free Vulnerability due to premature buffer cleanup on timeout.
- [High] Improper error handling overriding successful firmware commands with `-ERESTARTSYS`, causing duplicate execution.
- [Medium] `se_resume` unconditionally returns 0 even if `se_restore_imem_state` fails, violating the API contract.
--

--- Patch [3]: [PATCH v37 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit 12b954054edf722eadc852286283752fad83199e
Author: Pankaj Gupta <[email protected]>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave to support
features like HSM, SHE & V2X.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ 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)
> +{
> +	dma_addr_t get_info_addr = 0;
> +	void *get_info_data = NULL;
> +	u32 get_info_len;
> +	int ret = 0;

[ ... ]

> +	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, is it safe to
unconditionally free the DMA buffer here by calling ele_get_info_cleanup()?

Since the secure enclave firmware executes asynchronously and doesn't know
the host timed out, could it eventually finish the delayed operation and
perform a DMA write to this freed physical address? Does this memory need
to be quarantined or leaked when the firmware is marked busy to prevent
potential 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);

[ ... ]

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> --- a/drivers/firmware/imx/ele_common.c
> +++ 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)
> +{
> +	unsigned long flags;
> +	int err;

[ ... ]

> +	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]
The comment mentions that the command is not re-sent, but if this function
returns -ERESTARTSYS to the VFS layer, won't the syscall be rewound and
re-executed from the beginning?

Since there is no tracking state to prevent it, will this cause the driver
to blindly resend the exact same command to the firmware? Could this lead
to duplicate execution of non-idempotent operations or resource leaks when
a successful command result is discarded and then re-run?

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

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> +static int se_resume(struct device *dev)
> +{
> +	struct se_if_priv *priv = dev_get_drvdata(dev);
> +	struct se_fw_load_info *load_fw;
> +	int ret = 0;
> +
> +	load_fw = get_load_fw_instance(priv);
> +
> +	if (load_fw->imem_mgmt) {
> +		ret = se_restore_imem_state(priv, &load_fw->imem);
> +		if (ret)
> +			dev_warn(dev, "Failure restoring IMEM state[0x%x]", ret);
> +	}
> +
> +	return 0;

[Severity: Medium]
Is it intentional to unconditionally return 0 from se_resume() even when
se_restore_imem_state() fails?

The implementation of se_restore_imem_state() returns -EIO on failure with
the stated intent that the PM subsystem knows the enclave is non-functional
after resume. 

By discarding the error code here, does this mask the failure from the PM
core and falsely inform it that the device resumed successfully?

> +}
> +
> +DEFINE_SIMPLE_DEV_PM_OPS(se_pm, se_suspend, se_resume);

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