RE: [PATCH v38 3/7] firmware: imx: add driver for NXP EdgeLock Enclave

> > +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info
> > +*s_info) {
>
> [ ... ]
>
> > +   ret = ele_msg_send_rcv(priv, tx_msg, ELE_GET_INFO_REQ_MSG_SZ,
> rx_msg,
> > +                          ELE_GET_INFO_RSP_MSG_SZ);
> > +   if (ret < 0) {
> > +           ele_get_info_cleanup(priv, get_info_data, get_info_addr,
> > +get_info_len);
>
> [Severity: High]
> If an enclave command times out, ele_msg_send_rcv() returns -ETIMEDOUT and
> the error path immediately calls ele_get_info_cleanup(), which frees the DMA
> buffer. If the firmware is still executing and completes the transaction later, could
> it perform a DMA write into physical memory that has been reallocated to
> another subsystem?
>
> > +           return ret;
> > +   }
>
[Already taken care in 5/7]
At 3/7 the cleanup frees unconditionally (gen_pool_free/dma_free_coherent) and
the timeout leg only does a bare atomic_set(\&priv->fw_busy, 1),
which cannot fence an in-flight enclave DMA.
The symbols that fix it (se_is_fw_busy_ctx/se_mark_fw_busy, priv_dev_ctx) do not exist at 3/7.
It is resolved by PATCH v33 5/7 (fb107010a07b, "firmware: imx: adds miscdev"),
which adds per-context fw_busy ownership and rewrites ele_get_info_cleanup() to
 quarantine (return without freeing) the buffer while the owning context is still busy.

> [ ... ]
>
> > +int ele_debug_dump(struct se_if_priv *priv) {
>
> [ ... ]
>
> > +           rx_msg->header.size -= 2;
> > +
> > +           if (rx_msg->header.size > 2)
> > +                   rx_msg->header.size--;
> > +
> > +           for (i = 0; i < rx_msg->header.size; i += 2)
> > +                   dev_info(priv->dev, "%s%02x_%02x: 0x%08x 0x%08x",
> > +                            FW_DBG_DUMP_FIXED_STR, msg_ex_cnt, i,
> > +                            rx_msg->data[i + 1], rx_msg->data[i + 2]);
>
> [Severity: Low]
> Does this loop read past the actual message payload length? If the firmware
> returns an odd number of payload words, the remaining size is not even. The
> loop body would execute when exactly 1 word remains but print both rx_msg-
> >data[i + 1] and rx_msg->data[i + 2].
>
Not an issue.
rx_msg->data[i + 2], would be printed as 0x00000000.

> > +
> > +           msg_ex_cnt++;
> > +   } while (keep_logging);
> > +
> > +   return ret;
> > +}
>
> > diff --git a/drivers/firmware/imx/se_ctrl.c
> > b/drivers/firmware/imx/se_ctrl.c index 0000000000000..53091cc37e7e0
> > 100644
> > --- /dev/null
> > +++ b/drivers/firmware/imx/se_ctrl.c
>
> [ ... ]
>
> > +static void se_if_probe_cleanup(void *plat_dev) {
> > +   struct platform_device *pdev = plat_dev;
> > +   struct device *dev = &pdev->dev;
> > +   struct se_if_priv *priv;
> > +
> > +   priv = dev_get_drvdata(dev);
> > +   if (!priv)
> > +           return;
> > +
> > +   if (priv->rx_chan)
> > +           mbox_free_channel(priv->rx_chan);
> > +   if (priv->tx_chan)
> > +           mbox_free_channel(priv->tx_chan);
> > +
> > +   /*
> > +    * Being device managed buffer, no need to free the buffer allocated
> > +    * in se probe to store encrypted IMEM.
> > +    */
> > +
> > +   /*
> > +    * No need to check, if reserved memory is allocated
> > +    * before calling for its release. Or clearing the
> > +    * un-set bit.
> > +    */
> > +   of_reserved_mem_device_release(dev);
> > +
> > +   dev_set_drvdata(dev, NULL);
> > +
> > +   kfree(priv);
>
> [Severity: High]
> Similarly, does this teardown path free device-managed DMA memory without
> waiting for or aborting a potentially busy firmware transaction?

Superseded by patch 5/7.

The teardown safety concern is valid in isolation on commit 3/7 ("firmware: imx: add driver for NXP EdgeLock Enclave"),
which only has the minimal skeleton.
It is resolved in commit 5/7 ("firmware: imx: adds miscdev") where `se_if_probe_cleanup()` is rewritten to:

1. `atomic_set(&priv->going_away, 1)` — circuit-breaker that causes new `ele_msg_send_rcv()` callers to return `-ENODEV` without arming a transaction.
2. `complete_all(&priv->waiting_rsp_clbk_hdl.done)` — wakes any thread currently blocked waiting for a FW response.
3. `cancel_work_sync(&priv->fw_busy_work)` — drains the late-response workqueue.
4. `kref_put(&priv->refcount, se_if_priv_release)` replaces the bare `kfree(priv)` — `priv` and all embedded objects (completions, spinlocks) are freed only after all reference holders have dropped their ref.

The IMEM DMA buffer (`dmam_alloc_coherent`) is devres-managed and was never freed by `kfree(priv)` in either version; devres releases it automatically after the devm action returns

>
> > +}
>
> [ ... ]
>
> > +static int se_resume(struct device *dev) {
> > +   struct se_if_priv *priv = dev_get_drvdata(dev);
> > +   struct se_fw_load_info *load_fw;
> > +   int ret = 0;
> > +
> > +   load_fw = get_load_fw_instance(priv);
> > +
> > +   if (load_fw->imem_mgmt) {
> > +           ret = se_restore_imem_state(priv, &load_fw->imem);
> > +           if (ret)
> > +                   dev_warn(dev, "Failure restoring IMEM state[0x%x]",
> ret);
> > +   }
> > +
> > +   return 0;
>
> [Severity: Medium]
> Should this return the actual error instead of 0? se_restore_imem_state()
> documents that it returns -EIO so the PM subsystem knows the enclave is non-
> functional after resume, but the failure is caught here and 0 is unconditionally
> returned, breaking the expected power management error handling semantics.
>
Accepted will fix in v39.

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

NXP Confidential