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

> > @@ -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.
> >                */
>
Not a bug.
The existing comment in ele_msg_addr_field.c explicitly documents the rationale for the message ELE_OEM_AUTH_CONTAINER_REQ.
- The firmware reads the container.
- Any accidental over-read would occur within firmware's own address space.
- The over-read contents are not returned to the caller.
- The caller has no visibility into firmware internal memory.
- The firmware does not write back through the supplied address.
Therefore, there is no obvious confidentiality or integrity impact visible to the attacker.

> [ ... ]
>
> > @@ -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;
> >  }

Accepted. Will fix this in v38.

>

> [ ... ]
>
> > @@ -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;
> >                       }
>
Accepted. Will be fixed v38.

> > 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.

Not an issue.
Ownership enforced by design: Only the registered cmd_receiver gets NVM callbacks;
handle values come from FW responses already in `dev_ctx`.

>
> [ ... ]
>
> > +     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?

Accepted. Will fix it in v38.
Will rename the purpose SE_IOCTL_ENABLE_CMD_RCV to
SE_IOCTL_ENABLE_CMD_RCV_STATUS.
This ioctl will not enable the command receiver. It will return the status of
enablement done as part of successful response to ELE_STORAGE_OPEN_REQ.


>
> > +     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;
> > +     }
>
Not an issue, once ioctl SE_IOCTL_ENABLE_CMD_RCV is re-purposed to SE_IOCTL_ENABLE_CMD_RCV_STATUS.
Function fw_api_specific_ops() now stores `strg_hdl` before calling function set_dev_ctx_as_command_receiver(),
so function cleanup_dev_ctx() can always close the handle in FW even if registration fails.
A `dev_err` log is emitted on failure.
Despite failure in executing function set_dev_ctx_as_command_receiver(), the successful response to Userspace is correct.
As Userspace must fetch the status via ioctl `SE_IOCTL_ENABLE_CMD_RCV_STATUS`, to know the enablement status of the
command receiver.

 > > 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;
> > +     }
> > +}
>
Not an issue.
FW sends these commands.
FW is sequential (FIFO); `se_if_cmd_lock` held by `ele_msg_send_rcv()` ensures only one export is ever in-flight.
No concurrent writer possible.

> [ ... ]
>
> > 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;
>
Not an issue.
`dma_alloc_coherent()` returns NULL on exhaustion; open fails gracefully.
Kernel fd limits and OOM killer provide adequate protection.

> [ ... ]
>
> > @@ -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) {
>
Accepted, will be fixed in v38.

> [ ... ]
>
> > +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.
Accepted will fix in v38.
When `!going_away` and `!cleanup_done`, `fw_busy` has already been atomically cleared at the top of `se_clear_fw_busy()`,
so `se_dev_ctx_shared_mem_cleanup()` can safely run.
The fix is to call it unconditionally in the non-teardown path:

>
> > +
> > +     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;
> > +}
>
func se_if_fops_read() is called by command receiver only.
But fw_api_specific_ops(), is called by both the functions se_if_fops_read() &
se_ioctl_cmd_snd_rcv_rsp_handler().
Fix this by splitting the function fw_api_specific_ops() to create cmd_receiver_specific_ops()
Which check do thing if command id matches ELE_STORAGE_MASTER_EXPORT_REQ or
ELE_STORAGE_CHUNK_EXPORT_REQ.



> --
> Sashiko AI
> review * https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> sashiko.dev%2F%23%2Fpatchset%2F20260819-imx-se-if-v37-0-
> 5ef5de9ff1dc%40nxp.com%3Fpart%3D5&data=05%7C02%7Cpankaj.gupta%40
> nxp.com%7C96eee7ca659c40ee378808defdbaf292%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C639227176597863159%7CUnknown%7CTWF
> pbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4z
> MiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=7UAS819b
> %2BvXWVh2QYDJQnMXp%2BROfVbi2OWTbrrsqYco%3D&reserved=0

NXP Confidential