Re: [PATCH v49 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:
- [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 <pankaj.gupta@nxp.com>

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.

> 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
[ ... ]
> @@ -66,13 +66,13 @@ int se_val_cmd_addrs(struct se_if_device_ctx *dev_ctx, struct se_api_msg *msg,
[ ... ]
> +		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?

[ ... ]
> @@ -529,13 +529,13 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
[ ... ]
> +	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;
> +	}

[ ... ]
> 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
> @@ -20,13 +20,13 @@ static const struct se_cmd_addr_field ele_get_info_addr_fields[] = {
[ ... ]
> +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.

[ ... ]
> @@ -125,13 +125,13 @@ static const struct se_cmd_addr_field ele_pub_key_export_addr_fields[] = {
[ ... ]
> +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?

[ ... ]
> 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
> @@ -1070,13 +1070,13 @@ static int se_if_misc_register(struct se_if_priv *priv)
[ ... ]
> +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