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 <[email protected]>
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/[email protected]?part=5
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.