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

> > @@ -617,6 +617,6 @@
> >  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]
> If there are multiple secure enclave interfaces, do they share this global static
> array ele_storage_master_export_addr_fields? If two instances execute an
> export concurrently, could they race to overwrite this buf_size?
> If one instance's validation uses the other's size, might this allow a smaller
> bounds check to pass while the firmware executes a larger DMA write, resulting
> in out-of-bounds memory corruption?

Accepted. It will be fixed in v39

>
> > diff --git a/drivers/firmware/imx/ele_fw_api.c
> > b/drivers/firmware/imx/ele_fw_api.c
> > --- a/drivers/firmware/imx/ele_fw_api.c
> > +++ b/drivers/firmware/imx/ele_fw_api.c
> > @@ -152,7 +152,7 @@
> >     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 ...
> >              */
> >             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;
> >             if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> >                     ret = -EINVAL;
> >             break;
> >
> > [ ... ]
> >
> > @@ -252,7 +252,7 @@
> >     case ELE_STORAGE_OPEN_REQ: {
> >             int rc;
> >
> >             /*
> > ...
> >              */
> >             dev_ctx->strg_hdl = rx_msg->data[1];
> >
> >             rc = set_dev_ctx_as_command_receiver(dev_ctx, false);
> >             if (rc)
> >                     dev_err(priv->dev,
> >                             "Failed to register %s as CMD-Receiver: %d\n",
> >                             dev_ctx->devname, rc);
> >             break;
> >     }
>
> [Severity: High]
> Since modify_lock is dropped before sending the command to firmware in the
> pre-check, could two processes concurrently pass this check and both receive
> storage handles from the firmware? If the second process fails registration in
> set_dev_ctx_as_command_receiver() but the error is just logged and discarded,
> wouldn't it successfully return the handle to userspace? Could this result in the
> firmware incorrectly routing the second process's NVM callbacks to the first
> registered process?

Not an issue.
Userspace call the IOCTL SE_IOCTL_ENABLE_CMD_RCV_STATUS,
to know the execution status of function set_dev_ctx_as_command_receiver().
If it fails, it will initiate ELE_STORAGE_CLOSE_REQ, immediately.

>
> > diff --git a/drivers/firmware/imx/ele_common.c
> > b/drivers/firmware/imx/ele_common.c
> > --- a/drivers/firmware/imx/ele_common.c
> > +++ b/drivers/firmware/imx/ele_common.c
> > @@ -140,4 +140,4 @@
> >             if (f->size_idx != SE_CMD_ADDR_NO_SIZE) { ...
> >             } 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]
> If a malicious command receiver skips calling read() and directly issues a
> write() to send an export response, does buf_size remain at its default value of
> zero? If buf_size is zero, the check else if (f->buf_size) evaluates to false,
> completely bypassing the end-bounds check. Could an attacker exploit this by
> specifying an address at the very end of the shared memory window, causing the
> firmware to DMA the large export payload out of bounds?

Accepted. Add a check for last received command id verification se_cmd_receiver_allowed_rsp per "priv".
Will fix in v39.
>
> > [ ... ]
> >
> > @@ -367,4 +367,4 @@
> >             if (ret == 0) {
> > ...
> >                     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 completion_done() the correct check to use in this timeout path? If device
> teardown runs concurrently and invokes complete_all(), won't
> completion_done() return true? This would cause the code to skip calling
> se_mark_fw_busy() and proceed to free the shared DMA buffer while the
> firmware might still be processing the command, potentially leading to a DMA-
> after-free memory corruption. Should this check use !se_clbk_hdl->rx_delivered
> instead, similar to the other paths?
>
Accepted. Will fix in v39.

> > diff --git a/drivers/firmware/imx/se_ctrl.c
> > b/drivers/firmware/imx/se_ctrl.c
> > --- a/drivers/firmware/imx/se_ctrl.c
> > +++ b/drivers/firmware/imx/se_ctrl.c
> > @@ -1885,5 +1885,5 @@
> >  static int se_if_fops_open(struct inode *nd, struct file *fp)  {
> >     struct miscdevice *miscdev = fp->private_data;
> >     struct se_if_open_gate *gate;
> > ...
> >     gate = container_of(miscdev, struct se_if_open_gate, miscdev);
> >
> >     if (!se_if_open_gate_get(gate))
> >             return -ENODEV;
>
> [Severity: High]
> Could the gate pointer be freed concurrently before se_if_open_gate_get() is
> called? When userspace calls open(), the VFS resolves the miscdev and drops
> misc_mtx. If device unbind executes at this exact moment,
> misc_deregister() can complete and the teardown path will free the gate
> structure. When the open thread resumes, won't it dereference a freed gate
> structure here, resulting in a use-after-free?

Not an issue.
Dereference gate before se_if_open_gate_get(): this is safe because
misc_open() in drivers/char/misc.c holds misc_mtx for the entire
duration of calling our .open callback, and misc_deregister() also
takes misc_mtx. Therefore teardown cannot call misc_deregister() - and
subsequently drop the device-owned kref that would free gate - while
we are executing here. Once se_if_open_gate_get() succeeds we own a
kref ourselves, so gate is safe to use after misc_mtx is released.

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

NXP Confidential