Re: [PATCH v30 3/7] firmware: imx: add driver for NXP EdgeLock Enclave

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] DMA Use-After-Free: buffers are freed on timeout while the firmware may still be executing and writing to them, leading to silent memory corruption.
- [High] Returning -ERESTARTSYS after a hardware transaction has successfully completed causes non-idempotent commands to be re-executed and drops their results.
- [High] The `fw_busy` circuit breaker is bypassed when `ele_msg_rcv` times out via the `time_after_eq` check, leading to protocol desynchronization.
- [High] Data race in `ele_msg_send_rcv()` when setting up `rx_msg` allows spurious firmware responses to corrupt state and desynchronize the protocol.
- [Medium] `ele_get_info()` uses `gen_pool_dma_alloc()` to allocate an SRAM DMA buffer without zero-initializing it, potentially leaking sensitive data.
--

--- Patch [3]: [PATCH v30 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit e6e24144537ee5a010fb1fa57fe8270006013954
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
Subject: firmware: imx: add driver for NXP EdgeLock Enclave

This commit adds a MU-based communication interface for the NXP EdgeLock
secure enclave. The driver supports communication with the secure enclave
firmware over messaging units on SoCs like i.MX8ULP and i.MX93.

Closes: https://sashiko.dev/#/patchset/20260721-imx-se-if-v29-0-04a362f4fcca@nxp.com?part=3

> Documented as intentional, no code change (drivers/firmware/imx/ele_common.c):
> - [Medium] "Unsafe syscall restart on completed non-idempotent hardware
>   operations" in ele_msg_send_rcv():
>   This is intentional protocol-synchronization behavior, not a
>   re-execution bug. A signal is deliberately not acted on while a firmware
>   message exchange is in progress; ele_msg_rcv() defers it until the
>   response path completes and the FW/kernel protocol state is synchronized
>   again, so Linux and firmware never diverge on message ownership. Only
>   after synchronization is the interrupted wait surfaced to userspace via
>   -ERESTARTSYS. The command/response ABI must treat this as an interrupted
>   operation after synchronization; it is not permission for the kernel
>   driver to re-send the command, so the enclave never receives it twice.
>   Clarified with an expanded code comment.

[Severity: High]
Is this understanding of -ERESTARTSYS accurate?

Returning -ERESTARTSYS to userspace after a hardware transaction has
successfully completed causes the kernel's architecture-specific signal
handling to automatically restart the system call. This transparently forces
the userspace application to re-execute the exact same command.

For non-idempotent operations, won't the hardware execute the command twice,
discarding the successful result of the first execution?

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> new file mode 100644
> index 0000000000000..a2ede9c01691d
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -0,0 +1,277 @@
[ ... ]
> +static void ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf, dma_addr_t d_addr,
> +				 size_t size)
> +{
> +	if (priv->mem_pool)
> +		gen_pool_free(priv->mem_pool, (unsigned long)buf, size);
> +	else
> +		dma_free_coherent(priv->dev, size, buf, d_addr);
> +}

[Severity: Critical]
Is there a risk of a DMA use-after-free here?

If ele_msg_send_rcv() times out, this cleanup function is called immediately
to free the DMA buffer. However, the unresponsive hardware is not reset and
might still complete the operation slightly later, performing a DMA write to
the freed physical address.

Can this corrupt unrelated kernel memory or cause an IOMMU fault if the
memory has been reallocated to another subsystem?

[ ... ]
> +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
> +{
[ ... ]
> +	if (priv->mem_pool)
> +		get_info_data = gen_pool_dma_alloc(priv->mem_pool,
> +						   ELE_GET_INFO_BUFF_SZ,
> +						   &get_info_addr);

[Severity: Medium]
Does this allocation need to be zero-initialized?

Memory allocated from an SRAM pool via gen_pool_dma_alloc() is not
guaranteed to be zeroed. If the firmware returns a success status but only
partially writes the ele_dev_info structure, the subsequent memcpy() will
copy the uninitialized SRAM contents into s_info:

	memcpy(s_info, get_info_data, sizeof(*s_info));

Could this leak sensitive data from previous hardware operations to
userspace when these fields are exposed?

[ ... ]
> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> new file mode 100644
> index 0000000000000..f9be8b8556ad8
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -0,0 +1,480 @@
[ ... ]
> +int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl)
> +{
[ ... ]
> +	do {
> +		if (is_rsp_wait_with_timeout) {
> +			if (time_after_eq(jiffies, deadline_jiffies)) {
> +				ret = -ETIMEDOUT;
> +				break;
> +			}
> +			remaining_jiffies = deadline_jiffies - jiffies;
> +		}

[Severity: High]
Are we bypassing the fw_busy circuit breaker here?

When a signal interrupts the wait, the loop iterates again. If the deadline
has already passed, this path breaks out of the loop and returns -ETIMEDOUT
without executing the block below that sets atomic_set(&priv->fw_busy, 1).

If the circuit breaker is not tripped, will the driver incorrectly allow new
commands to be sent while the firmware is still processing the old one,
potentially copying a late response into the buffer of the new command?

[ ... ]
> +int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
> +		     void *rx_msg, int exp_rx_msg_sz)
> +{
> +	int err;
> +
> +	guard(mutex)(&priv->se_if_cmd_lock);
> +
> +	if (atomic_read(&priv->fw_busy)) {
> +		dev_dbg(priv->dev, "ELE became unresponsive.\n");
> +		return -EBUSY;
> +	}
> +	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
> +	priv->waiting_rsp_clbk_hdl.rx_msg_sz = exp_rx_msg_sz;
> +	priv->waiting_rsp_clbk_hdl.rx_msg = rx_msg;

[Severity: High]
Is there a data race when setting up these variables locklessly?

The commit message notes that the read side in se_if_rx_callback() was moved
inside the clbk_rx_lock. However, if the write side here is lockless, the
compiler or CPU can reorder these updates.

If a concurrent or delayed hardware interrupt triggers se_if_rx_callback(),
could it acquire the lock and observe a non-NULL rx_msg with a stale
rx_msg_sz of 0 from a previous transaction's cleanup? This would cause the
interrupt handler to copy 0 bytes and call complete(), leading the kernel to
process an unwritten buffer.

[ ... ]
> +	err = ele_msg_rcv(priv, &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]
Does discarding a successful hardware response and returning -ERESTARTSYS
here cause unintended re-execution?

As noted regarding the commit message, the kernel will automatically restart
the system call when returning to userspace. If the hardware successfully
completed a non-idempotent operation, won't this discard the result and
force the system call to be re-executed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-imx-se-if-v30-0-ce8ba256692c@nxp.com?part=3