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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The DMA buffer used for get_info is freed prematurely upon a timeout, allowing the enclave firmware to corrupt memory when it completes the transaction.
- [High] Allocating memory with GFP_KERNEL during the PM suspend/resume phases can cause system deadlocks.
- [Medium] A successfully completed firmware command is incorrectly reported as interrupted (-ERESTARTSYS), introducing a replay vulnerability.
- [Medium] se_restore_imem_state() ignores validation failure and returns success even if the IMEM state is bad.
- [Medium] Multi-byte fields in struct dev_info are populated via DMA by little-endian hardware but are accessed as native integers by the driver.
--

--- Patch [3]: [PATCH v35 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit 357109d983ad738d0dc813242f9d555c6b2d561c
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: add driver for NXP EdgeLock Enclave

This commit adds the MU-based communication interface for the NXP EdgeLock
secure enclave, enabling messaging between the Linux kernel and the enclave
firmware on i.MX SoC platforms.

Link: https://sashiko.dev/#/patchset/20260121-imx-se-if-v32-0-c5222df51cc2@nxp.com?part=3

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

[Severity: High]
If the ele_msg_send_rcv() transaction times out, the hardware might still
process the command and DMA the response payload later. Since this cleanup
function unconditionally frees the DMA buffer back to the pool or system,
could a delayed hardware response silently corrupt whatever kernel data is
subsequently allocated at this physical address?

[ ... ]
> +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;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	memset(s_info, 0x0, sizeof(*s_info));
> +
> +	struct se_api_msg *tx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_REQ_MSG_SZ, GFP_KERNEL);

[Severity: High]
Can this allocation cause a deadlock during system suspend? 

During system suspend, se_suspend() calls se_save_imem_state(), which invokes
ele_get_info(). Using GFP_KERNEL here and for the DMA buffer below can trigger
direct memory reclaim. If the reclaim process waits on I/O or swapping while
underlying storage devices are already suspended, will the system hang?

[ ... ]
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.h
[ ... ]
> +struct dev_info {
> +	u8  cmd;
> +	u8  ver;
> +	u16 length;
> +	u16 soc_id;
> +	u16 soc_rev;
> +	u16 lmda_val;
> +	u8  ssm_state;
> +	u8  dev_atts_api_ver;
> +	u8  uid[MAX_UID_SIZE];
> +	u8  sha_rom_patch[DEV_GETINFO_ROM_PATCH_SHA_SZ];
> +	u8  sha_fw[DEV_GETINFO_FW_SHA_SZ];
> +};

[Severity: Medium]
Since these multi-byte fields are populated directly via DMA from the NXP 
enclave firmware (which uses little-endian format), will they be read
incorrectly on big-endian architectures? 

For instance, get_se_soc_info() directly accesses s_info->d_info.soc_rev
without any byte-swapping, which might yield an invalid version number on
big-endian systems. Should these be defined as __le16 instead?

[ ... ]
> --- /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: Medium]
Does this unintentionally convert a successful firmware transaction into an
error?

When a wait is interrupted by a signal, ele_msg_rcv() continues waiting 
uninterruptibly. When the firmware finally responds successfully, err will
be positive. By overwriting this success with -ERESTARTSYS, won't this 
cause the kernel to silently replay the syscall from userspace? For 
non-idempotent operations, executing the command twice could corrupt enclave 
state.

[ ... ]
> +int se_restore_imem_state(struct se_if_priv *priv, struct se_imem_buf *imem)
> +{
[ ... ]
> +	/*
> +	 * After importing IMEM, check if IMEM state is equal to 0xCA
> +	 * to ensure IMEM is fully loaded and
> +	 * ELE functionality can be used.
> +	 */
> +	ret = ele_get_info(priv, &s_info);
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to get info from ELE.");
> +		return ret;
> +	}
> +	imem->state = s_info.d_addn_info.imem_state;
> +
> +	if (s_info.d_addn_info.imem_state == ELE_IMEM_STATE_OK)
> +		dev_dbg(priv->dev, "Successfully restored IMEM.");
> +	else
> +		dev_err(priv->dev, "Failed to restore IMEM.");
> +
> +	return ret;
> +}

[Severity: Medium]
If the IMEM state is bad (not ELE_IMEM_STATE_OK), the code logs an error
but still returns the success code (ret = 0) from the preceding ele_get_info()
call. Does this hide the hardware failure from the power management subsystem
and leave the enclave in a non-functional state after a system resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-imx-se-if-v35-0-11b25bb308ef@nxp.com?part=3