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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The command receiver bypasses session and storage handle ownership checks, allowing it to close arbitrary handles belonging to other processes.
- [High] Permitting the command receiver to issue `cmd_tag` messages via `write()` corrupts the synchronous IOCTL response channel.
- [High] The driver converts a successful firmware transaction into an `-EINTR` error if a signal was received during the wait, leading to silent data corruption and handle leaks.
- [High] Incomplete bounds validation for buffers with no explicit size allows the firmware to read outside the shared memory window.
- [High] TOCTOU race in `ELE_STORAGE_OPEN_REQ` validation allows multiple command receivers to open storage sessions, misrouting NVM callbacks.
- [Medium] The SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl always fails with -ENOMEM.
- [Low] Variable declarations immediately follow a switch case label, which is invalid C syntax.
--

--- Patch [5]: [PATCH v39 5/7] firmware: imx: adds miscdev ---
commit 592c4b7b59033acb6e7acab4d619d7a526911c5e
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: adds miscdev
    
This commit adds the character driver for the NXP EdgeLock Enclave (ELE).
It exposes ioctls for userspace applications to allocate shared memory, get
SoC info, and exchange messages with the enclave firmware.

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

> 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
> @@ -47,100 +47,150 @@ int se_val_cmd_addrs(struct se_if_device_ctx *dev_ctx, struct se_api_msg *msg,
>  		     u32 tx_msg_sz, const struct se_cmd_addr_field *fields,
>  		     size_t count)
>  {

[ ... ]

> +		if (f->size_idx == SE_CMD_RCVR_ADDR_VAR_SIZE) {
> +			struct cmd_rcvr_data_info *crcvr_info =
> +							&dev_ctx->priv->crcvr_info;
> +			/*
> +			 * Export-response buffer: the size was supplied by FW
> +			 * in the preceding export command and stored per SE
> +			 * interface in cmd_rcvr_var_size.
> +			 */
> +			if ((u64)crcvr_info->cmd_rcvr_var_size > end - addr)
> +				return -EACCES;
> +		} 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]
If size_idx is SE_CMD_ADDR_NO_SIZE and buf_size is 0, the logic only checks
that the start address is within bounds (addr < base || addr >= end) earlier
in the function.

Could a user supply a DMA address at the very end of the shared memory window
(e.g., end - 1), causing the firmware to read the multi-byte container header
out of bounds into adjacent physical memory?

>  	}
>  
>  	return 0;
>  }

[ ... ]

> @@ -468,76 +468,86 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  		     int tx_msg_sz, void *rx_msg, int exp_rx_msg_sz)
>  {

[ ... ]

>  	err = ele_msg_rcv(dev_ctx, &priv->waiting_rsp_clbk_hdl);
>  
>  	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
>  		/*
>  		 * Signal was deferred until the FW/kernel protocol resynchronized.
>  		 * On success report -ERESTARTSYS for the interrupted wait; the
>  		 * command is not re-sent. Keep real errors like -ETIMEDOUT.
>  		 */
>  		if (err > 0)
>  			err = -ERESTARTSYS;

[Severity: High]
Does this sequence result in silent data corruption and handle leaks?

If a signal interrupts the wait in ele_msg_send_rcv() and the wait
continues killably, a successful firmware response (err > 0) gets
overridden to -ERESTARTSYS.

The ioctl handler (se_ioctl_cmd_snd_rcv_rsp_handler) then converts this to
-EINTR and returns without copying the successful response payload (rx_msg)
back to userspace.

Will userspace subsequently retry the operation with zeroed shared memory
buffers, leading to dropped response data or leaked firmware handles?

>  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
>  		dev_dbg(priv->dev, "%s: Err[0x%x]:Interrupted by signal.",
>  			dev_ctx->devname, err);
>  	}
>  
>  	ele_msg_send_rcv_cleanup(priv);
>  
>  	return err;
>  }

[ ... ]

> @@ -552,74 +552,74 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  {

[ ... ]

>  	/* Incoming command: wake up the receiver if any. */
>  	if (header->tag == priv->if_defs->cmd_tag) {
>  		se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
>  		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);

[ ... ]

>  	} else if (header->tag == priv->if_defs->rsp_tag) {
>  		bool exception_for_sz_mismatch = check_hdr_exception_for_sz(priv, header);
>  		u32 exp_rx_msg_sz = 0;
>  
>  		/*
>  		 * rx_msg and rx_msg_sz are owned by the sender under clbk_rx_lock.
>  		 * Read both under the lock: drop a late response instead of copying
>  		 * into freed memory, and avoid a stale size. A late response also
>  		 * closes the firmware-busy circuit breaker.
>  		 */
>  		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
>  		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);

[Severity: High]
Can asynchronous writes corrupt synchronous ioctl responses?

The write() handler allows sending cmd_tag messages without waiting for a
response. When the firmware replies with a rsp_tag, se_if_rx_callback()
blindly assigns it to the single synchronous ioctl waiter
(priv->waiting_rsp_clbk_hdl).

If the command receiver writes a command while another process is blocked
in an ioctl, will the ioctl process wake up with the asynchronous response
data, corrupting its state?

>  		if (!se_clbk_hdl->rx_msg) {
>  			if (atomic_read(&priv->fw_busy))
>  				schedule_fw_busy_work = true;
>  			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  
>  			if (schedule_fw_busy_work)
>  				schedule_work(&priv->fw_busy_work);
>  
>  			dev_info(dev, "ELE responded (late), recovery FW available.");
>  			return;
>  		}

> 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
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2026 NXP
> + */
> +
> +#include "se_ctrl.h"
> +#include "ele_common.h"
> +#include "ele_fw_api.h"
> +
> +static int se_cmd_receiver_allowed_cmd(struct se_if_device_ctx *dev_ctx,
> +				       struct se_api_msg *msg, u32 tx_msg_sz)
> +{
> +	u8 cmd = msg->header.command;
> +
> +	switch (cmd) {
> +	case ELE_SESSION_CLOSE_REQ:
> +	case ELE_STORAGE_CLOSE_REQ:
> +		return 0;

[Severity: High]
Does this bypass the handle ownership checks for the command receiver?

In ele_uapi_allowed_fw_cmd(), if the caller is the command receiver,
the request is delegated here and unconditionally allowed.

This seems to bypass the main switch statement where dev_ctx->sess_hdl
and dev_ctx->strg_hdl are verified against msg->data[0]. Could a malicious
command receiver use this to close arbitrary handles belonging to other
processes?

> +	case ELE_STORAGE_MASTER_IMPORT_REQ:
> +		const struct se_cmd_addr_field *fields;
> +		size_t count;

[Severity: Low]
Is this variable declaration right after the case label valid C11 syntax?
Standard C requires a statement after a label, and a declaration is not
considered a statement. Does this cause a build failure?

> +
> +		fields = ele_fw_cmd_addr_fields(cmd, &count);
> +		return se_val_cmd_addrs(dev_ctx, msg, tx_msg_sz, fields, count);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

[ ... ]

> +int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr *header,
> +			    u32 tx_msg_sz, u32 rx_msg_sz)
> +{

[ ... ]

> +	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
> +		 * already registered as the command receiver. If we let the
> +		 * command through, FW would allocate a new storage handle and
> +		 * start sending NVM callbacks for it; those callbacks would be
> +		 * routed to the existing receiver (process A), not to the
> +		 * caller (process B). This would let process A observe and
> +		 * tamper with process B's NVM traffic. Reject early, before
> +		 * the command reaches FW, so no handle is allocated and the
> +		 * state stays consistent.
> +		 */
> +		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;

[Severity: High]
Can concurrent callers race between this pre-check and the actual receiver
registration?

The modify_lock is dropped immediately after checking if a receiver is
already registered. Multiple callers could pass this check, send their
requests to firmware, and receive valid storage handles.

> +		if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> +			ret = -EINVAL;
> +		break;

[ ... ]

> +void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct se_api_msg *rx_msg)
> +{
> +	struct se_msg_hdr *header = &rx_msg->header;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	switch (header->command) {
> +	case ELE_SESSION_OPEN_REQ:
> +		dev_ctx->sess_hdl = rx_msg->data[1];
> +		break;
> +	case ELE_SESSION_CLOSE_REQ:
> +		dev_ctx->sess_hdl = 0;
> +		break;
> +	case ELE_STORAGE_OPEN_REQ: {
> +		int rc;
> +
> +		/*
> +		 * Record the storage handle before registering as command
> +		 * receiver. FW has already allocated the handle; if we assigned
> +		 * it only after a successful registration, a failing
> +		 * set_dev_ctx_as_command_receiver() (e.g. -EBUSY) would leave
> +		 * strg_hdl at 0 while the ioctl still returns success to
> +		 * userspace. The kernel would then never close the handle on
> +		 * teardown, leaking it in FW. Storing it first guarantees
> +		 * cleanup_dev_ctx() closes it on the next close(), regardless
> +		 * of whether registration succeeded.
> +		 */
> +		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]
If the set_dev_ctx_as_command_receiver() call fails due to the race
mentioned above, the error is merely logged and ignored.

Does this leave the second caller with a valid storage handle, while all
NVM callbacks are misrouted to the first caller? Should this failure path
be handled more robustly?

> 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
> @@ -0,0 +1,635 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2026 NXP
> + */

[ ... ]

> +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]
Does omitting length constraints for the container header allow out-of-bounds
reads? As noted above, since size_idx is SE_CMD_ADDR_NO_SIZE and buf_size
defaults to 0, no upper bound check is performed when the command is validated
in se_val_cmd_addrs().

> +};

> 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

[ ... ]

> @@ -1195,30 +1195,30 @@ int set_dev_ctx_as_command_receiver(struct se_if_device_ctx *dev_ctx, bool is_io
>  {
>  	struct se_if_priv *priv = dev_ctx->priv;
>  	struct se_api_msg *new_rx_msg = NULL;
>  	struct se_clbk_handle *se_clbk_hdl;
>  	unsigned long flags;
>  
>  	se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
>  	guard(mutex)(&priv->modify_lock);
>  	if (se_clbk_hdl->dev_ctx == dev_ctx)
>  		return 0;
>  
>  	if (se_clbk_hdl->dev_ctx)
>  		return -EBUSY;
>  
>  	if (!dev_ctx->strg_hdl)
>  		return -EINVAL;
>  
>  	if (is_ioctl)
>  		return -ENOMEM;

[Severity: Medium]
Does this unconditionally reject the SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl?
When the ioctl calls this function with is_ioctl = true, it immediately
returns -ENOMEM before any allocations happen. Is this intentional, or does
it break the ioctl completely?

[ ... ]

> @@ -1505,30 +1505,30 @@ static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
>  					    void __user *uarg)
>  {

[ ... ]

>  		/*
>  		 * -ERESTARTSYS here means the wait was interrupted by a signal
>  		 * after the command had already been handed to - and executed
>  		 * by - the firmware, with its response delivered into rx_msg
>  		 * (ele_msg_send_rcv() converts only a positive, i.e. successfully
>  		 * received, result to -ERESTARTSYS). If that response carried a
>  		 * freshly allocated session/storage handle, record it now via
>  		 * fw_api_specific_ops(): the handle is already live in firmware,
>  		 * so leaving it untracked would stop cleanup_dev_ctx() from ever
>  		 * closing it and leak the firmware resource. Validate the
>  		 * delivered response first, using its own declared length bounded
>  		 * by the caller's buffer, so a truncated or malformed reply is
>  		 * not acted upon.
>  		 */
>  		if (err == -ERESTARTSYS) {
>  			u32 rsp_sz = rx_msg->header.size << 2;
>  
>  			if (rsp_sz && rsp_sz <= cmd_snd_rcv_rsp_info.rx_buf_sz &&
>  			    !se_val_rsp_hdr_n_status(priv, rx_msg,
>  						     tx_msg->header.command, rsp_sz,
>  						     tx_msg->header.ver ==
>  						     priv->if_defs->base_api_ver)) {
>  				se_dev_ctx_cpy_out_data(dev_ctx);
>  				fw_api_specific_ops(dev_ctx, rx_msg);
>  			}
>  			/*
>  			 * Returning -ERESTARTSYS would let the VFS transparently restart
>  			 * the ioctl, which would re-run the command with the just
>  			 * cleaned-up (zeroed) shared input buffers. Report -EINTR instead
>  			 * so the syscall is not auto-restarted; userspace enters its
>  			 * signal handler and can decide whether to reissue the command.
>  			 */
>  			err = -EINTR;
>  		}

[Severity: High]
See the note about silent data corruption and handle leaks above; this is where
the successful ioctl returns -EINTR without copying the response to the user.

[ ... ]

> @@ -1807,30 +1807,30 @@ static ssize_t se_if_fops_write(struct file *fp, const char __user *buf,
>  				size_t size, loff_t *ppos)
>  {
>  	struct se_if_device_ctx *dev_ctx = fp->private_data;
>  	struct se_if_priv *priv;
>  	int err;
>  
>  	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &dev_ctx->fops_lock) {
>  		if (dev_ctx->cleanup_done)
>  			return -ENODEV;
>  
>  		priv = dev_ctx->priv;
>  
>  		dev_dbg(priv->dev, "%s: write from buf (%p)%zu, ppos=%lld.", dev_ctx->devname,
>  			buf, size, ((ppos) ? *ppos : 0));
>  
>  		if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
>  			se_dev_ctx_shared_mem_cleanup(dev_ctx);
>  			return -EINVAL;
>  		}
>  
>  		if (size < SE_MU_HDR_SZ || size > MAX_ALLOWED_TX_MSG_SZ) {
>  			dev_err(priv->dev, "%s: User buffer too small/large(%zu < %d)",
>  				dev_ctx->devname, size,
>  				size < SE_MU_HDR_SZ ? SE_MU_HDR_SZ :
>  								MAX_ALLOWED_TX_MSG_SZ);
>  			return -ENOSPC;
>  		}
>  
>  		struct se_api_msg *tx_msg __free(kfree) = memdup_user(buf, size);
>  		if (IS_ERR(tx_msg))
>  			return PTR_ERR(tx_msg);
>  
>  		err = se_chk_tx_msg_hdr(dev_ctx, &tx_msg->header, size, 0);
>  		if (err)
>  			return err;
>  
>  		print_hex_dump_debug("from user ", DUMP_PREFIX_OFFSET, 4, 4,
>  				     tx_msg, size, false);
>  
>  		err = ele_msg_send(dev_ctx, tx_msg, size);
>  
>  		return err;

[Severity: High]
See the note about asynchronous writes corrupting synchronous ioctl responses
above. This is the write handler that sends the command that generates the
conflicting response.

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-imx-se-if-v39-0-41e4257c2afc@nxp.com?part=5