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

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Aug 17, 2026 at 06:03:01PM +0000, Pankaj Gupta (OSS) wrote:
> > > --- 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.

Why? at lease addr should include payload size?

Frank

> - 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-
> > 45c42847bfd8@oss.nxp.com?part=5
>
> NXP Confidential