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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Freeing the DMA buffer on timeout in `ele_get_info` allows the firmware to corrupt memory if it completes the transaction late.
- [High] A time-of-check to time-of-use (TOCTOU) race on the volatile `jiffies` variable can cause an arithmetic underflow, leading to an infinite wait.
--

--- Patch [3]: [PATCH v32 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit a0eb4a58fa2ae5b51f82478e8f3c91165237467e
Author: Pankaj Gupta <[email protected]>

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave. NXP hardware
IP(s) for secure-enclaves like Edgelock Enclave(ELE), are embedded in the
SoC to support features like HSM, SHE & V2X using message based
communication interface.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 0000000000000..724f6e913ce79 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c

[ ... ]

> @@ -65,15 +65,15 @@ int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
>  	se_fill_cmd_msg_hdr(priv, (struct se_msg_hdr *)&tx_msg->header,
>  			    ELE_GET_INFO_REQ, ELE_GET_INFO_REQ_MSG_SZ, true);
>  
>  	tx_msg->data[0] = upper_32_bits(get_info_addr);
>  	tx_msg->data[1] = lower_32_bits(get_info_addr);
>  	tx_msg->data[2] = sizeof(*s_info);
>  	ret = ele_msg_send_rcv(priv, tx_msg, ELE_GET_INFO_REQ_MSG_SZ, rx_msg,
>  			       ELE_GET_INFO_RSP_MSG_SZ);
>  	if (ret < 0) {
>  		ele_get_info_cleanup(priv, get_info_data, get_info_addr, get_info_len);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
If the transaction times out and ele_msg_send_rcv() returns -ETIMEDOUT, does
freeing the DMA buffer here risk memory corruption?

The commit message mentions that "DMA free on timeout: -ETIMEDOUT means FW is
fenced by fw_busy, so the free is safe", but while fw_busy prevents the host
from sending new commands, it does not stop the firmware from finishing the
current command late and writing to the DMA address.

If the buffer is freed and reallocated elsewhere, a late DMA write could
silently corrupt memory.

>  		return ret;
>  	}
>  
>  	ret = se_val_rsp_hdr_n_status(priv, rx_msg, ELE_GET_INFO_REQ,
>  				      ELE_GET_INFO_RSP_MSG_SZ, true);

[ ... ]

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 0000000000000..f40eb4a07982d 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -61,16 +61,16 @@ 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)) {
>  				/* Deadline hit: fence hung FW, like the ret==0 path. */
>  				spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  				se_clbk_hdl->rx_msg = NULL;
>  				if (!completion_done(&se_clbk_hdl->done))
>  					atomic_set(&priv->fw_busy, 1);
>  				spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  				ret = -ETIMEDOUT;
>  				break;
>  			}
>  			remaining_jiffies = deadline_jiffies - jiffies;
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Since jiffies is volatile, the compiler will read it again here after the
time_after_eq() check. If a timer tick increments jiffies exactly between these
two reads, could this subtraction underflow?

An underflow would result in a massive timeout value near ULONG_MAX being
passed to the completion wait functions below, hanging the thread indefinitely.

>  		}
>  
>  		if (wait_uninterruptible)
>  			ret = wait_for_completion_timeout(&se_clbk_hdl->done,
>  							  remaining_jiffies);

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