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

> > 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
> > @@ -246,6 +246,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx
> *dev_ctx, void *tx_msg,
> >  	guard(mutex)(&priv->se_if_cmd_lock);
> >
> > +	/*
> > +	 * Teardown has begun: do not arm a new transaction. A thread that
> was
> > +	 * blocked on se_if_cmd_lock while se_if_probe_cleanup() ran its
> > +	 * complete_all() would otherwise miss that wake-up, re-arm the
> > +	 * completion below and wait the full timeout, while unbind blocks
> > +	 * indefinitely on this thread's fops_lock. Bail out instead.
> > +	 */
> > +	if (atomic_read(&priv->going_away))
> > +		return -ENODEV;
> > +
> >  	if (atomic_read(&priv->fw_busy)) {
> >  		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n",
> dev_ctx->devname);
> >  		return -EBUSY;
> >  	}
> > +
> >  	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
> 
> [Severity: High]
> Is there a TOCTOU race condition here with se_if_probe_cleanup()? If
> se_if_probe_cleanup() sets going_away = 1 and calls complete_all() without
> acquiring se_if_cmd_lock, could it execute complete_all() right after this
> atomic_read() check but before reinit_completion() is called? Would this erase
> the forced teardown wakeup and cause a permanent deadlock?
> 
> [ ... ]

[Rejected]
TOCTOU race between ele_msg_send_rcv() checking going_away
and se_if_probe_cleanup() forcing wakeup (lost wakeup / deadlock):
No change is needed.
 
The ordering is already safe: se_if_probe_cleanup() sets
going_away before complete_all() (with an explicit ordering comment),
ele_msg_send_rcv() reads going_away under se_if_cmd_lock and bails with
-ENODEV before reinit_completion(), and ele_msg_rcv() re-checks
going_away after the wake and returns -ENODEV so a teardown-forced
completion is never mistaken for a real response. No code change.

> 
> > @@ -279,6 +284,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx
> *dev_ctx, void *tx_msg,
> >  	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;
> >  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this explicit override of a
> successful hardware interaction with -ERESTARTSYS break Power Management
> transitions? If a signal is pending during system suspend, could this cause
> internal kernel hardware operations like se_save_imem_state() to fail
> spuriously and permanently lose the enclave IMEM state required for proper
> resume?
> 
> [ ... ]
Accepted, will be fixed in next respin.

@@ -100,22 +100,10 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
        if (se_clbk_hdl == &priv->waiting_rsp_clbk_hdl) {
                is_rsp_wait_with_timeout = true;
                deadline_jiffies = jiffies + remaining_jiffies;
+               if (se_clbk_hdl->dev_ctx == priv->priv_dev_ctx)
+                       wait_uninterruptible = true;
        }
> 
> > @@ -323,9 +395,13 @@ void se_if_rx_callback(struct mbox_client *mbox_cl,
> void *msg)
> >  		/*
> >  		 * Clamp the copy length to the pre-allocated receiver buffer
> (MAX_NVM_MSG_LEN).
> >  		 */
> >  		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);
> 
> [Severity: High]
> Does this unconditionally overwrite the shared rx_msg buffer and call
> complete() upon receiving a firmware command? If the firmware issues a
> second command before the userspace daemon executes a read() to consume
> the first one, could the first command's payload be completely overwritten,
> resulting in a spurious -ENODEV failure?
> 
Rejected.

Firmware commands to the command receiver can overwrite an
unprocessed command (se_if_rx_callback() cmd_tag branch memcpy+complete
unconditionally): NO-CHANGE.
The NVM command-receiver protocol is half-duplex:
the firmware issues one storage/NVM command at a time and
waits for the userspace daemon's response before issuing the next, so
there is no in-flight command to overwrite. The receiver uses a
dedicated long-lived buffer and clbk_rx_lock already serialises the
callback against se_if_fops_read().

No code change.

> > 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
> > @@ -284,11 +292,308 @@ static int get_se_soc_info(struct se_if_priv
> > *priv, const struct se_soc_info *se
> > +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);
> 
> [Severity: Critical]
> If priv->mem_pool is NULL, the mem_pool_buf_list remains zero-initialized.
> Will se_cleanup_mem_pool_buf() later unconditionally traverse this
> uninitialized list head via list_for_each_entry_safe() and cause a kernel panic
> when the file descriptor is closed?
> 
> [ ... ]
> 
Accepted.
 -       se_cleanup_mem_pool_buf(dev_ctx, reclaim);
+       if (priv->mem_pool)
+               se_cleanup_mem_pool_buf(dev_ctx, reclaim);

> > +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx
> *dev_ctx,
> > +					    void __user *uarg)
> > +{
> [ ... ]
> > +	struct se_api_msg *tx_msg __free(kfree) =
> > +
> 	memdup_user(u64_to_user_ptr(cmd_snd_rcv_rsp_info.tx_buf),
> > +			    cmd_snd_rcv_rsp_info.tx_buf_sz);
> > +	if (IS_ERR(tx_msg)) {
> > +		err = PTR_ERR(tx_msg);
> > +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg,
> &cmd_snd_rcv_rsp_info);
> > +		return err;
> > +	}
> 
> [Severity: Critical]
> Does this allow user-space to construct raw command payloads and embed
> unvalidated DMA addresses? Since these addresses are not validated to
> ensure they point to memory allocated via SE_IOCTL_SETUP_IOBUF before
> calling ele_msg_send(), could a malicious application embed spoofed physical
> addresses (like kernel page tables) and instruct the hardware to access them?
> 
> [ ... ]
Not an issue.
- [Critical, new] Userspace can construct raw command payloads and embed
  unvalidated DMA addresses (confused-deputy): NO-CHANGE. 

Passing physical/DMA addresses inside the message body is inherent to the
firmware ABI this interface exposes, and is constrained by
se_chk_tx_msg_hdr() -> ele_uapi_allowed_base_cmd()/
ele_uapi_allowed_fw_cmd(), which restrict the command set to a
vetted allow-list (power-management, reset, BBSM, RNG init, FW
re-init, CAAM resource release and SE internal memory management are
all blocked). The enclave firmware is the trust boundary that
validates addresses against the caller's provisioned resources;
per-command address bounds-checking in the kernel would duplicate that
policy without owning the authoritative resource map. No code change.

> 
> > +	struct se_api_msg *rx_msg __free(kfree) =
> > +		kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL);
> > +	if (!rx_msg) {
> > +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg,
> &cmd_snd_rcv_rsp_info);
> > +		return -ENOMEM;
> > +	}
> 
> [Severity: High]
> Can this lead to a slab-out-of-bounds read in se_val_rsp_hdr_n_status()?
> If rx_buf_sz is small (e.g., 4 bytes) but the firmware returns a header size
> larger than SE_MU_HDR_WORD_SZ for debug commands, will
> check_hdr_exception_for_sz() bypass the mismatch check and cause
> se_val_rsp_hdr_n_status() to read past the 4-byte allocated rx_msg buffer?
> 
Accepted. Will be fixed in next re-spin.
-       if (header->size > SE_MU_HDR_WORD_SZ) {
+       if (header->size > SE_MU_HDR_WORD_SZ && (sz >> 2) > SE_MU_HDR_WORD_SZ) {
                status = RES_STATUS(msg->data[0]);
                if (status != priv->if_defs->success_tag) {

> [ ... ]
> 
> > +	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) {
> > +		err = se_dev_ctx_cpy_out_data(dev_ctx);
> > +		if (err < 0) {
> > +			se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg,
> &cmd_snd_rcv_rsp_info);
> > +			return err;
> > +		}
> > +	}
> 
> [Severity: High]
> Does this mask the failure when the firmware command fails? If
> se_val_rsp_hdr_n_status() sets a negative error code in rsp_status_err, it skips
> se_dev_ctx_cpy_out_data(), but the positive message size from
> ele_msg_send_rcv() stored in err isn't updated. Could this cause the ioctl to
> return a positive value to userspace, falsely indicating success?
> 
Accepted. 
diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
index a00cf75b0d8c..a3a70cf45d80 100644
--- a/drivers/firmware/imx/se_ctrl.c
+++ b/drivers/firmware/imx/se_ctrl.c
@@ -1370,7 +1370,8 @@ static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
 
                return err;
        }
-
+       /* updating the actual received buffer size. */
+       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");

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