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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Permanent destruction of priv_dev_ctx's shared DMA buffer on late firmware response.
- [High] Leaked firmware session handles due to killable wait in `close()` aborting instantly on fatal signals.
- [High] Deadlock risk during PM suspend due to GFP_KERNEL allocation inside a required GFP_NOIO context.
--

commit 8a8db094cff29d90b76f6889f2022ca7253f3ec8
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: adds miscdev

Adds the driver for communication interface to NXP secure-enclave hardware
via a character driver. Handles ABI, initialization, shared memory, and
restricted command forwarding.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
[ ... ]
> @@ -121,9 +121,9 @@ int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
>  	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,
>  					  &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.

[ ... ]
> 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
[ ... ]
> @@ -342,6 +342,7 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
>  				if (se_clbk_hdl->rx_msg) {
>  					/*
>  					 * The enclave may still DMA into this
>  					 * buffer (either a normal timeout or
>  					 * a teardown complete_all() wakeup).
>  					 * 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.

[ ... ]
> 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
[ ... ]
> @@ -406,6 +406,12 @@ static void cleanup_se_shared_mem(struct se_if_device_ctx *dev_ctx, bool recla
>  	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
>  	 * buffer has been freed. On the deliberate-leak path the buffer is
>  	 * abandoned on purpose, so clearing the pointer here guarantees a later
>  	 * 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.

[ ... ]
> @@ -1149,6 +1149,7 @@ static void se_clear_fw_busy(struct se_if_priv *priv)
>  			/*
>  			 * Late response arrived after going_away is clear.
>  			 * fw_busy has already been cleared atomically above;
>  			 * reclaim the shared-memory slot now. For priv_dev_ctx
>  			 * 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.

[ ... ]
> @@ -1331,6 +1331,10 @@ static void cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
>  				struct se_if_device_ctx *tx_ctx = is_fclose ? dev_ctx :
>  							dev_ctx->priv->priv_dev_ctx;
>  
>  				if (dev_ctx->strg_hdl && se_close_storage(tx_ctx,
>  									  dev_ctx->strg_hdl))
>  					dev_err(dev_ctx->priv->dev, "failed to close storage.\n");
>  				if (dev_ctx->sess_hdl && se_close_session(tx_ctx,
>  									  dev_ctx->sess_hdl))
>  					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.

[ ... ]
> @@ -2145,6 +2145,8 @@ int se_get_mem_pool_buf(struct se_if_device_ctx *dev_ctx, void **buf,
>  	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.

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