Re: [PATCH v46 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:
- [High] Kernel heap out-of-bounds read when processing firmware responses due to trusting the firmware-supplied `header.size` over the actual user-specified allocation size.
- [High] Concurrent `ELE_STORAGE_OPEN_REQ` commands bypass the command-receiver exclusivity check, allowing a process to intercept another's NVM traffic.
- [High] Use-after-free on `priv` object due to premature `cancel_work_sync()` before `mbox_free_channel()` allows `schedule_work()` to queue work on a freed object.
- [High] Syscall returning `-EINTR` after hardware operation is fully completed leads to lost response payloads, corrupted retries, and broken state synchronization.
- [Low] Mixing `goto`-based error handling with `scoped_guard()` declarations in `se_if_probe_cleanup`.
--

commit afeff3ca68cac56d7230f74585096a1ade46c177
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: adds miscdev

This commit adds the character driver interface to the NXP secure-enclave,
allowing user-space applications to exchange messages with hardware IPs like
EdgeLock Enclave.

Link: https://sashiko.dev/#/patchset/20260902-imx-se-if-v45-0-6e400879d8ec@nxp.com?part=5

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 10fb4f60b4293..1e7ee8100f846 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> @@ -377,9 +701,24 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
>  		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  		if (!se_clbk_hdl->rx_msg) {
> -			/* Close circuit breaker on spinlock race */
> -			atomic_set(&priv->fw_busy, 0);
> +			/*
> +			 * Only schedule fw_busy_work when going_away is clear.
> +			 * se_if_probe_cleanup() sets going_away under
> +			 * clbk_rx_lock before calling cancel_work_sync(). If
> +			 * going_away is already set here, teardown has already
> +			 * run (or is running) cancel_work_sync(); scheduling
> +			 * the work again after that point would re-queue it
> +			 * against the freed priv object, causing a
> +			 * use-after-free when the work executes.
> +			 */
> +			if (atomic_read(&priv->fw_busy) &&
> +			    !atomic_read(&priv->going_away))
> +				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);

[Severity: High]
Does this code risk a use-after-free on the priv object? The spinlock is
explicitly dropped before schedule_work() is called, which widens the race
window. If cancel_work_sync() runs concurrently in se_if_probe_cleanup()
while the lock is dropped here, it will return immediately since the work
hasn't been scheduled yet, allowing the object to be freed before this thread
calls schedule_work().

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> new file mode 100644
> index 0000000000000..72cecc5c8e258
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_fw_api.c
[ ... ]
> +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;
> +		if (rx_msg_sz < ELE_STORAGE_OPEN_RSP_SZ)
> +			ret = -EINVAL;
> +		break;

[Severity: High]
Could concurrent ELE_STORAGE_OPEN_REQ commands bypass this exclusivity check?
The lock is dropped immediately after checking
priv->cmd_receiver_clbk_hdl.dev_ctx, which creates a race window before the
command receiver is actually registered. This might allow a secondary process
to intercept the NVM traffic of another.

[ ... ]
> +void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct se_api_msg *rx_msg,
> +			 bool is_cmd_interrupted)
> +{
[ ... ]
> +	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];
> +
> +		if (is_cmd_interrupted) {
> +			if (se_close_storage(dev_ctx, dev_ctx->strg_hdl))
> +				dev_err(dev_ctx->priv->dev, "failed to close storage.\n");
> +			dev_ctx->strg_hdl = 0;
> +			break;
> +		}
> +
> +		rc = set_dev_ctx_as_command_receiver(dev_ctx);
> +		if (rc)
> +			dev_err(priv->dev,
> +				"Failed to register %s as CMD-Receiver: %d\n",
> +				dev_ctx->devname, rc);
> +		break;
> +	}

[Severity: High]
If registration fails due to the race mentioned above (rc != 0), is it safe
to only log an error without closing the storage handle and without returning
the error to the user? The ioctl will successfully return the valid storage
handle to the second process.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 0792e5152436b..baeb28929b2de 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> @@ -341,31 +719,144 @@ static void se_if_probe_cleanup(void *plat_dev)
>  	if (!priv)
>  		return;
[ ... ]
> +	while (true) {
> +		dev_ctx = NULL;
> +
> +		scoped_guard(mutex, &priv->modify_lock) {
> +			if (list_empty(&priv->dev_ctx_list))
> +				goto out_done;

[Severity: Low]
Does mixing goto-based error handling with scoped_guard() macros violate the
subsystem's LIFO cleanup definition rules? This can cause confusing ownership
semantics and can lead to resource leaks or double frees.

> +
> +			dev_ctx = list_first_entry(&priv->dev_ctx_list,
> +						   struct se_if_device_ctx, link);
> +
> +			/* pin this context so close() cannot free it under us */
> +			kref_get(&dev_ctx->refcount);
> +			dlink_dev_ctx(dev_ctx);
> +		}
> +
> +		/*
> +		 * Local cleanup outside the global lock avoids ABBA deadlock
> +		 * with paths that already take dev_ctx->fops_lock first.
> +		 */
> +		cleanup_dev_ctx(dev_ctx, false);
> +		kref_put(&dev_ctx->refcount, se_if_dev_ctx_release);
> +	}
> +out_done:
> +
> +	/*
> +	 * Cancel fw_busy_work before acquiring se_if_cmd_lock. The work
> +	 * handler, se_clear_fw_busy(), acquires dev_ctx->fops_lock. A
> +	 * concurrent close() may hold fops_lock and then attempt to acquire
> +	 * se_if_cmd_lock via se_close_storage(). Calling cancel_work_sync()
> +	 * while already holding se_if_cmd_lock would therefore deadlock all
> +	 * three threads. Canceling first, without any lock held, lets the
> +	 * work and the close() finish independently.
> +	 *
> +	 * going_away was set under clbk_rx_lock earlier in this function.
> +	 * Once set, se_if_rx_callback() will not call schedule_work() again,
> +	 * so the work cannot be re-queued after cancel_work_sync() returns.
> +	 */
> +	cancel_work_sync(&priv->fw_busy_work);
> +	/*
> +	 * Free the mailbox channels under se_if_cmd_lock. ele_msg_send_rcv()
> +	 * holds se_if_cmd_lock for the full duration of a synchronous
> +	 * transaction, including the mbox_send_message() call on priv->tx_chan.
> +	 * going_away was set above and complete_all() has already woken any
> +	 * in-flight waiter, so any transaction in progress will unwind to
> +	 * -ENODEV and release the lock quickly. Acquiring se_if_cmd_lock here
> +	 * guarantees no caller is still touching the channels when they are
> +	 * freed, and nulling the pointers under the lock prevents any sender
> +	 * that races past the going_away check from accessing a freed channel.
> +	 */
> +	scoped_guard(mutex, &priv->se_if_cmd_lock) {
> +		if (priv->rx_chan) {
> +			mbox_free_channel(priv->rx_chan);

[Severity: High]
Could the premature cancel_work_sync() allow schedule_work() to queue work
on a freed object? Moving cancel_work_sync() after mbox_free_channel() would
guarantee no new work could be scheduled from the rx callback.

[ ... ]
> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	err = ele_msg_send_rcv(dev_ctx, tx_msg, cmd_snd_rcv_rsp_info.tx_buf_sz,
> +			       rx_msg, cmd_snd_rcv_rsp_info.rx_buf_sz);
> +	if (err < 0) {
> +		/*
> +		 * -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)) {
> +				se_dev_ctx_cpy_out_data(dev_ctx);
> +				fw_api_specific_ops(dev_ctx, rx_msg, true);
> +			}
> +			/*
> +			 * 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;
> +		}
> +
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +
> +		return err;
> +	}

[Severity: High]
Does returning -EINTR here without copying the rx_msg payload back to
userspace cause data loss or corrupted retries? If the hardware operation
completed successfully but was interrupted by a signal, masking success with
an error forces userspace to discard a valid payload and potentially retry,
which can lead to cryptographic IV reuse or leaked handles.

[ ... ]
> +	/*
> +	 * Validate using the size the firmware declared in the response header
> +	 * rather than cmd_snd_rcv_rsp_info.rx_buf_sz (the amount actually
> +	 * received, clamped to the caller's buffer). If the caller supplied a
> +	 * buffer smaller than the firmware's full response, rx_buf_sz reflects
> +	 * the truncated copy and se_val_rsp_hdr_n_status() would fail the size
> +	 * check, causing fw_api_specific_ops() to be skipped and any freshly
> +	 * allocated session/storage handle to go unrecorded. Using the
> +	 * firmware-declared size ensures a well-formed response is always
> +	 * recognised and its handle is tracked for cleanup.
> +	 *
> +	 * Any size discrepancy between the firmware response header and the
> +	 * userspace-supplied buffer is already logged by the mailbox receive
> +	 * callback before control returns here.
> +	 */
> +	rsp_status_err =
> +		se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg->header.command,
> +					rx_msg->header.size << 2, tx_msg->header.ver);

[Severity: High]
Does trusting the firmware-supplied header.size instead of the actual user
allocation size cause an out-of-bounds read? Since se_val_rsp_hdr_n_status()
reads from rx_msg->data[0] assuming the buffer size matches the firmware
size, a maliciously small user allocation could lead to out-of-bounds access
on the kernel heap.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-imx-se-if-v46-0-aefaab525034@nxp.com?part=5