Re: [PATCH v37 5/7] firmware: imx: adds miscdev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Race condition between ELE_STORAGE_OPEN_REQ pre-check and command receiver registration allows hijacking NVM traffic.
- [High] Global static variables used to store firmware-requested DMA buffer sizes can be overwritten by concurrent operations, leading to out-of-bounds DMA.
- [High] Flawed DMA address validation for input-only buffers with unbounded sizes allows out-of-bounds DMA reads.
- [High] A malicious command receiver can bypass the DMA end-bound check for storage export responses by supplying a small size to read(), causing the expected buffer size to be zero-padded.
- [High] Internal device context (priv_dev_ctx) permanently leaks shared memory on firmware timeout.
- [High] The fix for the unbind race condition (using !rx_delivered instead of !completion_done()) was only applied to the -ERESTARTSYS path, leaving the timeout and deadline paths vulnerable to the exact same DMA-after-free.
- [High] Use-After-Free of the devname string pointer in se_if_rx_callback() due to accessing it after dropping clbk_rx_lock.
- [High] The command receiver bypasses strict session ownership checks, allowing it to maliciously spoof handles and close arbitrary sessions belonging to other processes.
- [High] The driver unconditionally allocates 128KB of coherent DMA memory every time a user opens the misc device, without tracking total allocations or limiting the number of open file descriptors.
- [Medium] Self-deadlock in fw_api_specific_ops() via recursive acquisition of priv->modify_lock.
--

--- Patch [5]: [PATCH v37 5/7] firmware: imx: adds miscdev ---
commit 6bbd6617ac10e062e148f028c3c0f9417699688c
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, that
enables exchanging messages with NXP secure enclave HW IP(s)
like EdgeLock Enclave, from:
- User-Space Applications via character driver.

ABI documentation for the NXP secure-enclave driver.

Link: https://github.com/nxp-imx/imx-secure-enclave.git
Link: https://github.com/nxp-imx/imx-smw.git

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index b662063c3b1c2..e1c53b3c7036c 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -110,6 +110,9 @@ int se_val_cmd_addrs(struct se_if_device_ctx *dev_ctx, struct se_api_msg *msg,
>  		/* Zero marks an absent optional buffer. */
>  		if (!addr)
>  			continue;
>  
>  		if (addr < base || addr >= end)
>  			return -EACCES;
>  
>  		/*
>  		 * When the message also carries this buffer's length, the whole

[Severity: High]
Does this validation allow out-of-bounds DMA reads? For input-only buffers
like ELE_OEM_AUTH_CONTAINER_REQ in ele_common.c, the size_idx is set to
SE_CMD_ADDR_NO_SIZE and buf_size to 0. This bypasses the end-boundary
check entirely. Could an attacker place the start address near the end of
the shared memory window and provide a large embedded length in the payload,
forcing the firmware to read past the DMA window into adjacent physical
memory?

>  		 * buffer [addr, addr + len) must fit inside the window, not just
>  		 * its start. addr is already >= base and < end here, so end - addr
>  		 * is a positive value and the comparison cannot overflow.
>  		 */

[ ... ]

> @@ -133,6 +133,6 @@ int se_val_cmd_addrs(struct se_if_device_ctx *dev_ctx, struct se_api_msg *msg,
>  		} else if (f->buf_size) {
>  			/* buf_size: literal byte count
>  			 *           (FW-defined constant or saved at runtime).
>  			 */
>  			if ((u64)f->buf_size > end - addr)
>  				return -EACCES;
>  		}

[Severity: High]
Could this be bypassed if buf_size is dynamically set to zero? If a malicious
command receiver provides a small size to se_if_fops_read() in se_ctrl.c, the
message could be truncated, leaving rx_msg_snap zero-padded.
fw_api_specific_ops() would then read data[1] as 0 and pass it to
ele_set_sz_in_field_addr(), setting buf_size to 0. This would bypass the
end-bound check entirely in se_val_cmd_addrs(), potentially allowing
out-of-bounds DMA.

>  	}
>  
>  	return 0;
>  }

[ ... ]

> @@ -230,8 +230,8 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
>  			if (time_after_eq(now, deadline_jiffies)) {
>  				/* Deadline hit: fence hung FW, like the ret==0 path. */
>  				spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  				se_clbk_hdl->rx_msg = NULL;
>  				if (!completion_done(&se_clbk_hdl->done))
>  					se_mark_fw_busy(dev_ctx);
>  				spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);

[Severity: High]
Is it possible for this path to suffer from the same DMA-after-free
regression that was fixed in the signal interruption path? The time_after_eq
deadline hit path and the ret == 0 timeout path in ele_msg_rcv() still use
completion_done() instead of checking rx_delivered. If a timeout races with
teardown, could the circuit breaker fail to arm, causing the DMA buffer to
be freed while firmware is still writing to it?

>  				ret = -ETIMEDOUT;
>  				break;
>  			}

[ ... ]

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> new file mode 100644
> index 0000000000000..0c4c6f7de0205
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_fw_api.c
> @@ -0,0 +1,361 @@

[ ... ]

> +static int se_cmd_receiver_allowed_cmd(struct se_if_device_ctx *dev_ctx,
> +				       struct se_api_msg *msg, u32 tx_msg_sz)
> +{
> +	u8 cmd = msg->header.command;
> +
> +	switch (cmd) {
> +	case ELE_SESSION_CLOSE_REQ:
> +	case ELE_STORAGE_CLOSE_REQ:
> +		return 0;

[Severity: High]
Does this allow the command receiver to bypass strict session ownership
checks? By returning 0 unconditionally for ELE_SESSION_CLOSE_REQ and
ELE_STORAGE_CLOSE_REQ in se_cmd_receiver_allowed_cmd(), it seems a malicious
command receiver could spoof handles and close arbitrary sessions belonging
to other processes.

> +	case ELE_STORAGE_MASTER_IMPORT_REQ:
> +		const struct se_cmd_addr_field *fields;
> +		size_t count;

[ ... ]

> +	case ELE_STORAGE_OPEN_REQ:
> +		/* Might be cleared as part of tear down. */
> +		if (dev_ctx->strg_hdl) {
> +			ret = -EEXIST;
> +			break;
> +		}
> +		/*
> +		 * Reject the storage-open request when another context is
> +		 * already registered as the command receiver. If we let the
> +		 * command through, FW would allocate a new storage handle and
> +		 * start sending NVM callbacks for it; those callbacks would be
> +		 * routed to the existing receiver (process A), not to the
> +		 * caller (process B). This would let process A observe and
> +		 * tamper with process B's NVM traffic. Reject early, before
> +		 * the command reaches FW, so no handle is allocated and the
> +		 * state stays consistent.
> +		 */
> +		scoped_guard(mutex, &priv->modify_lock)
> +			if (priv->cmd_receiver_clbk_hdl.dev_ctx &&
> +			    priv->cmd_receiver_clbk_hdl.dev_ctx != dev_ctx)
> +				ret = -EBUSY;

[Severity: High]
Is there a race condition here since modify_lock is dropped before the
command is sent to firmware? Could another process hijack the command
receiver slot via SE_IOCTL_ENABLE_CMD_RCV during this window, causing the
NVM traffic for this storage session to be routed to the attacker instead?

> +		if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> +			ret = -EINVAL;
> +		break;

[ ... ]

> +	case ELE_STORAGE_OPEN_REQ: {
> +		int rc = 0;
> +
> +		/*
> +		 * Record the storage handle before registering as command
> +		 * receiver. FW has already allocated the handle; if we assigned
> +		 * it only after a successful registration, a failing
> +		 * set_dev_ctx_as_command_receiver() (e.g. -EBUSY) would leave
> +		 * strg_hdl at 0 while the ioctl still returns success to
> +		 * userspace. The kernel would then never close the handle on
> +		 * teardown, leaking it in FW. Storing it first guarantees
> +		 * cleanup_dev_ctx() closes it regardless of registration.
> +		 */
> +		dev_ctx->strg_hdl = rx_msg->data[1];
> +
> +		rc = set_dev_ctx_as_command_receiver(dev_ctx);
> +		if (rc)
> +			dev_err(priv->dev,
> +				"Failed to register %s as CMD-Receiver: %d\n",
> +				dev_ctx->devname, rc);

[Severity: High]
If set_dev_ctx_as_command_receiver() fails and returns -EBUSY, the error is
logged but execution continues in fw_api_specific_ops(). Could this leave
the storage handle assigned to the caller while the actual NVM traffic gets
routed to an attacker who won the race to become the command receiver?

> +		break;
> +	}

[ ... ]

> diff --git a/drivers/firmware/imx/ele_msg_addr_field.c b/drivers/firmware/imx/ele_msg_addr_field.c
> new file mode 100644
> index 0000000000000..1da6583b6c312
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> @@ -0,0 +1,654 @@

[ ... ]

> +void ele_set_sz_in_field_addr(u8 cmd, u32 size)
> +{
> +	switch (cmd) {
> +	case ELE_STORAGE_MASTER_EXPORT_REQ:
> +		ele_storage_master_export_addr_fields[0].buf_size = size;
> +		break;

[Severity: High]
Is it safe to mutate the buf_size of a global static array here? If multiple
sessions or devices handle concurrent export commands, could this create a
data race in ele_set_sz_in_field_addr() where one operation overwrites the
expected buffer size of another, potentially leading to out-of-bounds DMA?

> +	case ELE_STORAGE_CHUNK_EXPORT_REQ:
> +		ele_storage_chunk_export_addr_fields[0].buf_size = size;
> +		break;
> +	}
> +}

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index a8974eef190b6..56a4e53be37b8 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> +static int init_se_shared_mem(struct se_if_device_ctx *dev_ctx)
> +{
> +	struct se_shared_mem_mgmt_info *se_shared_mem_mgmt = &dev_ctx->se_shared_mem_mgmt;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	INIT_LIST_HEAD(&se_shared_mem_mgmt->pending_out);
> +	INIT_LIST_HEAD(&se_shared_mem_mgmt->pending_in);
> +
> +	if (priv->mem_pool)
> +		INIT_LIST_HEAD(&se_shared_mem_mgmt->mem_pool_buf_list);
> +
> +	se_shared_mem_mgmt->non_secure_mem.ptr =
> +			dma_alloc_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
> +					   &se_shared_mem_mgmt->non_secure_mem.dma_addr,
> +					   GFP_KERNEL);

[Severity: High]
Does this unconditionally allocate 128KB of contiguous DMA memory every time
a user opens the device? Without tracking total allocations or limiting the
number of open file descriptors in se_if_fops_open(), could a malicious user
open the device in a loop and quickly exhaust the CMA/DMA memory pools,
causing a system-wide denial of service?

> +	if (!se_shared_mem_mgmt->non_secure_mem.ptr)
> +		return -ENOMEM;

[ ... ]

> @@ -540,6 +540,6 @@ void se_if_rx_callback(...)
>  		se_clbk_hdl->rx_msg_sz = min_t(u32, rx_msg_sz, MAX_NVM_MSG_LEN);
>  		devname = se_clbk_hdl->dev_ctx->devname;
>  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
>  		complete(&se_clbk_hdl->done);
>  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  		if (sz_mismatch)
>  			dev_err(dev,
>  				"%s: CMD-RCVER NVM: hdr(0x%x) with different sz(%d != %d).\n",
>  				devname, *(u32 *)header,

[Severity: High]
Is it safe to dereference devname after releasing the clbk_rx_lock? Since
complete() is called before the lock is dropped in se_if_rx_callback(),
could a user process wake up, close the file descriptor, and free dev_ctx
and its devname before dev_err() is executed, resulting in a use-after-free?
This also appears to happen in the rsp_tag path below.

>  				(header->size << 2), rx_msg_sz);
>  	} else if (header->tag == priv->if_defs->rsp_tag) {

[ ... ]

> +static void se_clear_fw_busy(struct se_if_priv *priv)
> +{
> +	struct se_if_device_ctx *dev_ctx = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->fw_busy_lock, flags);
> +	dev_ctx = priv->fw_busy_dev_ctx;
> +	priv->fw_busy_dev_ctx = NULL;
> +	atomic_set(&priv->fw_busy, 0);
> +	spin_unlock_irqrestore(&priv->fw_busy_lock, flags);
> +
> +	if (!dev_ctx)
> +		return;

[ ... ]

> +	scoped_guard(mutex, &dev_ctx->fops_lock) {
> +		if (atomic_read(&priv->going_away)) {
> +			/*
> +			 * Fatal, but deliberately non-panic: the enclave is
> +			 * unresponsive at unbind with a transaction still in
> +			 * flight. Both the coherent staging buffer and any
> +			 * gen_pool buffers this context owns are abandoned
> +			 * (host descriptors freed, DMA-visible memory leaked)
> +			 * to avoid a DMA-after-free while the enclave may still
> +			 * be writing. Emit one headline error here rather than
> +			 * per-buffer so the count of faulted contexts is clear.
> +			 * Do not use WARN/BUG: this path is recoverable and
> +			 * panic_on_warn kernels must not be brought down by it.
> +			 */
> +			dev_err(priv->dev,
> +				"%s: FATAL: enclave stuck at unbind, DMA leaked.\n",
> +				dev_ctx->devname);
> +			cleanup_se_shared_mem(dev_ctx, false);
> +		} else if (dev_ctx->cleanup_done) {
> +			cleanup_se_shared_mem(dev_ctx, true);
> +		}
> +	}

[Severity: High]
Could this permanently leak the 128KB shared memory slot for priv_dev_ctx
if a firmware timeout occurs? Since priv_dev_ctx->cleanup_done is false
until module unload, se_clear_fw_busy() skips the cleanup pass when a late
response arrives. Because se_dev_ctx_shared_mem_cleanup() won't reset pos
while the context is marked busy, the allocation is never reclaimed.

> +
> +	kref_put(&dev_ctx->refcount, se_if_dev_ctx_release);
> +}

[ ... ]

> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
> +			       loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	u8 rx_msg_snap[MAX_NVM_MSG_LEN] = {};

[ ... ]

> +		/* We may need to copy the output data to user before
> +		 * delivering the completion message.
> +		 */
> +		err = se_dev_ctx_cpy_out_data(dev_ctx);
> +		if (err < 0) {
> +			se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +			mutex_unlock(&dev_ctx->fops_lock);
> +			return err;
> +		}
> +		/* Copy data from the buffer using the snapshot taken under the lock. */
> +		print_hex_dump_debug("to user ", DUMP_PREFIX_OFFSET, 4, 4,
> +				     rx_msg_snap, copy_len, false);
> +
> +		fw_api_specific_ops(dev_ctx, (struct se_api_msg *)rx_msg_snap);
> +		err = copy_len;

[Severity: Medium]
Can this lead to a self-deadlock? se_if_fops_read() holds priv->modify_lock
when it calls fw_api_specific_ops(). If the firmware sends a command like
ELE_STORAGE_OPEN_REQ or ELE_STORAGE_CLOSE_REQ, fw_api_specific_ops() will
attempt to re-acquire priv->modify_lock either directly or via
set_dev_ctx_as_command_receiver(), causing a deadlock.

> +		if (copy_to_user(buf, rx_msg_snap, copy_len))
> +			err = -EFAULT;
> +
> +		se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +		mutex_unlock(&dev_ctx->fops_lock);
> +	}
> +
> +	return err;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-imx-se-if-v37-0-5ef5de9ff1dc@nxp.com?part=5