RE: [PATCH v36 5/7] firmware: imx: adds miscdev

> > --- a/drivers/firmware/imx/ele_common.c
> > +++ b/drivers/firmware/imx/ele_common.c
> > @@ -101,15 +101,15 @@ int se_val_cmd_addrs(struct se_if_device_ctx
> > *dev_ctx, struct se_api_msg *msg,
> >
> >             addr = msg->data[f->lsb_idx];
> >             if (f->has_msb)
> >                     addr |= (u64)msg->data[f->msb_idx] << 32;
> >
> >             /* 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
> >              * buffer [addr, addr + len) must fit inside the window, not
> just
>
> [Severity: High]
> Does this code prevent firmware from reading outside the shared memory
> window when the command lacks an explicit buffer size?
>
> For commands like ELE_OEM_AUTH_CONTAINER_REQ that use
> SE_CMD_ADDR_NO_SIZE with no buf_size, only the start address is checked
> against the bounds. If a caller provides a start address at the very end of the
> window (e.g., end - 1), could the firmware read the payload out-of-bounds
> into adjacent physical memory?

- Not a bug - by design.
- The existing comment explicitly documents the rationale:
   -- this is a read-only input buffer
       --- (firmware copies the container header into its own internal memory for authentication; no DMA write-back through this address).
   -- A rogue caller can at most cause firmware to read within the shared-memory window it already owns.
- Output buffers must be fully bounded;
- Input-only buffers are safe with addr-only checks.
- This is an input buffer. No fix needed.

>
> [ ... ]
>
> > @@ -290,16 +290,16 @@ int ele_msg_rcv(struct se_if_device_ctx
> *dev_ctx, struct se_clbk_handle *se_clbk
> >                      * before the fatal signal: se_if_rx_callback() has
> already
> >                      * copied it and set rx_delivered under the same lock,
> so the
> >                      * enclave is done with the buffer. Report it as a
> normal
> >                      * receive (rx_msg_sz) so the handle it carries is still
> >                      * recorded and later closed, rather than leaked.
> >                      */
> >                     if (is_rsp_wait_with_timeout) {
> >                             spin_lock_irqsave(&se_clbk_hdl-
> >clbk_rx_lock, flags);
> >                             if (se_clbk_hdl->rx_delivered) {
> >                                     ret = se_clbk_hdl->rx_msg_sz;
> >                                     spin_unlock_irqrestore(&se_clbk_hdl-
> >clbk_rx_lock, flags);
> >                                     break;
> >                             }
> >                             if (se_clbk_hdl->rx_msg) {
> >                                     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]
> Can a concurrent fatal signal and driver unbind bypass the firmware-busy
> circuit breaker here?
>
> If a process blocking in ele_msg_rcv() receives a fatal signal at the exact
> moment se_if_probe_cleanup() executes and calls complete_all(), the wait
> completes. The signal path wakes up, and completion_done() evaluates to
> true.
> This causes the signal path to skip calling se_mark_fw_busy().
>
> Could this lead to teardown freeing the DMA buffer via
> cleanup_se_shared_mem(..., true) while the firmware is still busy and writing
> into the freed memory?

Accepted. Will call se_mark_fw_busy() un-conditionally in this path, to prevent UAF.

>
> > --- a/drivers/firmware/imx/ele_fw_api.c
> > +++ b/drivers/firmware/imx/ele_fw_api.c
> > @@ -180,14 +180,14 @@ void fw_api_specific_ops(struct se_if_device_ctx
> *dev_ctx, struct se_api_msg *rx
> >     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);
> >             break;
> >     }
>
> [Severity: High]
> Does this silently ignore a registration failure and leave the state out of sync?
>
> If process A already opened a storage session and is the command receiver,
> and process B calls ELE_STORAGE_OPEN_REQ,
> set_dev_ctx_as_command_receiver() will return -EBUSY. However, the error is
> only logged, and dev_ctx->strg_hdl remains set.
>
> Could this trick the firmware into sending NVM callbacks for process B's
> storage session that then get routed to process A?
>

Accepted. Will add preventive check.

> > --- a/drivers/firmware/imx/ele_msg_addr_field.c
> > +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> > @@ -568,10 +568,10 @@ static const struct se_cmd_addr_field
> ele_rng_get_random_addr_fields[] = {
> >     /*
> >      * PSA_COMPLIANT get-random payload. reserved/flags is data[0];
> > rnd_addr
>
> [ ... ]
>
> >  static struct se_cmd_addr_field ele_storage_master_export_addr_fields[] =
> {
> >     { .lsb_idx = 2, .flag_idx = SE_CMD_ADDR_ALWAYS,
> >       .size_idx = SE_CMD_ADDR_NO_SIZE, .buf_size = 0 },     /*
> key_store_export_address */
> >  };
>
> [ ... ]
>
> >  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;
> >     case ELE_STORAGE_CHUNK_EXPORT_REQ:
> >             ele_storage_chunk_export_addr_fields[0].buf_size = size;
> >             break;
> >     }
> >  }
>
> [Severity: High]
> Can concurrent operations cause a race condition when storing transaction
> sizes in these global static arrays?
>
> If two callers concurrently trigger ELE_STORAGE_MASTER_EXPORT_REQ,
> fw_api_specific_ops() calls ele_set_sz_in_field_addr() which mutates the
> shared global array ele_storage_master_export_addr_fields without
> synchronization.
>
> If a context requiring a large buffer is overwritten by a context supplying a
> small size, could se_val_cmd_addrs() use the clobbered small size, pass the
> bounds check, and allow the firmware to write the large export out-of-
> bounds?
>
- The command-receiver is a slave to the FW.
- FW sends commands to its slave to fetch information from command receiver,
that are required by FW to process a command request initiated by the waiting dev-context
while holding the mutex se_if_cmd_lock (via ele_msg_send_rcv()).
- ELE FW processes commands sequentially in FIFO order.
- There is never more than one concurrent in-flight export transaction, so ele_set_sz_in_field_addr() is never called concurrently.
- No fix needed


> > --- a/drivers/firmware/imx/se_ctrl.c
> > +++ b/drivers/firmware/imx/se_ctrl.c
> > @@ -1727,14 +1727,14 @@ static int
> se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> >      * ioctl reports plain success; the firmware status is conveyed to
> >      * userspace inside the response buffer itself.
> >      */
> >     cmd_snd_rcv_rsp_info.rx_buf_sz = err;
> >     err = 0;
> >
> >     dev_dbg(priv->dev, "%s: %s %s.", dev_ctx->devname, __func__,
> >             "message received, start transmit to user");
> >
> >     rsp_status_err =
> >             se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg-
> >header.command,
> >                                     cmd_snd_rcv_rsp_info.rx_buf_sz,
> >                                     tx_msg->header.ver == priv->if_defs-
> >base_api_ver);
> >
> >     if (!rsp_status_err) {
> >             /*
> >              * The response is well formed and fully fits the caller's
> >              * buffer, so any FW-allocated session/storage handle it
> carries
> >              * (data[1]) has been delivered. Record it now, before the
> >              * copy-out steps below. The FW has already committed the
> handle;
> >              * running fw_api_specific_ops() only after a successful
> >              * se_dev_ctx_cpy_out_data()/copy_to_user() would leave
> the
> >              * handle untracked - and so never closed on teardown,
> leaking it
> >              * in FW - whenever the caller supplied a bad output pointer.
> >              */
> >             fw_api_specific_ops(dev_ctx, rx_msg);
> >
> >             err = se_dev_ctx_cpy_out_data(dev_ctx);
>
> [Severity: Medium]
> Does a short rx_buf_sz from userspace bypass handle recording and cause a
> resource leak?
>
> If a caller issues ELE_SESSION_OPEN_REQ but specifies an artificially small
> rx_buf_sz (e.g., 4 bytes), se_val_rsp_hdr_n_status() will return -EINVAL
> because the returned buffer size is smaller than the firmware's header length.
>
> Because rsp_status_err is set, the call to fw_api_specific_ops() is skipped.
> Will the successfully allocated handle be permanently leaked since it is never
> recorded in dev_ctx->sess_hdl for teardown?
>
Accepted. Will fix this in v37, by checking the rx_msg_sz for control messages ELE_OPEN_SESSION and ELE_OPEN_STORAGE only.

> [ ... ]
>
> > @@ -1849,21 +1849,21 @@ static ssize_t se_if_fops_read(struct file *fp,
> char __user *buf, size_t size,
> >     /*
> >      * Snapshot rx_msg pointer under clbk_rx_lock before releasing it.
> >      * unset_dev_ctx_as_command_receiver() can acquire the lock, NULL
> out
> >      * rx_msg, and free the buffer at any time after the unlock; using a
> >      * stale pointer from the shared field after the unlock is a UAF.
> >      */
> >     scoped_guard(mutex, &priv->modify_lock) {
> >             spin_lock_irqsave(&priv-
> >cmd_receiver_clbk_hdl.clbk_rx_lock, flags);
> >             if (priv->cmd_receiver_clbk_hdl.dev_ctx != dev_ctx ||
> >                 !priv->cmd_receiver_clbk_hdl.rx_msg ||
> >                 !priv->cmd_receiver_clbk_hdl.rx_msg_sz) {
> >                     spin_unlock_irqrestore(&priv-
> >cmd_receiver_clbk_hdl.clbk_rx_lock, flags);
> >                     mutex_unlock(&dev_ctx->fops_lock);
> >                     return -ENODEV;
> >             }
> >             /* Taking snapshot is enough for the one common pre-
> allocated buffer. */
> >             copy_len = min(size, priv->cmd_receiver_clbk_hdl.rx_msg_sz);
> >             memcpy(rx_msg_snap, priv->cmd_receiver_clbk_hdl.rx_msg,
> copy_len);
> >             priv->cmd_receiver_clbk_hdl.rx_msg_sz = 0;
> >             spin_unlock_irqrestore(&priv-
> >cmd_receiver_clbk_hdl.clbk_rx_lock,
> > flags);
> >
> >             /* 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);
>
> [Severity: High]
> Can this sequence read uninitialized stack memory if the user provides a short
> length?
>
> If a user acting as the command receiver calls read() with a short length (e.g.,
> 4 bytes), se_if_fops_read() copies only 4 bytes into the 256-byte stack array
> rx_msg_snap.
>
> It then unconditionally calls fw_api_specific_ops() with this partially
> uninitialized buffer. If the command is ELE_STORAGE_MASTER_EXPORT_REQ,
> fw_api_specific_ops() reads rx_msg->data[1], which now contains garbage
> stack memory. Could this update the expected buffer size with an invalid
> value and break DMA bounds checking?
>
Accepted. Will fix this in v37, by initializing the rx_msg_snap = {};

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260817-imx-se-if-v36-0-
> [email protected]?part=5

NXP Confidential