RE: [PATCH v33 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
> [ ... ]
> > @@ -128,6 +178,26 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx,
> struct se_clbk_handle *se_clbk
> >  		}
> > +
> > +		/*
> > +		 * A positive wait return normally means firmware delivered a
> > +		 * response. During teardown, se_if_probe_cleanup() forces this
> > +		 * wait to return via complete_all() without any real response.
> > +		 * If that happens the firmware may still be executing and could
> > +		 * DMA into the shared buffer later. Treat it as a failed
> > +		 * transaction and arm the circuit breaker so the shared memory
> > +		 * is quarantined (not freed) instead of being reclaimed while
> > +		 * the enclave might still write to it.
> > +		 */
> > +		if (is_rsp_wait_with_timeout && atomic_read(&priv-
> >going_away)) {
> > +			spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
> > +			se_clbk_hdl->rx_msg = NULL;
> > +			se_mark_fw_busy(dev_ctx);
> > +			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock,
> flags);
> > +			ret = -ENODEV;
> > +			break;
> > +		}
> 
> [Severity: High]
> Does this logic unintentionally intercept legitimate teardown responses?
> 
> During se_if_probe_cleanup(), going_away is set to 1. The teardown path then
> calls cleanup_dev_ctx(), which sends graceful close commands via
> se_close_storage() and se_close_session(). These commands explicitly bypass the
> going_away check in ele_msg_send_rcv() so they can be sent.
> 
> However, when the hardware replies, ele_msg_rcv() checks going_away and
> misidentifies the genuine wakeup as the forced unbind abort, failing the close
> operation and leaking the DMA memory.
> 
Accepted. Will be fixed in V34.

> > 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
> [ ... ]
> > @@ -339,31 +709,116 @@ static void se_if_probe_cleanup(void *plat_dev)
> >  	if (!priv)
> >  		return;
> >
> > +	scoped_guard(mutex, &priv->se_if_cmd_lock) {
> > +		/*
> > +		 * Announce teardown before doing anything else.
> ele_msg_send_rcv()
> > +		 * checks going_away under se_if_cmd_lock and bails out
> instead of
> > +		 * arming a new transaction, and ele_msg_rcv() uses it to tell a
> > +		 * teardown-forced completion apart from a genuine firmware
> response.
> > +		 * This must be set before the complete_all() below so those
> checks
> > +		 * are not dead code.
> > +		 */
> > +		atomic_set(&priv->going_away, 1);
> > +		/*
> > +		 * Wake any ioctl thread blocked in ele_msg_rcv() before
> iterating the
> > +		 * device-context list. That waiter sleeps on this completion
> while
> > +		 * holding its dev_ctx->fops_lock, and cleanup_dev_ctx() below
> also
> > +		 * takes dev_ctx->fops_lock. Completing first lets the waiter
> make
> > +		 * progress and drop fops_lock, avoiding an unbind hang.
> > +		 */
> > +		complete_all(&priv->waiting_rsp_clbk_hdl.done);
> > +	}
> 
> [Severity: Critical]
> Could this lead to a deadlock during unbind?
> 
> When an IOCTL calls ele_msg_send_rcv(), it holds se_if_cmd_lock across the
> blocking wait in ele_msg_rcv(). If the device unbinds concurrently,
> se_if_probe_cleanup() attempts to acquire the exact same se_if_cmd_lock
> before executing complete_all() to wake up the IOCTL thread.
> 
> The unbind thread would block waiting for the lock, while the IOCTL thread
> won't release it until it is woken up.

Accepted. Will be fixed in V34.

> 
> > 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
> [ ... ]
> > +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;
> 
> [Severity: High]
> What happens if userspace sends multiple ELE_SESSION_OPEN_REQ or
> ELE_STORAGE_OPEN_REQ ioctls on the same file descriptor?
> 
> The new handle is directly assigned to dev_ctx->sess_hdl, overwriting any
> previous handle without closing it. Does this allow unprivileged userspace
> programs to exhaust firmware resources by leaking previous handles?
> 
Accepted. In ele_uapi_allowed_fw_cmd(), it is checked. if the dev_ctx exists, it will be not allowed giving error as -EEXISTS.

@@ -38,10 +38,19 @@ int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr
                return -EOPNOTSUPP;
 
        switch (header->command) {
+       case ELE_SESSION_OPEN_REQ:
+               /* Might be cleared as part of tear down. */
+               ret = dev_ctx->sess_hdl ? -EEXIST : 0;
+               break;
        case ELE_SESSION_CLOSE_REQ:
                /* Might be cleared as part of tear down. */
                ret = dev_ctx->sess_hdl ? 0 : -ENXIO;
                break;
+       case ELE_STORAGE_OPEN_REQ:
+               /* Might be cleared as part of tear down. */
+               ret = dev_ctx->strg_hdl ? -EEXIST : 0;
+               break;
+       }
        case ELE_STORAGE_CLOSE_REQ:
                /* Might be cleared as part of tear down. */
                ret = dev_ctx->strg_hdl ? 0 : -ENXIO;

> [Severity: High]
> Is it safe to unconditionally read rx_msg->data[1] here?
> 

[Not an issue]
Yes. It is safe.
Before the control reaches here to execute fw_api_specific_ops(), it is checked that
func se_val_rsp_hdr_n_status() must not return error.

static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
                                            void __user *uarg)
{
        ....

        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 (!err && !rsp_status_err)
                fw_api_specific_ops(dev_ctx, rx_msg);
        ....
}
> The rx_msg buffer is allocated using kzalloc based on the user-provided
> rx_buf_sz in se_ioctl_cmd_snd_rcv_rsp_handler(). If userspace provides an 8-
> byte buffer size and the firmware responds with a truncated payload, would this
> read past the end of the allocation and leak adjacent kernel memory?
>
[Not an issue] 
Since se_if_rx_callback() already clamps the copy to min(rx_msg_sz, exp_rx_msg_sz) & 
Response is validated with successful response, then only data[1] is read.
what must be safely copied already.

> [ ... ]
> > +int se_close_session(struct se_if_priv *priv, u32 session_hdl) {
> > +	struct se_api_msg *tx_msg __free(kfree) = NULL;
> > +	struct se_api_msg *rx_msg __free(kfree) = NULL;
> > +	int ret;
> > +
> > +	if (!priv || !priv->priv_dev_ctx) {
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> 
> [Severity: High]
> Are there concerns with mixing goto-based error handling and scope-based
> cleanup here?
> 
> The cleanup subsystem guidelines advise against using goto labels (like goto exit)
> in the same function as __free annotations, as it can create confusing ownership
> semantics and lead to double-free or resource leak bugs.
> The same pattern is present in se_close_storage(). Would returning directly be
> preferable?
Accepted. Will fix it in v34

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