Re: [PATCH v35 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:
- [Critical] The driver allows userspace to embed arbitrary DMA addresses in permitted firmware commands, enabling arbitrary physical memory overwrite.
- [High] The `se_if_open_gate` structure can be accessed after it is freed if an `open()` system call races with driver unbind.
- [High] A concurrent `close()` racing with driver unbind causes a Use-After-Free on the mailbox channel `priv->tx_chan` due to premature unlinking.
- [High] Firmware session and storage handles can be permanently leaked if response validation or userspace memory copies fail, bypassing the tracking of allocated handles.
- [High] Signal interruption converts an interruptible wait into an unkillable uninterruptible wait for up to 5000 seconds.
--

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

This commit adds a character driver to communicate with the NXP secure
enclave hardware. It provides an ioctl interface for userspace applications
to send commands, manage shared memory, and restricts invalid or disallowed
firmware commands.

Closes: https://sashiko.dev/#/patchset/20260805-imx-se-if-v34-0-7e4713d14e0a@nxp.com?part=5

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -15,13 +15,57 @@
>  
>  #define FW_DBG_DUMP_FIXED_STR		"ELE"
>  
> +int ele_uapi_allowed_base_cmd(struct se_if_priv *priv,
> +			      struct se_msg_hdr *header)
> +{
> +	switch (header->command) {
> +	case ELE_PING_REQ: return 0;
> +	case ELE_DEBUG_DUMP_REQ: return 0;
> +	case ELE_OEM_AUTH_CONTAINER_REQ: return 0;
> +	case ELE_OEM_VERIFY_IMAGE_REQ: return 0;
> +	case ELE_OEM_REL_CONTAINER_REQ: return 0;
> +	case ELE_FW_LIFE_CYCLE_REQ: return 0;
> +	case ELE_READ_FUSE_REQ: return 0;
> +	case ELE_GET_FW_VERS_REQ: return 0;
> +	case ELE_RETURN_LIFE_CYCLE_REQ: return 0;
> +	case ELE_GET_EVENT_REQ: return 0;
> +	case ELE_COMMIT_REQ: return 0;
> +	case ELE_GEN_KEY_BLOB_REQ: return 0;
> +	case ELE_GET_FW_STATUS_REQ: return 0;
> +	case ELE_XIP_DECRYPT_REQ: return 0;
> +	case ELE_WRITE_FUSE: return 0;
> +	case ELE_GET_INFO_REQ: return 0;
> +	case ELE_DEV_ATTEST_REQ: return 0;
> +	case ELE_WRITE_SHADOW_FUSE_REQ: return 0;
> +	case ELE_READ_SHADOW_FUSE_REQ: return 0;
> +	default:
> +		return -EACCES;
> +	}
> +}

[Severity: Critical]
Does this allowlist permit userspace to embed arbitrary DMA addresses without
kernel validation?

Commands like ELE_GET_INFO_REQ accept physical DMA addresses directly in
their payloads. Because se_chk_tx_msg_hdr() only validates the command header
against this list and performs no payload sanitization, could an unprivileged
user embed arbitrary physical addresses (e.g., kernel text) leading to the
hardware enclave overwriting arbitrary memory?

> 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
[ ... ]
> @@ -71,7 +122,7 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
> +		if (ret == -ERESTARTSYS) {
> +			/*
> +			 * Record that a signal was observed, then continue waiting non-
> +			 * interruptibly until the response arrives or the timeout
> +			 * expires. The caller can surface the interruption to userspace
> +			 * after the protocol transaction is brought back to a
> +			 * synchronized state.
> +			 */
> +			if (is_rsp_wait_with_timeout &&
> +			    READ_ONCE(se_clbk_hdl->rx_msg)) {
> +				WRITE_ONCE(se_clbk_hdl->signal_rcvd, true);
> +				wait_uninterruptible = true;
> +				continue;
> +			}
> +			break;
> +		}

[Severity: High]
Can this loop trap a process in an unkillable state for a very long time?

If a process is interrupted by a signal while waiting in ele_msg_rcv(), the
code converts the interruptible wait into an uninterruptible one by setting
wait_uninterruptible = true. Given that SE_RCV_MSG_LONG_TIMEOUT_MS is 5000
seconds, this could leave the task unkillable for over an hour, potentially
triggering the kernel's hung task watchdog.

> 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
[ ... ]
> +static void dlink_n_cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
> +{
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	if (is_fclose) {
> +		scoped_guard(mutex, &priv->modify_lock)
> +			dlink_dev_ctx(dev_ctx);
> +	}
> +
> +	cleanup_dev_ctx(dev_ctx, is_fclose);
> +}

[Severity: High]
Does unlinking dev_ctx before executing cleanup_dev_ctx() open a race
condition during driver unbind?

In se_if_probe_cleanup(), the unbind thread iterates over priv->dev_ctx_list
to gracefully close contexts. By unlinking the context here before the
cleanup completes, the unbind thread won't see it, won't block on fops_lock,
and might call mbox_free_channel(priv->tx_chan) prematurely. This could cause
the concurrent close thread to send on a freed channel in ele_msg_send().

[ ... ]
> +	rsp_status_err =
> +		se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg->header.command,
> +					cmd_snd_rcv_rsp_info.rx_buf_sz,
> +					tx_msg->header.ver == priv->if_defs->base_api_ver);
> +
> +	if (!rsp_status_err) {
> +		err = se_dev_ctx_cpy_out_data(dev_ctx);
> +		if (err < 0) {
> +			se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +			return err;
> +		}
> +	}
> +
> +	/* Copy data from the buffer */
> +	print_hex_dump_debug("to user ", DUMP_PREFIX_OFFSET, 4, 4, rx_msg,
> +			     cmd_snd_rcv_rsp_info.rx_buf_sz, false);
> +
> +	if (copy_to_user(u64_to_user_ptr(cmd_snd_rcv_rsp_info.rx_buf), rx_msg,
> +			 cmd_snd_rcv_rsp_info.rx_buf_sz)) {
> +		dev_err(priv->dev, "%s: Failed to copy to user.", dev_ctx->devname);
> +		err = -EFAULT;
> +	}
> +
> +	cleanup_err = se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +
> +	if (cleanup_err && !err)
> +		err = cleanup_err;
> +
> +	if (!err && !rsp_status_err)
> +		fw_api_specific_ops(dev_ctx, rx_msg);

[Severity: High]
Could this error handling sequence leak firmware session or storage handles?

If the firmware successfully executes an ELE_SESSION_OPEN_REQ, it allocates a
handle. However, if a user submits an intentionally truncated rx_buf_sz
(causing se_val_rsp_hdr_n_status() to fail) or a bad user pointer (causing
copy_to_user() to fail), the function skips calling fw_api_specific_ops().
This leaves the handle untracked by the kernel, preventing cleanup and
leaking hardware resources.

[ ... ]
> +/* Open a character device. */
> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
> +	struct miscdevice *miscdev = fp->private_data;
> +	struct se_if_open_gate *gate;
> +	struct se_if_device_ctx *misc_dev_ctx;
> +	struct se_if_device_ctx *dev_ctx;
> +	struct se_if_priv *priv;
> +	int err = 0;
> +
> +	gate = container_of(miscdev, struct se_if_open_gate, miscdev);
> +
> +	if (!se_if_open_gate_get(gate))
> +		return -ENODEV;

[Severity: High]
Is there a use-after-free risk if open() races with driver unbind?

When the module unbinds, se_if_probe_cleanup() calls misc_deregister(),
which does not fully synchronize with active open() calls. If the unbind
thread drops the final reference to priv in se_if_priv_release(),
se_if_open_gate_put() frees the gate object. A concurrent se_if_fops_open()
might then attempt to access the refcount of this already freed gate pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-imx-se-if-v35-0-11b25bb308ef@nxp.com?part=5