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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Moving `atomic_set(&fbusy_info->fw_busy, 0)` to the end of `se_clear_fw_busy()` causes all session and storage handle cleanup requests to fail, permanently leaking firmware resources.
- [High] Missing upper-bound checks for variable-length input DMA buffers in ELE_OEM_AUTH_CONTAINER_REQ and ELE_KEYSTORE_REPROV_ENABLE_REQ allow malicious userspace to trigger out-of-bounds physical memory reads, potentially crashing the secure enclave or system.
--

commit b1854f8ad905b9bef822cde3f1cab2d887c816c2
Author: Pankaj Gupta [off-list ref]

firmware: imx: adds miscdev

This commit introduces a character device driver to facilitate communication
with the NXP secure enclave (EdgeLock Enclave) from user-space applications.
It manages the shared memory buffers, enforces command whitelisting, and
handles device context cleanup.

[ ... ]

[ ... ]

+		if (f->size_idx == SE_CMD_RCVR_ADDR_VAR_SIZE) {

[ ... ]

+		} else if (f->size_idx != SE_CMD_ADDR_NO_SIZE) {
+			u64 len;
+
+			if (f->size_idx >= payload_words)
+				return -EINVAL;
+
+			/* size_mask == 0 with a valid size_idx is a descriptor bug. */
+			if (!f->size_mask)
+				return -EINVAL;
+
+			/*
+			 * Widen to u64 before shifting: size_shift is u8 and
+			 * shifting a u32 by >= 32 is undefined behaviour.
+			 */
+			len = ((u64)msg->data[f->size_idx] >> f->size_shift) & f->size_mask;
+			if (len > end - addr)
+				return -EACCES;
+		} else if (f->buf_size) {
+			/* buf_size: literal byte count (FW-defined constant). */
+			if ((u64)f->buf_size > end - addr)
+				return -EACCES;
+		}

[Severity: High]
Does this code fail to enforce an upper bound when f->size_idx is
SE_CMD_ADDR_NO_SIZE and f->buf_size is 0?

If a malicious caller provides a DMA address very close to the end of the
window (e.g., end - 1), it seems the firmware might read past the allocated
shared memory boundary. Could this lead to an out-of-bounds physical memory
read over the DMA bus?

[ ... ]

[ ... ]

+	if (atomic_read(&fbusy_info->fw_busy)) {
+		spin_unlock_irqrestore(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
+		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n", dev_ctx->devname);
+		return -EBUSY;
+	}

[ ... ]

[ ... ]

+static const struct se_cmd_addr_field ele_oem_auth_cntr_addr_fields[] = {
+	/*
+	 * Container Header address: a 64-bit physical address split across two
+	 * words. data[0] holds the 32-bit MSB and data[1] holds the 32-bit LSB
+	 * (ELE API spec Table 27, word size = 0x3, so the command is header +
+	 * MSB + LSB only). The message carries no length word for this buffer;
+	 * the container size is variable and not communicated in the MU payload,
+	 * and no static firmware-defined maximum is specified. Because this is a
+	 * read-only input buffer (the ELE ROM/FW copies the container header
+	 * into its internal memory for authentication and does not write back
+	 * through this address), enforcing only the start-address range check is
+	 * acceptable: a rogue caller can at most cause firmware to read within
+	 * the shared-memory window, which is memory the caller already owns.
+	 * Output buffers must be fully bounded; input-only buffers are safe with
+	 * addr-only checks.
+	 */
+	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
+	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* container_hdr_addr */
+};

[Severity: High]
Is the assumption in this comment correct?

Even for an input-only buffer, if there is no length bound checked here, a
rogue caller could supply a valid start address immediately preceding an
unmapped or restricted physical memory region. The firmware reading the
variable-length container header could then cross the boundary, triggering a
bus error, IOMMU fault, or SError that might crash the system or enclave.

[ ... ]

[ ... ]

+static const struct se_cmd_addr_field ele_keystore_reprov_en_addr_fields[] = {
+	/*
+	 * Signed message address: a 64-bit physical address split across two
+	 * words. data[0] holds the 32-bit MSB and data[1] holds the 32-bit LSB
+	 * (ELE API spec Table 202, word size = 0x3, so the command is header +
+	 * MSB + LSB only). The address points to the start of the complete
+	 * signed message block (header + 12-byte payload from Table 204 +
+	 * signature); the total block size depends on the signing format and is
+	 * not carried anywhere in the MU payload words. No static
+	 * firmware-defined maximum for the full block is specified. Because this
+	 * is a read-only input buffer (firmware reads and verifies the signed
+	 * block, does not write back through this address), enforcing only the
+	 * start-address range check is acceptable: a rogue caller can at most
+	 * cause firmware to read within the shared-memory window, which is
+	 * memory the caller already owns. Output buffers must be fully bounded;
+	 * input-only buffers are safe with addr-only checks.
+	 */
+	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
+	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* signed_msg_addr */
+};

[Severity: High]
Similar to the container header check in ele_oem_auth_cntr_addr_fields, could
this lack of an upper bound size check allow malicious userspace to trigger
out-of-bounds physical memory reads by the firmware during reprovisioning
operations?

[ ... ]

[ ... ]

+static void se_clear_fw_busy(struct se_if_priv *priv)
+{

[ ... ]

+			} 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);
+				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);
+				dev_ctx->strg_hdl = 0;
+				dev_ctx->sess_hdl = 0;
+				cleanup_se_shared_mem(dev_ctx, true);
+			} else {

[ ... ]

+	spin_lock_irqsave(&fbusy_info->fw_busy_lock, flags);
+	atomic_set(&fbusy_info->fw_busy, 0);
+	spin_unlock_irqrestore(&fbusy_info->fw_busy_lock, flags);
+	kref_put(&dev_ctx->refcount, se_if_dev_ctx_release);
+}

[Severity: High]
Will this cleanup path accidentally leak the handles it intends to close?

In se_clear_fw_busy(), atomic_set(&fbusy_info->fw_busy, 0) is deferred until
the very end of the function. However, the calls to se_close_storage() and
se_close_session() both rely on ele_msg_send_rcv().

Because ele_msg_send_rcv() checks atomic_read(&fbusy_info->fw_busy) and
returns -EBUSY immediately if it is set (as quoted earlier from
drivers/firmware/imx/ele_common.c), won't these close requests be rejected
without ever reaching the firmware? This could allow a userspace process
repeatedly killed by SIGKILL to exhaust the secure enclave's handles.

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