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

[sashiko-bot@kernel.org]

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?

> [ ... ]
> @@ -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.

> [ ... ]
> @@ -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.

> [ ... ]
> @@ -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