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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Deadlock during unbind caused by `se_if_probe_cleanup()` blocking on `se_if_cmd_lock` before waking the active transaction.
- [High] Overwriting `dev_ctx->sess_hdl` and `dev_ctx->strg_hdl` on repeated open requests leaks previously opened firmware handles.
- [High] Mixing `goto`-based error handling with `__free()` annotations violates the cleanup subsystem guidelines.
- [High] Valid firmware responses during teardown are misidentified as forced wakeups, failing the teardown protocol and leaking DMA memory.
- [High] Missing bounds check on firmware response payload allows out-of-bounds read of the allocated buffer in `fw_api_specific_ops`.
--

--- Patch [5]: [PATCH v33 5/7] firmware: imx: adds miscdev ---
commit 505332d25775977b34724dbd94f2b51249b86386
Author: Pankaj Gupta <[email protected]>
Subject: firmware: imx: adds miscdev

This commit adds the character driver for the NXP secure-enclave
communication interface. It enables userspace applications to exchange
messages with the hardware IP, managing shared memory, command validation,
and firmware status.

Closes: https://sashiko.dev/#/patchset/[email protected]?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
[ ... ]
> @@ -128,6 +178,26 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
>  		}
> +
> +		/*
> +		 * A positive wait return normally means firmware delivered a
> +		 * response. During teardown, se_if_probe_cleanup() forces this
> +		 * wait to return via complete_all() without any real response.
> +		 * If that happens the firmware may still be executing and could
> +		 * DMA into the shared buffer later. Treat it as a failed
> +		 * transaction and arm the circuit breaker so the shared memory
> +		 * is quarantined (not freed) instead of being reclaimed while
> +		 * the enclave might still write to it.
> +		 */
> +		if (is_rsp_wait_with_timeout && atomic_read(&priv->going_away)) {
> +			spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
> +			se_clbk_hdl->rx_msg = NULL;
> +			se_mark_fw_busy(dev_ctx);
> +			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
> +			ret = -ENODEV;
> +			break;
> +		}

[Severity: High]
Does this logic unintentionally intercept legitimate teardown responses?

During se_if_probe_cleanup(), going_away is set to 1. The teardown path then
calls cleanup_dev_ctx(), which sends graceful close commands via
se_close_storage() and se_close_session(). These commands explicitly bypass
the going_away check in ele_msg_send_rcv() so they can be sent.

However, when the hardware replies, ele_msg_rcv() checks going_away and
misidentifies the genuine wakeup as the forced unbind abort, failing the
close operation and leaking the DMA memory.

> 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
[ ... ]
> @@ -339,31 +709,116 @@ static void se_if_probe_cleanup(void *plat_dev)
>  	if (!priv)
>  		return;
>  
> +	scoped_guard(mutex, &priv->se_if_cmd_lock) {
> +		/*
> +		 * Announce teardown before doing anything else. ele_msg_send_rcv()
> +		 * checks going_away under se_if_cmd_lock and bails out instead of
> +		 * arming a new transaction, and ele_msg_rcv() uses it to tell a
> +		 * teardown-forced completion apart from a genuine firmware response.
> +		 * This must be set before the complete_all() below so those checks
> +		 * are not dead code.
> +		 */
> +		atomic_set(&priv->going_away, 1);
> +		/*
> +		 * Wake any ioctl thread blocked in ele_msg_rcv() before iterating the
> +		 * device-context list. That waiter sleeps on this completion while
> +		 * holding its dev_ctx->fops_lock, and cleanup_dev_ctx() below also
> +		 * takes dev_ctx->fops_lock. Completing first lets the waiter make
> +		 * progress and drop fops_lock, avoiding an unbind hang.
> +		 */
> +		complete_all(&priv->waiting_rsp_clbk_hdl.done);
> +	}

[Severity: Critical]
Could this lead to a deadlock during unbind?

When an IOCTL calls ele_msg_send_rcv(), it holds se_if_cmd_lock across the
blocking wait in ele_msg_rcv(). If the device unbinds concurrently,
se_if_probe_cleanup() attempts to acquire the exact same se_if_cmd_lock
before executing complete_all() to wake up the IOCTL thread.

The unbind thread would block waiting for the lock, while the IOCTL thread
won't release it until it is woken up.

> 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
[ ... ]
> +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;

[Severity: High]
What happens if userspace sends multiple ELE_SESSION_OPEN_REQ or
ELE_STORAGE_OPEN_REQ ioctls on the same file descriptor?

The new handle is directly assigned to dev_ctx->sess_hdl, overwriting any
previous handle without closing it. Does this allow unprivileged userspace
programs to exhaust firmware resources by leaking previous handles?

[Severity: High]
Is it safe to unconditionally read rx_msg->data[1] here?

The rx_msg buffer is allocated using kzalloc based on the user-provided
rx_buf_sz in se_ioctl_cmd_snd_rcv_rsp_handler(). If userspace provides an
8-byte buffer size and the firmware responds with a truncated payload,
would this read past the end of the allocation and leak adjacent kernel
memory?

[ ... ]
> +int se_close_session(struct se_if_priv *priv, u32 session_hdl)
> +{
> +	struct se_api_msg *tx_msg __free(kfree) = NULL;
> +	struct se_api_msg *rx_msg __free(kfree) = NULL;
> +	int ret;
> +
> +	if (!priv || !priv->priv_dev_ctx) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}

[Severity: High]
Are there concerns with mixing goto-based error handling and scope-based
cleanup here?

The cleanup subsystem guidelines advise against using goto labels (like
goto exit) in the same function as __free annotations, as it can create
confusing ownership semantics and lead to double-free or resource leak bugs.
The same pattern is present in se_close_storage(). Would returning
directly be preferable?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5