Re: [PATCH 1/7] firmware: imx: ele: Correct check condition in se_if_rx_callback

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch alters the condition for size mismatch handling by negating `check_hdr_exception_for_sz()`, inadvertently skipping the essential size truncation for exception commands and causing an out-of-bounds read and information leak.
- [Medium] The commit message indicates that the patch should return a failure when the size does not match for non-exception APIs, but the code only logs the error and proceeds.

Pre-existing issues:
- [Critical] Asynchronous mailbox callbacks can trigger a Null Pointer Dereference or cross-thread data corruption if the completion wait times out, is interrupted, or if unsolicited messages arrive.
- [Critical] The IOCTL handler directly dereferences a user-space pointer during header validation, causing memory safety violations and exposing a TOCTOU vulnerability.
- [High] In NVM command handling, the received message size is unconditionally overwritten after being clamped, leading to a heap buffer overflow.
--

commit dbd468c11e8d6f15f1dbfe05daf128166c6de3a4
Author: Peng Fan <peng.fan@nxp.com>

firmware: imx: ele: Correct check condition in se_if_rx_callback

This commit updates the response validation logic in the EdgeLock Enclave
driver to reverse the condition checking whether a command is in the
exception list for variable length responses.

>     Reverse logic should be used when checking whether response is correct.
>     The logic should be when size not match and API is not listed in
>     exception list, return failure.

[Severity: Medium]
The commit message states this should return a failure, but does the code
actually do that?

Looking at the modified code, it only logs a warning with dev_err() and
clamps the size. Execution then continues to memcpy() and complete(),
returning a positive size value to the caller, which the IOCTL treats as a
success. The failure state is never properly propagated.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index e5117a1f19959..8daf32eded43c 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -219,7 +219,7 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  			se_clbk_hdl->dev_ctx->devname, *(u32 *)header);
>  
>  		if (rx_msg_sz != se_clbk_hdl->rx_msg_sz &&
> -		    check_hdr_exception_for_sz(priv, header)) {
> +		    !check_hdr_exception_for_sz(priv, header)) {

[Severity: Critical]
By negating this condition, does this inadvertently skip the size clamping
logic for exception commands?

If check_hdr_exception_for_sz() returns true, this block is skipped, which
means se_clbk_hdl->rx_msg_sz retains its potentially large user-controlled
value instead of being truncated to the actual message size.

The subsequent memcpy() at the end of this function will then read past the
valid 1024-byte hardware mailbox buffer (msg) into the kernel heap, copying
out-of-bounds kernel data into rx_msg. This data is eventually returned to
userspace, resulting in an information disclosure.

[Severity: High]
This isn't a bug introduced by this patch, but does the NVM command handling
unconditionally overwrite the clamped size?

Looking earlier in se_if_rx_callback():

		if (rx_msg_sz > MAX_NVM_MSG_LEN) {
			dev_err(dev,
				"%s: CMD-RCVER NVM: hdr(0x%x) with different sz(%d != %d).\n",
				se_clbk_hdl->dev_ctx->devname, *(u32 *)header,
				rx_msg_sz, se_clbk_hdl->rx_msg_sz);

			se_clbk_hdl->rx_msg_sz = MAX_NVM_MSG_LEN;
		}
		se_clbk_hdl->rx_msg_sz = rx_msg_sz;

Because there is no else clause, se_clbk_hdl->rx_msg_sz is immediately
overwritten with the oversized rx_msg_sz. The subsequent memcpy() could then
write the oversized payload into the pre-allocated NVM buffer, which is
strictly sized at MAX_NVM_MSG_LEN, overflowing the kernel heap.

[Severity: Critical]
This is a pre-existing issue, but can this function experience a NULL pointer
dereference if a delayed response arrives after a timeout?

In ele_msg_send_rcv(), if ele_msg_rcv() times out or is interrupted, the
function returns and clears priv->waiting_rsp_clbk_hdl.dev_ctx to NULL,
while the rx_msg buffer is freed by the __free(kfree) attribute in the
ioctl handler.

When a delayed or unsolicited firmware response eventually arrives,
se_if_rx_callback() accesses se_clbk_hdl->dev_ctx->devname, directly
dereferencing the NULL dev_ctx. Could this cause a kernel panic, or lead to
cross-thread data corruption and a Use-After-Free if another thread is
currently waiting?

[Severity: Critical]
This is a pre-existing issue, but does the IOCTL handler directly
dereference a user-space pointer during header validation?

In se_ioctl_cmd_snd_rcv_rsp_handler(), cmd_snd_rcv_rsp_info.tx_buf is a u64
representing a user-space memory address. It is passed directly to
se_chk_tx_msg_hdr(), which casts it to a struct se_msg_hdr * and dereferences
it without a prior copy_from_user().

Wouldn't this cause an immediate kernel panic on systems with SMAP or PAN
enabled? Additionally, it allows a time-of-check to time-of-use (TOCTOU)
attack where a malicious thread can alter the command ID after the validation
check but before memdup_user(), bypassing validation.

>  			dev_err(dev,
>  				"%s: Rsp to CMD: hdr(0x%x) with different sz(%d != %d).\n",
>  				se_clbk_hdl->dev_ctx->devname, *(u32 *)header,

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