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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing DMA address validation for explicitly allowed base commands.
- [Low] NULL pointer passed to dev_err() in se_clear_fw_busy() after devname is freed.
--

commit 370e05357b53f6daf029676404a43be1e6253abf
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: adds miscdev
    
Adds the driver for communication interface to secure-enclave, that
enables exchanging messages with NXP secure enclave HW IP(s)
like EdgeLock Enclave, from User-Space Applications via character driver.
    
> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 650153d39f460..b60665120db2f 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -15,13 +15,66 @@
>  
>  #define FW_DBG_DUMP_FIXED_STR		"ELE"
>  
> -static void ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf, dma_addr_t d_addr,
> -				 size_t size)
> +int ele_uapi_allowed_base_cmd(struct se_if_device_ctx *dev_ctx,
> +			      struct se_msg_hdr *header, u32 tx_msg_sz)
>  {
> -	if (priv->mem_pool)
> -		gen_pool_free(priv->mem_pool, (unsigned long)buf, size);
> -	else
> -		dma_free_coherent(priv->dev, size, buf, d_addr);
> +	struct se_api_msg *msg = container_of(header, struct se_api_msg, header);
> +	const struct se_cmd_addr_field *fields;
> +	size_t count;
> +
> +	/*
> +	 * Identify the command first. Only commands in this allow-list may be
> +	 * issued from userspace; everything else is rejected. Once a command is
> +	 * known to be supported, decide whether it needs a DMA-address boundary
> +	 * check and, if so, run it before returning.
> +	 */
> +	switch (header->command) {
> +	case ELE_PING_REQ:
> +	case ELE_DEBUG_DUMP_REQ:
> +	case ELE_OEM_VERIFY_IMAGE_REQ:
> +	case ELE_OEM_REL_CONTAINER_REQ:
> +	case ELE_FW_LIFE_CYCLE_REQ:
> +	case ELE_READ_FUSE_REQ:
> +	case ELE_GET_FW_VERS_REQ:
> +	case ELE_RETURN_LIFE_CYCLE_REQ:
> +	case ELE_GET_EVENT_REQ:
> +	case ELE_COMMIT_REQ:
> +	case ELE_GET_FW_STATUS_REQ:
> +	case ELE_WRITE_FUSE:
> +	case ELE_WRITE_SHADOW_FUSE_REQ:
> +	case ELE_READ_SHADOW_FUSE_REQ:
> +		return 0;

[Severity: Medium]
Could this allow userspace to bypass DMA address validation? 

If any of these allowed base commands (such as ELE_OEM_VERIFY_IMAGE_REQ) embed 
DMA addresses in their payloads, returning 0 immediately bypasses the 
se_val_cmd_addrs() check. If this is the case, a malicious userspace 
application could supply arbitrary physical addresses, leading to arbitrary 
memory read/write by the firmware.

[ ... ]

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index b5d3421c09878..3b1209325633e 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> +static void se_clear_fw_busy(struct se_if_priv *priv)
> +{
[ ... ]
> +			if (dev_ctx == priv->priv_dev_ctx) {
> +				/*
> +				 * Internal context: probe-time static DMA buf;
> +				 * se_if_probe_cleanup reclaims it explicitly.
> +				 * Just reset logical pos and return gen_pool
> +				 * loan buffers for reuse.
> +				 */
> +				if (priv->mem_pool)
> +					se_cleanup_mem_pool_buf(dev_ctx, true);
> +				se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +			} else if (dev_ctx->cleanup_done) {
> +				/*
> +				 * Userspace fd already closed while fw_busy was
> +				 * armed (e.g. SIGKILL). FW has now responded;
> +				 * close deferred handles and free the DMA buf.
> +				 */
> +				if (dev_ctx->strg_hdl &&
> +				    se_close_storage(priv->priv_dev_ctx,
> +						     dev_ctx->strg_hdl))
> +					dev_err(priv->dev,
> +						"%s: failed to close deferred storage handle\n",
> +						dev_ctx->devname);

[Severity: Low]
Is dev_ctx->devname safe to use here?

When a userspace transaction times out, the fw_busy circuit breaker is armed. If 
the user then closes the file descriptor, cleanup_dev_ctx() is called and it 
frees dev_ctx->devname and sets it to NULL (along with setting cleanup_done to 
true). 

When the firmware responds late, se_clear_fw_busy() runs. Since cleanup_done is 
true, it tries to close deferred handles here, and if that fails, it passes the 
now-NULL dev_ctx->devname to dev_err(), resulting in "(null)" instead of the 
device name.

> +				if (dev_ctx->sess_hdl &&
> +				    se_close_session(priv->priv_dev_ctx,
> +						     dev_ctx->sess_hdl))
> +					dev_err(priv->dev,
> +						"%s: failed to close deferred session handle\n",
> +						dev_ctx->devname);

[Severity: Low]
Does this also pass a NULL dev_ctx->devname to dev_err()?

Just like the storage handle check above, if closing the session fails, 
dev_ctx->devname is NULL here because cleanup_dev_ctx() has already freed it.

[ ... ]
> +static void cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
> +{
[ ... ]
> +			/*
> +			 * fw_busy is caused by one timed-out synchronous transaction.
> +			 * Only that transaction's dev_ctx may still have coherent
> +			 * memory referenced by FW. Do not skip cleanup for unrelated
> +			 * contexts while fw_busy is set.
> +			 */
> +			if (se_is_fw_busy_ctx(dev_ctx))
> +				dev_warn(dev_ctx->priv->dev,
> +					 "%s: deferring shared memory cleanup while FW is busy\n",
> +					 dev_ctx->devname);
> +			else
> +				cleanup_se_shared_mem(dev_ctx, true);
> +
> +			kfree(dev_ctx->devname);
> +			dev_ctx->devname = NULL;
> +			dev_ctx->cleanup_done = true;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-imx-se-if-v50-0-80834ef510d3@nxp.com?part=5