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

>
> > +           if (f->size_idx == SE_CMD_RCVR_ADDR_VAR_SIZE) {
> > +                   struct cmd_rcvr_data_info *crcvr_info =
> > +                                                   &dev_ctx->priv-
> >crcvr_info;
> > +                   /*
> > +                    * Export-response buffer: the size was supplied by FW
> > +                    * in the preceding export command and stored per SE
> > +                    * interface in cmd_rcvr_var_size.
> > +                    */
> > +                   if ((u64)crcvr_info->cmd_rcvr_var_size > end - addr)
> > +                           return -EACCES;
> > +           } else if (f->size_idx != SE_CMD_ADDR_NO_SIZE) {
> > +                   u64 len;
> > +
> > +                   if (f->size_idx >= payload_words)
> > +                           return -EINVAL;
> > +
> > +                   /* size_mask == 0 with a valid size_idx is a descriptor
> bug. */
> > +                   if (!f->size_mask)
> > +                           return -EINVAL;
> > +
> > +                   /*
> > +                    * Widen to u64 before shifting: size_shift is u8 and
> > +                    * shifting a u32 by >= 32 is undefined behaviour.
> > +                    */
> > +                   len = ((u64)msg->data[f->size_idx] >> f->size_shift) & f-
> >size_mask;
> > +                   if (len > end - addr)
> > +                           return -EACCES;
> > +           } else if (f->buf_size) {
> > +                   /* buf_size: literal byte count (FW-defined constant). */
> > +                   if ((u64)f->buf_size > end - addr)
> > +                           return -EACCES;
> > +           }
>
> [Severity: High]
> If size_idx is SE_CMD_ADDR_NO_SIZE and buf_size is 0, the logic only checks
> that the start address is within bounds (addr < base || addr >= end) earlier in the
> function.
>
> Could a user supply a DMA address at the very end of the shared memory
> window (e.g., end - 1), causing the firmware to read the multi-byte container
> header out of bounds into adjacent physical memory?
>
> >     }
> >
> >     return 0;
> >  }
Not a Bug.
For two API(s):  ELE_OEM_AUTH_CONTAINER_REQ & ELE_KEYSTORE_REPROV_ENABLE_REQ, buf_size = 0.

Payload size is encoded inside the OEM Authentication Container & signed message header itself.
For ELE_OEM_AUTH_CONTAINER_REQ & ELE_KEYSTORE_REPROV_ENABLE_REQ, the API only passes the start address of the container to firmware.

The kernel cannot validate the full container size because determining the actual payload size requires parsing the container header, and that header is interpreted by firmware using information and keys that are not available outside the secure firmware.

Therefore, for this command, only the container start address is validated against the shared-memory region.
The firmware then reads and processes the container contents as part of the authentication flow.

Also note that this is a read-only input buffer.
Firmware does not perform any write-back through this address.
A malicious caller can only provide invalid input data for authentication, which would cause the authentication operation to fail.
It does not provide a mechanism for arbitrary memory corruption or privilege escalation from the kernel side.

The trust model for this command assumes that the firmware is responsible for validating and parsing the container format after receiving the start address. Since the container length is not available to the kernel, enforcing an addr + size check in the driver is not possible for this request type. Any bounds validation based on the authenticated container structure must therefore be performed by the firmware itself.

>
> [ ... ]
>
> > @@ -468,76 +468,86 @@ int ele_msg_send_rcv(struct se_if_device_ctx
> *dev_ctx, void *tx_msg,
> >                  int tx_msg_sz, void *rx_msg, int exp_rx_msg_sz)  {
>
> [ ... ]
>
> >     err = ele_msg_rcv(dev_ctx, &priv->waiting_rsp_clbk_hdl);
> >
> >     if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
> >             /*
> >              * Signal was deferred until the FW/kernel protocol
> resynchronized.
> >              * On success report -ERESTARTSYS for the interrupted wait; the
> >              * command is not re-sent. Keep real errors like -ETIMEDOUT.
> >              */
> >             if (err > 0)
> >                     err = -ERESTARTSYS;
>
> [Severity: High]
> Does this sequence result in silent data corruption and handle leaks?
>
> If a signal interrupts the wait in ele_msg_send_rcv() and the wait continues
> killably, a successful firmware response (err > 0) gets overridden to -
> ERESTARTSYS.
>
> The ioctl handler (se_ioctl_cmd_snd_rcv_rsp_handler) then converts this to -
> EINTR and returns without copying the successful response payload (rx_msg)
> back to userspace.
>
> Will userspace subsequently retry the operation with zeroed shared memory
> buffers, leading to dropped response data or leaked firmware handles?
>
> >             priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> >             dev_dbg(priv->dev, "%s: Err[0x%x]:Interrupted by signal.",
> >                     dev_ctx->devname, err);
> >     }
> >
> >     ele_msg_send_rcv_cleanup(priv);
> >
> >     return err;
> >  }
>
Accepted will fix in v40.
Will initiate the cleanup in case of SESSION_OPEN/STORAGE_OPEN are interrupted.
A successfully-completed SESSION_OPEN or STORAGE_OPEN interrupted by a signal left the handle live in FW but untracked in the kernel,
so cleanup_dev_ctx() would never close it. The `is_cmd_interrupted` path correctly closes and clears the handle.

> [ ... ]
>
> > @@ -552,74 +552,74 @@ void se_if_rx_callback(struct mbox_client
> > *mbox_cl, void *msg)  {
>
> [ ... ]
>
> >     /* Incoming command: wake up the receiver if any. */
> >     if (header->tag == priv->if_defs->cmd_tag) {
> >             se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
> >             spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>
> [ ... ]
>
> >     } else if (header->tag == priv->if_defs->rsp_tag) {
> >             bool exception_for_sz_mismatch =
> check_hdr_exception_for_sz(priv, header);
> >             u32 exp_rx_msg_sz = 0;
> >
> >             /*
> >              * rx_msg and rx_msg_sz are owned by the sender under
> clbk_rx_lock.
> >              * Read both under the lock: drop a late response instead of
> copying
> >              * into freed memory, and avoid a stale size. A late response also
> >              * closes the firmware-busy circuit breaker.
> >              */
> >             se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
> >             spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>
> [Severity: High]
> Can asynchronous writes corrupt synchronous ioctl responses?
>
> The write() handler allows sending cmd_tag messages without waiting for a
> response.
> When the firmware replies with a rsp_tag, se_if_rx_callback() blindly
> assigns it to the single synchronous ioctl waiter (priv->waiting_rsp_clbk_hdl).
>
> If the command receiver writes a command while another process is blocked in
> an ioctl, will the ioctl process wake up with the asynchronous response data,
> corrupting its state?
>
> >             if (!se_clbk_hdl->rx_msg) {
> >                     if (atomic_read(&priv->fw_busy))
> >                             schedule_fw_busy_work = true;
> >                     spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock,
> flags);
> >
> >                     if (schedule_fw_busy_work)
> >                             schedule_work(&priv->fw_busy_work);
> >
> >                     dev_info(dev, "ELE responded (late), recovery FW
> available.");
> >                     return;
> >             }
>
The original `se_chk_tx_msg_hdr()` accepted both `cmd_tag` and `rsp_tag` on the write() path.
Splitting into `se_chk_tx_rsp_msg_hdr()` (rsp_tag only) and `se_chk_tx_cmd_msg_hdr()` (cmd_tag only) will be the correct structural fix.
Accepted will fix in v40.

> > 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
> > @@ -0,0 +1,382 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2026 NXP
> > + */
> > +
> > +#include "se_ctrl.h"
> > +#include "ele_common.h"
> > +#include "ele_fw_api.h"
> > +
> > +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 bypass the handle ownership checks for the command receiver?
>
> In ele_uapi_allowed_fw_cmd(), if the caller is the command receiver, the request
> is delegated here and unconditionally allowed.
>
> This seems to bypass the main switch statement where dev_ctx->sess_hdl and
> dev_ctx->strg_hdl are verified against msg->data[0]. Could a malicious command
> receiver use this to close arbitrary handles belonging to other processes?

Without the ownership check, any process that happened to register as command receiver could write `ELE_SESSION_CLOSE_REQ` with an arbitrary handle value in `data[0]` and close another process's session.
Accepted will fix it on v40.

>
> > +   case ELE_STORAGE_MASTER_IMPORT_REQ:
> > +           const struct se_cmd_addr_field *fields;
> > +           size_t count;
>
> [Severity: Low]
> Is this variable declaration right after the case label valid C11 syntax?
> Standard C requires a statement after a label, and a declaration is not considered
> a statement. Does this cause a build failure?

No build failure reported.
Accepted. Will add curly braces in v40.

>
> > +
> > +           fields = ele_fw_cmd_addr_fields(cmd, &count);
> > +           return se_val_cmd_addrs(dev_ctx, msg, tx_msg_sz, fields,
> count);
> > +   default:
> > +           return -EOPNOTSUPP;
> > +   }
> > +}
>
> [ ... ]
>
> > +int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct
> se_msg_hdr *header,
> > +                       u32 tx_msg_sz, u32 rx_msg_sz)
> > +{
>
> [ ... ]
>
> > +   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]
> Can concurrent callers race between this pre-check and the actual receiver
> registration?
>
> The modify_lock is dropped immediately after checking if a receiver is already
> registered. Multiple callers could pass this check, send their requests to firmware,
> and receive valid storage handles.
>
> > +           if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> > +                   ret = -EINVAL;
> > +           break;
>

Not an issue.
FW only allow one active storage handle at a time.
Even if the second wins the race, FW will send failure response, as existing active storage handle exits.
FW's single-handle enforcement is the authoritative gate. A kernel pre-check race doesn't matter because a second FW call gets a failure response; no second valid handle is issued.

> [ ... ]
>
> > +void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct
> > +se_api_msg *rx_msg) {
> > +   struct se_msg_hdr *header = &rx_msg->header;
> > +   struct se_if_priv *priv = dev_ctx->priv;
> > +
> > +   switch (header->command) {
> > +   case ELE_SESSION_OPEN_REQ:
> > +           dev_ctx->sess_hdl = rx_msg->data[1];
> > +           break;
> > +   case ELE_SESSION_CLOSE_REQ:
> > +           dev_ctx->sess_hdl = 0;
> > +           break;
> > +   case ELE_STORAGE_OPEN_REQ: {
> > +           int rc;
> > +
> > +           /*
> > +            * 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 on the next close(), regardless
> > +            * of whether registration succeeded.
> > +            */
> > +           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]
> If the set_dev_ctx_as_command_receiver() call fails due to the race mentioned
> above, the error is merely logged and ignored.
>
> Does this leave the second caller with a valid storage handle, while all NVM
> callbacks are misrouted to the first caller? Should this failure path be handled
> more robustly?
>
No. As mentioned above only one active storage handle can exists.
If the set_dev_ctx_as_command_receiver() fails, the user space will come to know
When it send the IOCTL: SE_IOCTL_ENABLE_CMD_RCV_STATUS.
It will also fail with same reason as set_dev_ctx_as_command_receiver() failed here.
Which will initiate the cleanup of storage handle followed by session handle.
The premise of the scenario (second caller holds a valid strg_hdl while a different dev_ctx is the registered receiver) cannot occur precisely as mentioned above.
FW rejects the second STORAGE_OPEN, so `fw_api_specific_ops()` never stores a non-zero `strg_hdl` for the second caller.
The `set_dev_ctx_as_command_receiver()` error path logging is correct and the SE_IOCTL_ENABLE_CMD_RCV_STATUS mechanism gives userspace the right signal.

> > diff --git a/drivers/firmware/imx/ele_msg_addr_field.c
> > b/drivers/firmware/imx/ele_msg_addr_field.c
> > --- a/drivers/firmware/imx/ele_msg_addr_field.c
> > +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> > @@ -0,0 +1,635 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2026 NXP
> > + */
>
> [ ... ]
>
> > +static const struct se_cmd_addr_field ele_oem_auth_cntr_addr_fields[] = {
> > +   /*
> > +    * Container Header address: a 64-bit physical address split across two
> > +    * words. data[0] holds the 32-bit MSB and data[1] holds the 32-bit LSB
> > +    * (ELE API spec Table 27, word size = 0x3, so the command is header +
> > +    * MSB + LSB only). The message carries no length word for this buffer;
> > +    * the container size is variable and not communicated in the MU
> payload,
> > +    * and no static firmware-defined maximum is specified. Because this is a
> > +    * read-only input buffer (the ELE ROM/FW copies the container header
> > +    * into its internal memory for authentication and does not write back
> > +    * through this address), enforcing only the start-address range check is
> > +    * acceptable: a rogue caller can at most cause firmware to read within
> > +    * the shared-memory window, which is memory the caller already
> owns.
> > +    * Output buffers must be fully bounded; input-only buffers are safe with
> > +    * addr-only checks.
> > +    */
> > +   { .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx =
> SE_CMD_ADDR_ALWAYS,
> > +     .size_idx = SE_CMD_ADDR_NO_SIZE },                    /*
> container_hdr_addr */
>
> [Severity: High]
> Does omitting length constraints for the container header allow out-of-bounds
> reads? As noted above, since size_idx is SE_CMD_ADDR_NO_SIZE and buf_size
> defaults to 0, no upper bound check is performed when the command is
> validated in se_val_cmd_addrs().

As explained earlier too. It is not an issue:
Payload size is encoded inside the OEM Authentication Container itself.

This is a read-only input buffer.
Firmware does not perform any write-back through this address.
A malicious caller can only provide invalid input data for authentication, which would cause the authentication operation to fail.
It does not provide a mechanism for arbitrary memory corruption or privilege escalation from the kernel side.

The trust model for this command assumes that the firmware is responsible for validating and parsing the container format after receiving the start address. Since the container length is not available to the kernel, enforcing an addr + size check in the driver is not possible for this request type. Any bounds validation based on the authenticated container structure must therefore be performed by the firmware itself.

>
> > +};
>
> > 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
>
> [ ... ]
>
> > @@ -1195,30 +1195,30 @@ int set_dev_ctx_as_command_receiver(struct
> > se_if_device_ctx *dev_ctx, bool is_io  {
> >     struct se_if_priv *priv = dev_ctx->priv;
> >     struct se_api_msg *new_rx_msg = NULL;
> >     struct se_clbk_handle *se_clbk_hdl;
> >     unsigned long flags;
> >
> >     se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
> >     guard(mutex)(&priv->modify_lock);
> >     if (se_clbk_hdl->dev_ctx == dev_ctx)
> >             return 0;
> >
> >     if (se_clbk_hdl->dev_ctx)
> >             return -EBUSY;
> >
> >     if (!dev_ctx->strg_hdl)
> >             return -EINVAL;
> >
> >     if (is_ioctl)
> >             return -ENOMEM;
>
> [Severity: Medium]
> Does this unconditionally reject the SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl?
> When the ioctl calls this function with is_ioctl = true, it immediately returns -
> ENOMEM before any allocations happen. Is this intentional, or does it break the
> ioctl completely?
>

Not an issue. As IOCTL is for status only. If the enquiry made by dev_ctx is same as the registered cmd-receiver,
Then it return success.
If not matching mean dev_ctx registration as command receiver failed due to a reason.
That correct/exact reason for the failure got returned.
 `is_ioctl=true` is a status-query path, not an allocation path.
The early returns (-EBUSY, -EINVAL, -ENOMEM) are the exact reason the earlier `set_dev_ctx_as_command_receiver(false)` call failed, forwarded transparently to userspace.

> [ ... ]
>
> > @@ -1505,30 +1505,30 @@ static int
> se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> >                                         void __user *uarg)
> >  {
>
> [ ... ]
>
> >             /*
> >              * -ERESTARTSYS here means the wait was interrupted by a
> signal
> >              * after the command had already been handed to - and
> executed
> >              * by - the firmware, with its response delivered into rx_msg
> >              * (ele_msg_send_rcv() converts only a positive, i.e. successfully
> >              * received, result to -ERESTARTSYS). If that response carried a
> >              * freshly allocated session/storage handle, record it now via
> >              * fw_api_specific_ops(): the handle is already live in firmware,
> >              * so leaving it untracked would stop cleanup_dev_ctx() from
> ever
> >              * closing it and leak the firmware resource. Validate the
> >              * delivered response first, using its own declared length
> bounded
> >              * by the caller's buffer, so a truncated or malformed reply is
> >              * not acted upon.
> >              */
> >             if (err == -ERESTARTSYS) {
> >                     u32 rsp_sz = rx_msg->header.size << 2;
> >
> >                     if (rsp_sz && rsp_sz <=
> cmd_snd_rcv_rsp_info.rx_buf_sz &&
> >                         !se_val_rsp_hdr_n_status(priv, rx_msg,
> >                                                  tx_msg->header.command,
> rsp_sz,
> >                                                  tx_msg->header.ver ==
> >                                                  priv->if_defs-
> >base_api_ver)) {
> >                             se_dev_ctx_cpy_out_data(dev_ctx);
> >                             fw_api_specific_ops(dev_ctx, rx_msg);
> >                     }
> >                     /*
> >                      * Returning -ERESTARTSYS would let the VFS
> transparently restart
> >                      * the ioctl, which would re-run the command with the
> just
> >                      * cleaned-up (zeroed) shared input buffers. Report -
> EINTR instead
> >                      * so the syscall is not auto-restarted; userspace enters
> its
> >                      * signal handler and can decide whether to reissue the
> command.
> >                      */
> >                     err = -EINTR;
> >             }
>
> [Severity: High]
> See the note about silent data corruption and handle leaks above; this is where
> the successful ioctl returns -EINTR without copying the response to the user.

Agreed, a successfully-completed SESSION_OPEN or STORAGE_OPEN interrupted by a signal left the handle live in FW but untracked in the kernel, so cleanup_dev_ctx() would never close it.
The `is_cmd_interrupted` path correctly closes and clears the handle.
As already mentioned, it will get fixed.

> [ ... ]
>
> > @@ -1807,30 +1807,30 @@ static ssize_t se_if_fops_write(struct file *fp,
> const char __user *buf,
> >                             size_t size, loff_t *ppos)
> >  {
> >     struct se_if_device_ctx *dev_ctx = fp->private_data;
> >     struct se_if_priv *priv;
> >     int err;
> >
> >     scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &dev_ctx-
> >fops_lock) {
> >             if (dev_ctx->cleanup_done)
> >                     return -ENODEV;
> >
> >             priv = dev_ctx->priv;
> >
> >             dev_dbg(priv->dev, "%s: write from buf (%p)%zu, ppos=%lld.",
> dev_ctx->devname,
> >                     buf, size, ((ppos) ? *ppos : 0));
> >
> >             if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
> >                     se_dev_ctx_shared_mem_cleanup(dev_ctx);
> >                     return -EINVAL;
> >             }
> >
> >             if (size < SE_MU_HDR_SZ || size >
> MAX_ALLOWED_TX_MSG_SZ) {
> >                     dev_err(priv->dev, "%s: User buffer too
> small/large(%zu < %d)",
> >                             dev_ctx->devname, size,
> >                             size < SE_MU_HDR_SZ ? SE_MU_HDR_SZ :
> >
>       MAX_ALLOWED_TX_MSG_SZ);
> >                     return -ENOSPC;
> >             }
> >
> >             struct se_api_msg *tx_msg __free(kfree) = memdup_user(buf,
> size);
> >             if (IS_ERR(tx_msg))
> >                     return PTR_ERR(tx_msg);
> >
> >             err = se_chk_tx_msg_hdr(dev_ctx, &tx_msg->header, size, 0);
> >             if (err)
> >                     return err;
> >
> >             print_hex_dump_debug("from user ", DUMP_PREFIX_OFFSET,
> 4, 4,
> >                                  tx_msg, size, false);
> >
> >             err = ele_msg_send(dev_ctx, tx_msg, size);
> >
> >             return err;
>
> [Severity: High]
> See the note about asynchronous writes corrupting synchronous ioctl responses
> above. This is the write handler that sends the command that generates the
> conflicting response.
>

Accepted .
After the fix in 3rd disposition from the top, write() can only submit rsp_tag messages.
`ELE_SESSION_CLOSE_REQ` and `ELE_STORAGE_CLOSE_REQ` (which previously were the only cmd_tag paths)
 do not generate asynchronous FW responses, so they cannot inject a stray rsp_tag into `priv->waiting_rsp_clbk_hdl`.
The concern is fully neutralised by above fix.

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

NXP Confidential