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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] The physical/DMA address of the encrypted IMEM buffer (`load_fw->imem.phyaddr`) is never initialized after allocation, causing the firmware to write exported IMEM state to physical address 0.
- [High] If `ele_msg_send()` fails, `ele_msg_send_rcv()` returns early without clearing `priv->waiting_rsp_clbk_hdl.rx_msg`, leaving a dangling pointer that can be written to by the interrupt handler.
- [High] If a firmware response timeout occurs, `tx_msg` is freed by the caller while its pointer is still queued asynchronously in the mailbox framework.
- [High] Global static state `var_se_info` is shared across all instances of the device, leading to memory leaks and state corruption when multiple MU instances probe.
- [High] `ele_service_swap()` silently truncates the 64-bit physical address to 32 bits, and the driver fails to configure a 32-bit DMA mask.
- [High] The SoC device registered via `soc_device_register()` is never unregistered on driver removal, leading to a memory leak and sysfs UAF.
- [High] `se_if_probe_cleanup()` manually calls `dmam_free_coherent()` on devres-managed memory, causing a double-free and passing an invalid handle.
- [High] Hardcoded `MAX_SCHEDULE_TIMEOUT` is applied to active command receivers, bypassing the timeout budget and causing an unkillable deadlock.
- [Medium] Mixing `goto`-based cleanup with `__free()` attribute declarations in the same function.
- [Medium] The response buffer pointer `rx_msg` is cleared locklessly on the success path, allowing a Write-After-Free if a spurious response arrives.
- [Low] In the resume handler, the return value of `se_restore_imem_state()` is not assigned to the `ret` variable, silently swallowing hardware restore errors.
--

commit 6df55de736269bcb35dff3283a274cacbfb24e1f
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: add driver for NXP EdgeLock Enclave

This commit adds a messaging unit (MU) based communication interface for the
NXP EdgeLock Enclave (ELE). It exposes APIs via file descriptors for
configuring shared memory and exchanging messages.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 0000000000000..54d79c3d75af6
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.c
[ ... ]
> +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
> +{
[ ... ]
> +	struct se_api_msg *tx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_REQ_MSG_SZ, GFP_KERNEL);
> +	if (!tx_msg)
> +		return -ENOMEM;
> +
> +	struct se_api_msg *rx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_RSP_MSG_SZ, GFP_KERNEL);
> +	if (!rx_msg)
> +		return -ENOMEM;
[ ... ]
> +	ret = se_val_rsp_hdr_n_status(priv, rx_msg, ELE_GET_INFO_REQ,
> +				      ELE_GET_INFO_RSP_MSG_SZ, true);
> +	if (ret < 0)
> +		goto exit;
> +
> +	memcpy(s_info, get_info_data, sizeof(*s_info));
> +exit:
> +	if (priv->mem_pool)
> +		gen_pool_free(priv->mem_pool, (unsigned long)get_info_data,
> +			      ELE_GET_INFO_BUFF_SZ);
> +	else
> +		dma_free_coherent(priv->dev, ELE_GET_INFO_BUFF_SZ,
> +				  get_info_data, get_info_addr);
> +
> +	return ret;
> +}

[Severity: Medium]
Does this mix goto-based cleanup with __free() variable declarations?

Looking at ele_get_info(), tx_msg and rx_msg use the __free(kfree) scoped
cleanup macro, but the error paths use a goto exit label to manage the
cleanup of get_info_data. The cleanup subsystem guidelines prohibit mixing
these two error handling paradigms in the same function scope because it
obscures ownership semantics and can lead to double-frees or leaks.

[ ... ]
> +int ele_service_swap(struct se_if_priv *priv,
> +		     phys_addr_t addr,
> +		     u32 addr_size, u16 flag)
> +{
[ ... ]
> +	tx_msg->data[0] = flag;
> +	tx_msg->data[1] = addr_size;
> +	tx_msg->data[2] = ELE_NONE_VAL;
> +	tx_msg->data[3] = lower_32_bits(addr);
> +	tx_msg->data[4] = se_get_msg_chksum((u32 *)&tx_msg[0],
> +					    ELE_SERVICE_SWAP_REQ_MSG_SZ);

[Severity: High]
Does this silently truncate the 64-bit physical address to 32 bits?

If upper_32_bits(addr) is non-zero, this passes only the lower 32 bits to
the firmware without validation. When combined with dmam_alloc_coherent()
calls that lack a restricted 32-bit DMA mask, the memory could be allocated
above 4GB, causing the firmware to write the enclave state to the wrong
address.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 0000000000000..ba606f4e8be83
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> +int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl)
> +{
> +	bool wait_uninterruptible = false;
> +	unsigned long remaining_jiffies;
> +	unsigned long flags;
> +	int ret;
> +
> +	remaining_jiffies = MAX_SCHEDULE_TIMEOUT;
> +	do {
> +		if (wait_uninterruptible)
> +			ret = wait_for_completion_timeout(&se_clbk_hdl->done,
> +							  remaining_jiffies);

[Severity: High]
Does this hardcoded MAX_SCHEDULE_TIMEOUT bypass the timeout budget?

If userspace sends a command and the firmware never responds, the task will
block indefinitely in D-state. Since this holds the global se_if_cmd_lock
mutex, it could completely freeze the driver.

[ ... ]
> +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)
> +{
> +	int err;
> +
> +	guard(mutex)(&priv->se_if_cmd_lock);
> +
> +	if (atomic_read(&priv->fw_busy)) {
> +		dev_dbg(priv->dev, "ELE became unresponsive.\n");
> +		return -EBUSY;
> +	}
> +	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
> +	priv->waiting_rsp_clbk_hdl.rx_msg_sz = exp_rx_msg_sz;
> +	priv->waiting_rsp_clbk_hdl.rx_msg = rx_msg;
> +
> +	err = ele_msg_send(priv, tx_msg, tx_msg_sz);
> +	if (err < 0)
> +		return err;

[Severity: High]
If ele_msg_send() fails and returns early, is rx_msg left in a dangling
state?

Because priv->waiting_rsp_clbk_hdl.rx_msg is set before the call to
ele_msg_send(), an error here leaves the pointer populated. If the caller
then frees the buffer and a spurious response arrives, the interrupt handler
might memcpy into the freed memory, causing a write-after-free.

> +
> +	err = ele_msg_rcv(priv, &priv->waiting_rsp_clbk_hdl);

[Severity: High]
If ele_msg_rcv() times out and returns -ETIMEDOUT, does this leave the
pending message in the mailbox framework?

The caller frees tx_msg upon error, but since the mailbox client is configured
with tx_block = false, the pointer might still be queued in the mailbox
framework. When the hardware eventually processes it, the controller driver
could dereference the freed memory.

> +
> +	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
> +		err = -EINTR;
> +		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> +		dev_err(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
> +	}
> +	priv->waiting_rsp_clbk_hdl.rx_msg = NULL;
> +
> +	return err;
> +}

[Severity: Medium]
Is it safe to clear rx_msg here without holding clbk_rx_lock?

If a spurious response arrives immediately after a successful reception,
se_if_rx_callback() on another CPU might acquire the spinlock and see the
non-NULL pointer before this lockless assignment propagates, potentially
writing to the buffer after it has been freed by the caller.

[ ... ]
> +int se_save_imem_state(struct se_if_priv *priv, struct se_imem_buf *imem)
> +{
[ ... ]
> +	/*
> +	 * EXPORT command will save encrypted IMEM to given address,
> +	 * so later in resume, IMEM can be restored from the given
> +	 * address.
> +	 *
> +	 * Size must be at least 64 kB.
> +	 */
> +	ret = ele_service_swap(priv, imem->phyaddr, ELE_IMEM_SIZE, ELE_IMEM_EXPORT);

[Severity: Critical]
Can this pass an uninitialized physical address to the firmware?

Looking at se_if_probe(), the DMA address allocated is stored in a local
variable (imem_dma_addr) but is never assigned to load_fw->imem.phyaddr. As a
result, this passes 0 to ele_service_swap(). Writing 64KB to physical address
0 could corrupt critical system memory or exception vectors upon suspend.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 0000000000000..9a2c3c6111462
> --- /dev/null
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> +struct se_if_node {
> +	struct se_soc_info *se_info;
> +	u8 *pool_name;
> +	bool reserved_dma_ranges;
> +	struct se_if_defines if_defs;
> +};
> +
> +/* common for all the SoC. */
> +static struct se_var_info var_se_info;

[Severity: High]
Is it intentional that var_se_info is a global static variable rather than
per-device state?

If multiple MU instances probe, they will all share and overwrite this
singleton state. During unbind, se_if_probe_cleanup() could call
dmam_free_coherent() using a specific device's devres context on a shared
buffer pointer, causing memory leaks or devres mismatch corruption.

[ ... ]
> +static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se_info)
> +{
[ ... ]
> +	attr->serial_number = devm_kasprintf(priv->dev,
> +					     GFP_KERNEL, "%016llX",
> +					     GET_SERIAL_NUM_FROM_UID(s_info->d_info.uid,
> +								     MAX_UID_SIZE >> 2));
> +
> +	sdev = soc_device_register(attr);
> +	if (IS_ERR(sdev))
> +		return PTR_ERR(sdev);
> +
> +	return 0;
> +}

[Severity: High]
Does this permanently discard the handle needed to unregister the SoC device?

The returned soc_device handle is not saved. When the driver unbinds, devres
frees the attr memory, but because soc_device_unregister() is never called,
the device node remains active in sysfs. Any userspace process reading the
lingering attributes will access freed memory.

[ ... ]
> +static void se_if_probe_cleanup(void *plat_dev)
> +{
> +	struct platform_device *pdev = plat_dev;
> +	struct se_fw_load_info *load_fw;
> +	struct device *dev = &pdev->dev;
> +	struct se_if_priv *priv;
[ ... ]
> +	/*
> +	 * free the buffer in se remove, previously allocated
> +	 * in se probe to store encrypted IMEM
> +	 */
> +	if (load_fw && load_fw->imem.buf) {
> +		dmam_free_coherent(dev, ELE_IMEM_SIZE, load_fw->imem.buf,
> +				   load_fw->imem.phyaddr);
> +		load_fw->imem.buf = NULL;
> +	}

[Severity: High]
Does this explicitly free memory that is already managed by devres?

Because dmam_alloc_coherent() allocates devres-managed memory, the kernel
will automatically free it on teardown. Calling dmam_free_coherent() here
causes a double-free. Furthermore, since imem.phyaddr was never populated,
this passes an invalid DMA handle to the allocator.

[ ... ]
> +static int se_if_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (se_info->se_fw_img_nm.seco_fw_nm_in_rfs) {
> +		load_fw = get_load_fw_instance(priv);
> +		load_fw->se_fw_img_nm = &se_info->se_fw_img_nm;
> +		load_fw->is_fw_tobe_loaded = true;
> +
> +		if (load_fw->se_fw_img_nm->prim_fw_nm_in_rfs) {
> +			/* allocate buffer where SE store encrypted IMEM */
> +			imem_dma_addr = phys_to_dma(priv->dev, load_fw->imem.phyaddr);
> +			load_fw->imem.buf = dmam_alloc_coherent(priv->dev, ELE_IMEM_SIZE,
> +								&imem_dma_addr, GFP_KERNEL);

[Severity: Critical]
Is the allocated DMA address supposed to be saved in load_fw->imem.phyaddr?

The resulting imem_dma_addr is stored locally but never written back to
load_fw->imem.phyaddr. It remains 0, which is later passed to the firmware
in se_save_imem_state().

[Severity: High]
Is the device's DMA mask configured to 32-bits before this allocation?

Since ele_service_swap() silently truncates physical addresses to 32 bits,
allocating memory without enforcing a 32-bit DMA mask could result in buffers
above 4GB, which the firmware will truncate and then write to the wrong
address.

[ ... ]
> +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) {
> +		se_restore_imem_state(priv, &load_fw->imem);
> +		if (ret)
> +			dev_err(dev, "Failure restoring IMEM state[0x%x]", ret);
> +	}
> +
> +	return 0;
> +}

[Severity: Low]
Does this accidentally swallow the return value of se_restore_imem_state()?

The function call does not assign its result to ret. Because ret is
initialized to 0, the subsequent error check if (ret) will always fail,
masking any hardware restore failures.

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