Re: [PATCH 2/7] firmware: imx: ele: Correct ele_fw_authenticate API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Dangling pointers left in the global callback handle on error paths lead to Use-After-Free heap corruption or a NULL pointer dereference when a delayed/spurious response arrives.
- [Critical] Uninitialized physical address used for IMEM export/import.
- [High] Silent physical address truncation in `ele_service_swap()`.
- [High] The `ele_msg_rcv()` function causes an infinite busy loop (CPU lockup) if the calling task receives a signal.
--

commit f74ec8f682c5afa2812159200b4566d0c747b041
Author: Peng Fan <peng.fan@nxp.com>
Subject: firmware: imx: ele: Correct ele_fw_authenticate API

This commit corrects the ele_fw_authenticate API by enforcing that the upper
32 bits of both the container and image addresses are zero. It fails the
command if they are not, and correctly populates only the lower 32 bits in
the message data.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 23f68076b85ce..f6346f1545096 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -208,6 +208,11 @@ int ele_fw_authenticate(struct se_if_priv *priv, phys_addr_t contnr_addr,
>  	if (!priv)
>  		return -EINVAL;
>  
> +	if (upper_32_bits(contnr_addr) || upper_32_bits(img_addr)) {
> +		dev_err(priv->dev, "Wrong address: %pap %pap\n", &contnr_addr, &img_addr);
> +		return -EINVAL;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but while you are correctly adding bounds
checks here, does ele_service_swap() also need a similar fix?

It appears ele_service_swap() accepts a 64-bit phys_addr_t and silently
truncates it by assigning only lower_32_bits(addr) to the message, ignoring
the upper 32 bits. On 64-bit systems, could passing physical or dma addresses
above 4gb instruct the firmware to export or import data to the wrong
physical memory area, leading to memory corruption?

[Severity: Critical]
This isn't a bug introduced by this patch, but regarding physical addresses,
is there an uninitialized address used for imem export/import?

In se_if_probe(), a buffer for the encrypted imem is allocated via
dmam_alloc_coherent(), which assigns the generated dma address to a local
variable imem_dma_addr. However, it seems this address is never stored back
into the load_fw->imem.phyaddr struct field, leaving it zero.

During suspend/resume, could se_save_imem_state() pass this zero address to
ele_service_swap(), instructing the secure enclave firmware to overwrite
physical address 0x0 with its state?

>  	struct se_api_msg *tx_msg __free(kfree)	=
>  		kzalloc(ELE_FW_AUTH_REQ_SZ, GFP_KERNEL);

[ ... ]

> @@ -224,8 +229,8 @@ int ele_fw_authenticate(struct se_if_priv *priv, phys_addr_t contnr_addr,
>  		return ret;
>  
>  	tx_msg->data[0] = lower_32_bits(contnr_addr);
> -	tx_msg->data[1] = upper_32_bits(contnr_addr);
> -	tx_msg->data[2] = img_addr;
> +	tx_msg->data[1] = 0;
> +	tx_msg->data[2] = lower_32_bits(img_addr);
>  
>  	ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_FW_AUTH_REQ_SZ, rx_msg,
>  			       ELE_FW_AUTH_RSP_MSG_SZ);

[Severity: Critical]
This is a pre-existing issue, but since ele_msg_send_rcv() is called here,
could dangling pointers left in the global callback handle on error paths
lead to use-after-free heap corruption or a null pointer dereference?

In ele_msg_send_rcv(), rx_msg and dev_ctx are stored in
priv->waiting_rsp_clbk_hdl. If ele_msg_send() fails, the function returns
without clearing them, and the caller frees the heap-allocated rx_msg.

If a delayed or spurious response arrives later, could the asynchronous
se_if_rx_callback() use the stale handle to blindly copy payload data into
the freed rx_msg buffer?

Furthermore, if ele_msg_rcv() is interrupted by a signal, it sets dev_ctx to
null but leaves rx_msg, which could lead se_if_rx_callback() to dereference
a null pointer when accessing se_clbk_hdl->dev_ctx->devname.

[Severity: High]
This is another pre-existing issue, but does ele_msg_rcv() have the
potential to cause an infinite busy loop if the calling task receives a
signal?

It uses wait_for_completion_interruptible_timeout(), which returns
-ERESTARTSYS immediately if a signal is pending. In this case, the code
sets signal_rcvd = true and uses continue to restart the loop without
clearing the signal or returning.

Since the signal remains pending, the next call to the wait function returns
-ERESTARTSYS immediately again without sleeping. If the hardware never
responds, could this indefinitely hang the cpu core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-ele-v1-v1-0-a9570c4bffc9@nxp.com?part=2