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

quoted

@@ -121,9 +121,9 @@ int ele_get_info(struct se_if_priv *priv, struct

ele_dev_info *s_info)

quoted

    get_info_len = ELE_GET_INFO_BUFF_SZ;
    if (priv->mem_pool) {
            ret = se_get_mem_pool_buf(priv->priv_dev_ctx,

&get_info_data,

quoted

                                      &get_info_addr, get_info_len);

[Severity: High]
Could this cause a system deadlock during PM suspend?

The caller ele_get_info allocates its message buffers using GFP_NOIO
specifically to prevent direct reclaim from triggering I/O to a suspended
storage device. However, it relies on se_get_mem_pool_buf here, which
does a standard kernel allocation.

Accepted. Will be fixed. Allocation with GFP_NOIO  in v49.
- se_get_mem_pool_buf() used kzalloc_obj() (implicitly GFP_KERNEL)
  for the b_desc descriptor. ele_get_info() is called during PM
  suspend via se_save/restore_imem_state(); at that point direct
  reclaim can deadlock against a suspended storage device.

- Fix: add gfp_t gfp parameter to se_get_mem_pool_buf(); pass
   GFP_NOIO from ele_get_info().

- Code: se_ctrl.c: se_get_mem_pool_buf() kzalloc_obj() -> kzalloc(gfp);
   se_ctrl.h: declaration updated with gfp_t gfp parameter;
   ele_base_msg.c: ele_get_info() passes GFP_NOIO.

[ ... ]

quoted

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

[ ... ]

quoted

@@ -342,6 +342,7 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx,

struct se_clbk_handle *se_clbk

quoted

                            if (se_clbk_hdl->rx_msg) {
                                    /*
                                     * The enclave may still DMA into this
                                     * buffer (either a normal timeout or
                                     * a teardown complete_all()

wakeup).

quoted

                                     * Quarantine the buffer and arm the
                                     * circuit breaker unconditionally.
                                     */
                                    se_clbk_hdl->rx_msg = NULL;
                                    se_mark_fw_busy(dev_ctx);
                            }
                            spin_unlock_irqrestore(&se_clbk_hdl-
clbk_rx_lock, flags);
                    }
                    break;

[Severity: High]
Does this instantly abort the wait and arm the circuit breaker if a fatal signal
like SIGKILL is pending?

By breaking out early, it immediately marks the firmware as busy.

Yes. Code comment is added:
* WARNING: if the killed command was a resource-allocating
* operation (session open, storage open), the firmware will
* complete it and send back a handle ID in the response. That
* response is dropped here (rx_msg already NULL). The handle
* is never recorded in dev_ctx, so se_clear_fw_busy() cannot
* close it. The firmware resource leaks permanently and Linux
* and firmware state diverge. A board reset is required to
* recover. This is an inherent limitation of the request-
* response protocol: there is no mechanism to query or
* replay a dropped firmware response.

[ ... ]

quoted

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

[ ... ]

quoted

@@ -406,6 +406,12 @@ static void cleanup_se_shared_mem(struct

se_if_device_ctx *dev_ctx, bool recla

quoted

    if (free_dma_buf) {
            dma_free_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
                              se_shared_mem_mgmt-
non_secure_mem.ptr,
                              se_shared_mem_mgmt-
non_secure_mem.dma_addr);
    }

    /*
     * Drop the host-side tracking unconditionally. On the reclaim path

the

quoted

     * buffer has been freed. On the deliberate-leak path the buffer is
     * abandoned on purpose, so clearing the pointer here guarantees a

later

quoted

     * cleanup pass (e.g. se_if_priv_release()) cannot double-free it.
     */
    se_shared_mem_mgmt->non_secure_mem.ptr = NULL;
    se_shared_mem_mgmt->non_secure_mem.dma_addr = 0;
    se_shared_mem_mgmt->non_secure_mem.size = 0;

[Severity: High]
Are we unconditionally destroying the DMA buffer state here?

Even when reclaim is false, the size and pointers are zeroed out.
If this is called for priv_dev_ctx (which has no open/close cycle to reallocate
it), the persistent DMA buffer becomes permanently unusable.

Concern is valid. This will be fixed by the caller, as stated below.
Will not be called for priv_dev_ctx in se_clear_fw_busy(), as it is already called as part of unbind.

[ ... ]

quoted

@@ -1149,6 +1149,7 @@ static void se_clear_fw_busy(struct se_if_priv

*priv)

quoted

                    /*
                     * Late response arrived after going_away is clear.
                     * fw_busy has already been cleared atomically

above;

quoted

                     * reclaim the shared-memory slot now. For

priv_dev_ctx

quoted

                     * this is the only reclaim site (no close() path). For
                     * userspace contexts cleanup_se_shared_mem() is a
                     * safe no-op if the close() path already reset pos.
                     */
                    cleanup_se_shared_mem(dev_ctx, true);

[Severity: High]
Can this permanently break internal firmware communications for
priv_dev_ctx?

If an internal command times out, priv_dev_ctx is set as the busy context.
When a late response arrives, se_clear_fw_busy runs and calls
cleanup_se_shared_mem with reclaim=true.

Because cleanup_se_shared_mem zeros out the DMA pointer and size, the
permanent DMA buffer used by priv_dev_ctx is destroyed, breaking
subsequent operations like system suspend or firmware loading.

Accepted. Will be fixed in v49.

Problem:
DMA buffer reclaimed when it must not be cleanup_se_shared_mem(reclaim=true).
This function was called unconditionally for every dev_ctx on the late-response:
 - with(going_away clear) and without  path(going_away clear).
This caused two distinct failures:
   - priv_dev_ctx: its 128 KB coherent DMA buffer has no
   open/close reallocation cycle. Freeing it caused every subsequent
   internal FW command (PM resume IMEM swap, ele_get_info) to return
   -ENOMEM from get_shared_mem_slot() permanently.

   - userspace dev_ctx with fd still open (pure timeout, no
   close): freeing the buffer while the user's fd was still live
   caused the next ioctl to fault against a freed buffer.

Fix:
- Split the late-response reclaim on cleanup_done. When
   cleanup_done is false (priv_dev_ctx always; userspace fd still
   open), only reset pos and reclaim gen_pool buffers.
   Never call dma_free_coherent().
- Full reclaim is deferred to the cleanup_done==true path.
- for priv_dev_ctx, cleanup_se_shared_mem() will not be called from here.

[ ... ]

quoted

@@ -1331,6 +1331,10 @@ static void cleanup_dev_ctx(struct

se_if_device_ctx *dev_ctx, bool is_fclose)

quoted

                            struct se_if_device_ctx *tx_ctx = is_fclose ?

dev_ctx :

quoted

                                                    dev_ctx->priv-
priv_dev_ctx;

                            if (dev_ctx->strg_hdl &&

se_close_storage(tx_ctx,

quoted

dev_ctx->strg_hdl))

quoted

                                    dev_err(dev_ctx->priv->dev, "failed

to close storage.\n");

quoted

                            if (dev_ctx->sess_hdl &&

se_close_session(tx_ctx,

quoted

dev_ctx->sess_hdl))

quoted

                                    dev_err(dev_ctx->priv->dev, "failed

to close session.\n");

[Severity: High]
Will this skip sending the session close command and leak the firmware
session handle if the killable wait aborted instantly above?

Because ele_msg_send_rcv returns -EBUSY immediately if the circuit breaker
was armed by a SIGKILL interrupt, se_close_storage fails instantly. The
subsequent call to se_close_session is then also skipped due to the busy
firmware path.

An unprivileged user could trigger this leak by opening a session and
sending SIGKILL to their own process, permanently exhausting hardware
enclave resources.

Accepted, will fix in v49. No code changes here. Will fix it in se_clear_fw_busy().
Leaked FW session/storage handles:
   When a SIGKILL interrupted the killable wait, cleanup_dev_ctx()
   skipped se_close_session()/se_close_storage() because fw_busy was
   set. The circuit breaker cleared later but the fd was already
   closed and handles were leaked permanently.
   Fix: in the late-response path, when cleanup_done==true (fd was
   already closed while fw_busy was armed), perform the deferred
   session/storage close via priv_dev_ctx as transmit context.
   dev_ctx is still valid here: se_mark_fw_busy() took a kref_get()
   before parking dev_ctx in fw_busy_dev_ctx; that reference keeps
   dev_ctx alive until kref_put() at the end of se_clear_fw_busy().
   Code: se_ctrl.c: se_clear_fw_busy() cleanup_done==true path adds
   se_close_storage() + se_close_session() via priv_dev_ctx before
   calling cleanup_se_shared_mem(reclaim=true).

[ ... ]

quoted

@@ -2145,6 +2145,8 @@ int se_get_mem_pool_buf(struct

se_if_device_ctx *dev_ctx, void **buf,

quoted

    if (se_is_fw_busy_ctx(dev_ctx))
            return -EBUSY;

    b_desc = kzalloc_obj(*b_desc);
    if (!b_desc)
            return -ENOMEM;

[Severity: High]
Does this kzalloc_obj allocation violate the GFP_NOIO context required by
ele_get_info during suspend?

The kzalloc_obj macro implicitly uses GFP_KERNEL, which can trigger direct
memory reclaim. Since the storage device may already be suspended,
flushing dirty pages to it will lead to an unrecoverable system deadlock.

Accepted as mentioned above. Will be fix GFP_KERNEL allocation in GFP_NOIO context, in v49.
- Fix: add gfp_t gfp parameter to se_get_mem_pool_buf(); pass
   GFP_NOIO from ele_get_info().

- Code: se_ctrl.c: se_get_mem_pool_buf() kzalloc_obj() -> kzalloc(gfp);
   se_ctrl.h: declaration updated with gfp_t gfp parameter;
   ele_base_msg.c: ele_get_info() passes GFP_NOIO.

 > --

Sashiko AI review · https://sashiko.dev/#/patchset/20260904-imx-se-if-v48-0-
dad90eec7eaf@nxp.com?part=5

NXP Confidential