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

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Sep 02, 2026 at 04:35:21PM +0000, sashiko-bot@kernel.org wrote:
> 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().

schedule_work() can be call at atomic context. move it and replace
"schedule_fw_busy_work = true" may resolve this problem, because you set
going_aways under spin_lock.

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


suppose should return fail to user

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

suppose it can be fixed after move schedule_work into lock

>
> [ ... ]
> > +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 size become meansless if firwmare can truncate it.

Frank

> > +	 * 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