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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Global static `buf_size` variables in `ele_msg_addr_field.c` create a race condition between concurrent export commands across multiple Secure Enclave interfaces, allowing a DMA bounds-check bypass and memory corruption.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `ELE_STORAGE_OPEN_REQ` allows multiple processes to concurrently open storage handles while bypassing command receiver isolation.
- [High] The `ret == 0` timeout path in `ele_msg_rcv()` still uses `!completion_done()` instead of `!se_clbk_hdl->rx_delivered`, leaving a DMA-after-free race condition open.
- [High] Use-after-free of `struct se_if_open_gate` in `se_if_fops_open()` due to a race with module unbind/device teardown.
- [High] A malicious command receiver can bypass the end-boundary check for export responses by calling `write()` without calling `read()`, allowing out-of-bounds DMA writes.
--

--- Patch [5]: [PATCH v38 5/7] firmware: imx: adds miscdev ---
commit 55cc70beb0402e521ca21c2a4ece63037edf487d
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: adds miscdev
Summary: Adds the driver for communication interface to secure-enclave, that enables exchanging messages with NXP secure enclave HW IP(s). It includes a character driver interface for user-space applications to send and receive commands, manage memory, and handle firmware responses.

> diff --git a/drivers/firmware/imx/ele_msg_addr_field.c b/drivers/firmware/imx/ele_msg_addr_field.c
> --- a/drivers/firmware/imx/ele_msg_addr_field.c
> +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> @@ -575,6 +575,6 @@
>  static struct se_cmd_addr_field ele_storage_master_export_addr_fields[] = {
>  	{ .lsb_idx = 2, .flag_idx = SE_CMD_ADDR_ALWAYS,
>  	  .size_idx = SE_CMD_ADDR_NO_SIZE, .buf_size = 0 },	/* key_store_export_address */
>  };
>
> [ ... ]
>
> @@ -617,6 +617,6 @@
>  void ele_set_sz_in_field_addr(u8 cmd, u32 size)
>  {
>  	switch (cmd) {
>  	case ELE_STORAGE_MASTER_EXPORT_REQ:
>  		ele_storage_master_export_addr_fields[0].buf_size = size;
>  		break;

[Severity: High]
If there are multiple secure enclave interfaces, do they share this global
static array ele_storage_master_export_addr_fields? If two instances
execute an export concurrently, could they race to overwrite this buf_size?
If one instance's validation uses the other's size, might this allow a
smaller bounds check to pass while the firmware executes a larger DMA write,
resulting in out-of-bounds memory corruption?

> 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
> @@ -152,7 +152,7 @@
>  	case ELE_STORAGE_OPEN_REQ:
>  		/* Might be cleared as part of tear down. */
>  		if (dev_ctx->strg_hdl) {
>  			ret = -EEXIST;
>  			break;
>  		}
>  		/*
>  		 * Reject the storage-open request when another context is
> ...
>  		 */
>  		scoped_guard(mutex, &priv->modify_lock)
>  			if (priv->cmd_receiver_clbk_hdl.dev_ctx &&
>  			    priv->cmd_receiver_clbk_hdl.dev_ctx != dev_ctx)
>  				ret = -EBUSY;
>  		if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
>  			ret = -EINVAL;
>  		break;
>
> [ ... ]
>
> @@ -252,7 +252,7 @@
>  	case ELE_STORAGE_OPEN_REQ: {
>  		int rc;
>
>  		/*
> ...
>  		 */
>  		dev_ctx->strg_hdl = rx_msg->data[1];
>
>  		rc = set_dev_ctx_as_command_receiver(dev_ctx, false);
>  		if (rc)
>  			dev_err(priv->dev,
>  				"Failed to register %s as CMD-Receiver: %d\n",
>  				dev_ctx->devname, rc);
>  		break;
>  	}

[Severity: High]
Since modify_lock is dropped before sending the command to firmware in the
pre-check, could two processes concurrently pass this check and both receive
storage handles from the firmware? If the second process fails registration
in set_dev_ctx_as_command_receiver() but the error is just logged and
discarded, wouldn't it successfully return the handle to userspace? Could
this result in the firmware incorrectly routing the second process's NVM
callbacks to the first registered process?

> 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
> @@ -140,4 +140,4 @@
>  		if (f->size_idx != SE_CMD_ADDR_NO_SIZE) {
> ...
>  		} else if (f->buf_size) {
>  			/* buf_size: literal byte count
>  			 *           (FW-defined constant or saved at runtime).
>  			 */
>  			if ((u64)f->buf_size > end - addr)
>  				return -EACCES;
>  		}

[Severity: High]
If a malicious command receiver skips calling read() and directly issues a
write() to send an export response, does buf_size remain at its default
value of zero? If buf_size is zero, the check else if (f->buf_size)
evaluates to false, completely bypassing the end-bounds check. Could an
attacker exploit this by specifying an address at the very end of the shared
memory window, causing the firmware to DMA the large export payload out of
bounds?

> [ ... ]
>
> @@ -367,4 +367,4 @@
>  		if (ret == 0) {
> ...
>  			spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  			se_clbk_hdl->rx_msg = NULL;
>  			if (!completion_done(&se_clbk_hdl->done))
>  				se_mark_fw_busy(dev_ctx);
>  
>  			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);

[Severity: High]
Is completion_done() the correct check to use in this timeout path? If
device teardown runs concurrently and invokes complete_all(), won't
completion_done() return true? This would cause the code to skip calling
se_mark_fw_busy() and proceed to free the shared DMA buffer while the
firmware might still be processing the command, potentially leading to a
DMA-after-free memory corruption. Should this check use
!se_clbk_hdl->rx_delivered instead, similar to the other paths?

> 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
> @@ -1885,5 +1885,5 @@
>  static int se_if_fops_open(struct inode *nd, struct file *fp)
>  {
>  	struct miscdevice *miscdev = fp->private_data;
>  	struct se_if_open_gate *gate;
> ...
>  	gate = container_of(miscdev, struct se_if_open_gate, miscdev);
>  
>  	if (!se_if_open_gate_get(gate))
>  		return -ENODEV;

[Severity: High]
Could the gate pointer be freed concurrently before se_if_open_gate_get()
is called? When userspace calls open(), the VFS resolves the miscdev and
drops misc_mtx. If device unbind executes at this exact moment,
misc_deregister() can complete and the teardown path will free the gate
structure. When the open thread resumes, won't it dereference a freed
gate structure here, resulting in a use-after-free?

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