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

On Thu, Aug 06, 2026 at 11:17:23AM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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/[email protected]?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?

This one make sense, should return failure.

Frank

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