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

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

On Mon, Aug 24, 2026 at 05:50:10PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [High] TOCTOU race condition in ELE_STORAGE_OPEN_REQ allows NVM traffic interception
> - [High] Firmware session handles permanently leak if a process exits via a fatal signal due to the fw_busy circuit breaker
> - [High] SE_IOCTL_ENABLE_CMD_RCV_STATUS unconditionally returns -ENOMEM, rendering it broken
> - [High] Missing bounds validation for SE_CMD_ADDR_NO_SIZE buffers allows arbitrary out-of-bounds DMA reads
> - [High] Use-After-Free of priv due to fw_busy_work being scheduled after cancel_work_sync
> - [High] Mutexes initialized via devm_mutex_init() are destroyed before se_if_probe_cleanup() executes
> - [High] Successfully received firmware responses are dropped on non-fatal signals, leaking firmware resources
> - [Medium] SE_IOCTL_GET_MU_INFO returns hardcoded zeroes instead of enclave identification data
> --
>
> --- Patch [5]: [PATCH v42 5/7] firmware: imx: adds miscdev ---
> commit 2e1b01a2a813fca5bf21f8f8fd015fe9ea573491
> Author: Pankaj Gupta <pankaj.gupta@nxp.com>
> Subject: firmware: imx: adds miscdev
>
> This patch adds a character driver to communicate with NXP secure enclaves
> (such as EdgeLock) from user-space. It introduces an ioctl ABI, shared memory
> management, and validation checks for firmware commands.
>
> > diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> > index 0000000000000..0000000000000
> > --- a/drivers/firmware/imx/ele_common.c
> > +++ b/drivers/firmware/imx/ele_common.c
> > [ ... ]
> > @@ -546,6 +546,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
> >  	err = ele_msg_rcv(dev_ctx, &priv->waiting_rsp_clbk_hdl);
> >
> >  	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
> >  		/*
> >  		 * Signal was deferred until the FW/kernel protocol resynchronized.
> >  		 * On success report -ERESTARTSYS for the interrupted wait; the
> >  		 * command is not re-sent. Keep real errors like -ETIMEDOUT.
> >  		 */
> >  		if (err > 0)
> >  			err = -ERESTARTSYS;
>
> [Severity: High]
> Could this leak firmware resources when a non-fatal signal arrives?
> If a firmware response successfully arrives but defers a non-fatal signal,
> this code overwrites the positive success size with -ERESTARTSYS.
> Then in se_ioctl_cmd_snd_rcv_rsp_handler(), this is converted to -EINTR
> and returns early without copying the response payload to userspace.
> Because userspace never receives the newly generated handles (e.g., keys
> or contexts), it cannot issue cleanup commands.
>
> > [ ... ]
> > @@ -660,11 +660,11 @@ 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) {
> >  			if (atomic_read(&priv->fw_busy))
> >  				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]
> Could this lead to a use-after-free of the priv object?
> In se_if_probe_cleanup(), cancel_work_sync() is called to wait for and cancel
> fw_busy_work before the rx_chan mailbox is disabled. If a late firmware
> response arrives in this window, this callback will execute. Because the
> callback schedules the work unconditionally if fw_busy is set without checking
> going_away, the work could be re-queued after cancel_work_sync() returns,
> causing it to access the freed priv object when the unbind finishes.
>
> > diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> > index 0000000000000..0000000000000
> > --- a/drivers/firmware/imx/ele_fw_api.c
> > +++ b/drivers/firmware/imx/ele_fw_api.c
> > [ ... ]
> > @@ -155,10 +155,10 @@ int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr *he
> >  		/*
> >  		 * Reject the storage-open request when another context is
> >  		 * already registered as the command receiver.
> >  		 */
> >  		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;
>
> [Severity: High]
> Can this create a TOCTOU race condition?
> The modify_lock is dropped immediately after checking the receiver
> availability, opening a window before the command reaches firmware.
> If two threads concurrently send ELE_STORAGE_OPEN_REQ, both could pass this
> check and receive valid storage handles.
>
> > [ ... ]
> > @@ -244,11 +244,11 @@ void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct se_api_msg *rx
> >  		rc = set_dev_ctx_as_command_receiver(dev_ctx, false);
> >  		if (rc)
> >  			dev_err(priv->dev,
> >  				"Failed to register %s as CMD-Receiver: %d\n",
> >  				dev_ctx->devname, rc);
> >  		break;
> >  	}
>
> [Severity: High]
> Following the race condition above, if one thread successfully registers as the
> command receiver but the other fails here, the failure to register only logs
> an error and returns void. Does this allow the ioctl to return success, routing
> the second thread's firmware callbacks to the first thread and allowing
> interception of NVM traffic?
>
> > diff --git a/drivers/firmware/imx/ele_msg_addr_field.c b/drivers/firmware/imx/ele_msg_addr_field.c
> > index 0000000000000..0000000000000
> > --- a/drivers/firmware/imx/ele_msg_addr_field.c
> > +++ b/drivers/firmware/imx/ele_msg_addr_field.c
> > [ ... ]
> > @@ -51,7 +51,7 @@ static const struct se_cmd_addr_field ele_oem_auth_cntr_addr_fields[] = {
> >  	 * Output buffers must be fully bounded; input-only buffers are safe with
> >  	 * addr-only checks.
> >  	 */
> >  	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
> >  	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* container_hdr_addr */
> >  };
>
> [Severity: High]
> Can this unbounded input buffer allow arbitrary out-of-bounds DMA reads?
> The descriptor specifies SE_CMD_ADDR_NO_SIZE and a zero buf_size, which
> bypasses end-bounds checks. If a malicious userspace process places the buffer
> at the very end of the shared memory window and crafts a header with a huge
> size field, couldn't the firmware read beyond the mapped shared memory window
> into unmapped space and trigger AXI bus faults?
>
> > [ ... ]
> > @@ -138,7 +138,7 @@ static const struct se_cmd_addr_field ele_keystore_reprov_en_addr_fields[] = {
> >  	 * memory the caller already owns. Output buffers must be fully bounded;
> >  	 * input-only buffers are safe with addr-only checks.
> >  	 */
> >  	{ .lsb_idx = 1, .msb_idx = 0, .has_msb = true, .flag_idx = SE_CMD_ADDR_ALWAYS,
> >  	  .size_idx = SE_CMD_ADDR_NO_SIZE },			/* signed_msg_addr */
> >  };
>
> [Severity: High]
> This appears to be another instance where an unbounded input buffer might
> allow arbitrary sized DMA reads by the firmware. Is there a way to safely
> bound this input payload?
>
> > diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> > index 0000000000000..0000000000000
> > --- a/drivers/firmware/imx/se_ctrl.c
> > +++ b/drivers/firmware/imx/se_ctrl.c
> > [ ... ]
> > @@ -746,10 +746,10 @@ static void se_if_probe_cleanup(void *plat_dev)
> >  	while (true) {
> >  		dev_ctx = NULL;
> >
> >  		scoped_guard(mutex, &priv->modify_lock) {
> >  			if (list_empty(&priv->dev_ctx_list))
> >  				goto out_done;
>
> [Severity: High]
> Is there a devres initialization order issue here?
> Because devres actions execute in reverse order (LIFO), modify_lock will be
> destroyed before se_if_probe_cleanup() is invoked, since it was initialized
> after devm_add_action_or_reset. When the cleanup function later attempts
> to lock this destroyed mutex, could this lead to undefined behavior or
> crashes during device unbind?

Please check this

>
> > [ ... ]
> > @@ -1151,9 +1151,9 @@ int set_dev_ctx_as_command_receiver(struct se_if_device_ctx *dev_ctx, bool is_io
> >  	if (se_clbk_hdl->dev_ctx)
> >  		return -EBUSY;
> >
> >  	if (!dev_ctx->strg_hdl)
> >  		return -EINVAL;
> >
> >  	if (is_ioctl)
> >  		return -ENOMEM;
>
> [Severity: High]
> Does this check render the SE_IOCTL_ENABLE_CMD_RCV_STATUS ioctl broken?
> When se_ioctl() invokes this function with is_ioctl set to true, it will
> unconditionally return -ENOMEM here and the command receiver will never be
> registered via the API.

Please check this

>
> > [ ... ]
> > @@ -1532,9 +1532,9 @@ static int se_ioctl_get_mu_info(struct se_if_device_ctx *dev_ctx,
> >  	struct se_if_node *if_node;
> >  	int err = 0;
> >
> >  	if_node = container_of(priv->if_defs, typeof(*if_node), if_defs);
> >
> >  	if_info.se_if_id = 0;
> >  	if_info.interrupt_idx = 0;
> >  	if_info.tz = 0;
> >  	if_info.did = 0;
> >  	if_info.cmd_tag = priv->if_defs->cmd_tag;
>
> [Severity: Medium]
> Are these structural identification fields intentionally populated with
> hardcoded zeroes? Setting se_if_id, interrupt_idx, tz, and did to 0 prevents
> userspace from dynamically identifying the secure enclave's type or
> configuration.

Is it true?

Frank
>
> > [ ... ]
> > @@ -1614,11 +1614,11 @@ static void cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
> >  				struct se_if_device_ctx *tx_ctx = is_fclose ? dev_ctx :
> >  							dev_ctx->priv->priv_dev_ctx;
> >
> >  				if (dev_ctx->strg_hdl && se_close_storage(tx_ctx,
> >  									  dev_ctx->strg_hdl))
> >  					dev_err(dev_ctx->priv->dev, "failed to close storage.\n");
> >  				if (dev_ctx->sess_hdl && se_close_session(tx_ctx,
> >  									  dev_ctx->sess_hdl))
> >  					dev_err(dev_ctx->priv->dev, "failed to close session.\n");
> >  			}
>
> [Severity: High]
> Will firmware session handles leak if a process exits via a fatal signal?
> If se_close_storage() encounters a pending SIGKILL, its wait_for_completion
> aborts instantly and arms the fw_busy circuit breaker.
>
> When se_close_session() runs immediately afterward, it sees fw_busy is set
> and aborts the session close command without sending it. Does this permanently
> leak the session handle on the secure enclave?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260825-imx-se-if-v42-0-2e8efac0bb16@nxp.com?part=5