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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Mismatch in API version parameters when sending and validating session/storage close requests, causing the teardown cleanup to unconditionally fail.
- [High] Firmware storage handle is permanently leaked if command receiver registration fails after a successful ELE_STORAGE_OPEN_REQ.
- [High] Missing payload validation allows cross-process session/storage disruption and handle leaks via spoofed close requests.
- [High] Concurrent close() can race with device unbind, leading to a use-after-free of mailbox channels in mbox_send_message because se_close_session bypasses the going_away safeguard.
- [Medium] Discarding successful firmware responses on interrupted wait causes firmware resource leaks.
--

--- Patch [5]: [PATCH v34 5/7] firmware: imx: adds miscdev ---
commit 4fe94a5b9918db6ab33badde78a9ca4fc596c70c
Author: Pankaj Gupta <[email protected]>
Subject: firmware: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, that enables
exchanging messages with NXP secure enclave HW IP(s). Includes command
validation, session tracking, and teardown management.

Link: https://lore.kernel.org/r/[email protected]

> 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
[ ... ]
> @@ -288,6 +288,14 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  	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: Medium]
Is it possible this discards a successful firmware response?

In ele_msg_send_rcv(), if a signal interrupted the wait but the firmware
successfully processed a request (like ELE_SESSION_OPEN_REQ), err is
converted to -ERESTARTSYS. This causes se_ioctl_cmd_snd_rcv_rsp_handler()
to return -EINTR without calling fw_api_specific_ops(). As a result, the
successfully allocated session handle is never recorded by the kernel and
gets leaked permanently in the firmware.

> diff --git a/drivers/firmware/imx/ele_fw_api.c b/drivers/firmware/imx/ele_fw_api.c
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_fw_api.c
[ ... ]
> @@ -46,6 +46,10 @@ int ele_uapi_allowed_fw_cmd(struct se_if_device_ctx *dev_ctx, struct se_msg_hdr
>  	case ELE_SESSION_CLOSE_REQ:
>  		/* Might be cleared as part of tear down. */
>  		ret = dev_ctx->sess_hdl ? 0 : -ENXIO;
>  		break;

[Severity: High]
Does this validation allow one process to close another process's handle?

In ele_uapi_allowed_fw_cmd(), the code only verifies that the calling
context has some handle open (dev_ctx->sess_hdl is non-zero). It does not
check if the handle ID provided in the user's payload (tx_msg->data[0])
actually matches the context's dev_ctx->sess_hdl. A malicious user with one
valid handle could send a spoofed close request with another process's handle
ID, disrupting the victim and leaking their own handle.

[ ... ]
> @@ -79,15 +83,16 @@ void fw_api_specific_ops(struct se_if_device_ctx *dev_ctx, struct se_api_msg *rx
>  	case ELE_STORAGE_OPEN_REQ: {
>  		int rc = 0;
>  
>  		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;
>  		}
>  		dev_ctx->strg_hdl = rx_msg->data[1];
>  		break;
>  	}

[Severity: High]
Can this leak a newly opened storage handle on registration failure?

If set_dev_ctx_as_command_receiver() fails (e.g. returning -EBUSY because a
receiver is already registered), the code breaks early and skips assigning
rx_msg->data[1] to dev_ctx->strg_hdl. Because the ioctl still returns success
to userspace and dev_ctx->strg_hdl remains 0, the kernel will not track the
handle for teardown, resulting in a permanent firmware resource leak.

[ ... ]
> @@ -113,19 +118,22 @@ int se_close_session(struct se_if_priv *priv, u32 session_hdl)
>  
>  	se_fill_cmd_msg_hdr(priv, (struct se_msg_hdr *)&tx_msg->header,
>  			    ELE_SESSION_CLOSE_REQ, ELE_SESSION_CLOSE_REQ_SZ, true);
>  
>  	tx_msg->data[0] = session_hdl;
>  
>  	ret = ele_msg_send_rcv(priv->priv_dev_ctx,
>  			       tx_msg,
>  			       ELE_SESSION_CLOSE_REQ_SZ,
>  			       rx_msg,
>  			       ELE_SESSION_CLOSE_RSP_SZ);

[Severity: High]
Could this cause a use-after-free of mailbox channels during device unbind?

By hardcoding priv->priv_dev_ctx here instead of using the user's actual
device context, this unconditionally bypasses the teardown safeguard in
ele_msg_send_rcv() (which allows priv_dev_ctx teardown messages through).
If a user calls close() concurrently with driver unbind, and the unbind
thread frees priv->tx_chan, the concurrent close() thread will skip the
going_away check and attempt a transmission on the freed channel.

>  	if (ret < 0)
>  		return ret;
>  
>  	ret = se_val_rsp_hdr_n_status(priv,
>  				      rx_msg,
>  				      ELE_SESSION_CLOSE_REQ,
>  				      ELE_SESSION_CLOSE_RSP_SZ,
>  				      false);
>  	return ret;
>  }

[Severity: High]
Is there a version mismatch causing cleanup to unconditionally fail?

When formatting the request earlier in se_close_session(),
se_fill_cmd_msg_hdr() is called with is_base_api = true. But here,
se_val_rsp_hdr_n_status() is called with is_base_api = false, which strictly
expects the response version to match fw_api_ver. This mismatch causes
validation to fail with -EINVAL, leaking firmware resources since the kernel
considers the close operation to have failed. This identical mismatch also
exists in se_close_storage().

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