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

On Thu, Aug 06, 2026 at 11:27:24AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 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/[email protected]?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?

This one may security hole, you need check payload to make sure DMA
address is in ranges.

>
> > 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.


This one need double check.

Other high problem need your judgement.

Frank

>
> > 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/[email protected]?part=5