Re: [PATCH 5/7] firmware: imx: ele: Correct check_hdr_exception_for_sz

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free in `ele_msg_send_rcv()` if `ele_msg_send()` fails.
- [Critical] Unconditional NULL Pointer Dereference in `se_if_rx_callback()` on unsolicited firmware responses.
- [Critical] Heap Buffer Overflow in NVM command handler inside `se_if_rx_callback()`.
- [Critical] Direct Dereference of User Space Pointers without `copy_from_user()` violating SMAP.
- [High] Unbounded Memory Allocation in `SE_IOCTL_CMD_SEND_RCV_RSP` via `cmd_snd_rcv_rsp_info.rx_buf_sz`.
- [High] Heap Out-of-Bounds Read and Information Leak due to exception-handling bypass in `se_if_rx_callback()`.
--

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

firmware: imx: ele: Correct check_hdr_exception_for_sz
    
header.size is u8 type, "header.size >= 0" will always return true.
Actually the check condition should be ">= 2", because a valid response
includes at lease header(4bytes) and status(4bytes), total 2 ints.

And ELE_DEBUG_DUMP_RSP_SZ is counted using bytes, need to divide it by 4.

Fixes: 106ffe5d78ad8 ("firmware: imx: add driver for NXP EdgeLock Enclave")
Signed-off-by: Peng Fan <peng.fan@nxp.com>

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 8daf32eded43c..76bd3841acfcb 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -164,7 +164,7 @@ static bool check_hdr_exception_for_sz(struct se_if_priv *priv,
>  	 */
>  	if (header->command == ELE_DEBUG_DUMP_REQ &&
>  	    header->ver == priv->if_defs->base_api_ver &&
> -	    header->size >= 0 && header->size <= ELE_DEBUG_DUMP_RSP_SZ)
> +	    header->size >= 2 && header->size <= (ELE_DEBUG_DUMP_RSP_SZ / 4))
>  		return true;
>  
>  	return false;

[Severity: High]
This is a pre-existing issue, but I noticed something concerning in how
check_hdr_exception_for_sz() is used. In se_if_rx_callback(), if this
function returns true, it skips the size constraint check:

drivers/firmware/imx/ele_common.c:se_if_rx_callback() {
    [ ... ]
		if (rx_msg_sz != se_clbk_hdl->rx_msg_sz &&
		    !check_hdr_exception_for_sz(priv, header)) {
            [ ... ]
			se_clbk_hdl->rx_msg_sz = min(rx_msg_sz, se_clbk_hdl->rx_msg_sz);
		}
	}
    [ ... ]
	memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
    [ ... ]
}

Does this bypass allow se_clbk_hdl->rx_msg_sz to retain an unvalidated,
potentially large user-provided value? If so, couldn't the memcpy()
read past the bounds of the hardware mailbox msg buffer and leak adjacent
kernel memory to userspace?

[Severity: Critical]
This is a pre-existing issue, but while looking at se_if_rx_callback(),
I also noticed what looks like an unconditional overwrite for NVM commands:

drivers/firmware/imx/ele_common.c:se_if_rx_callback() {
    [ ... ]
		if (rx_msg_sz > MAX_NVM_MSG_LEN) {
            [ ... ]
			se_clbk_hdl->rx_msg_sz = MAX_NVM_MSG_LEN;
		}
		se_clbk_hdl->rx_msg_sz = rx_msg_sz;
    [ ... ]
}

Does the assignment se_clbk_hdl->rx_msg_sz = rx_msg_sz entirely defeat the
MAX_NVM_MSG_LEN check? Since the rx_msg buffer is pre-allocated with
MAX_NVM_MSG_LEN, could a large payload from the firmware cause a heap
buffer overflow in the subsequent memcpy()?

[Severity: Critical]
This isn't a bug introduced by this patch, but there might be a NULL pointer
dereference in se_if_rx_callback() when receiving unsolicited messages.

drivers/firmware/imx/ele_common.c:se_if_rx_callback() {
    [ ... ]
	if (header->tag == priv->if_defs->cmd_tag) {
		se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
		dev_dbg(dev, "Selecting cmd receiver:%s for mesg header:0x%x.",
			se_clbk_hdl->dev_ctx->devname,  *(u32 *)header);
    [ ... ]
}

If the driver is idle, won't se_clbk_hdl->dev_ctx be NULL? It looks like
dereferencing dev_ctx->devname here would cause an Oops if the firmware sends
an unexpected response or command.

[Severity: Critical]
This is a pre-existing issue, but I have a question about the ioctl handler
se_ioctl_cmd_snd_rcv_rsp_handler() in se_ctrl.c:

drivers/firmware/imx/se_ctrl.c:se_ioctl_cmd_snd_rcv_rsp_handler() {
    [ ... ]
	err = se_chk_tx_msg_hdr(priv, (struct se_msg_hdr *)cmd_snd_rcv_rsp_info.tx_buf);
    [ ... ]
	struct se_api_msg *tx_msg __free(kfree) =
		memdup_user(cmd_snd_rcv_rsp_info.tx_buf,
			    cmd_snd_rcv_rsp_info.tx_buf_sz);
    [ ... ]
}

Since cmd_snd_rcv_rsp_info.tx_buf is a user-space pointer, is it safe to
cast it and pass it to se_chk_tx_msg_hdr() before memdup_user() is called?
Wouldn't direct dereferencing of this pointer inside se_chk_tx_msg_hdr()
trigger an immediate panic on systems with SMAP enabled, or a page fault if
the pointer is invalid?

[Severity: High]
This isn't a bug introduced by this patch, but I spotted a potential unbounded
allocation in the same ioctl handler:

drivers/firmware/imx/se_ctrl.c:se_ioctl_cmd_snd_rcv_rsp_handler() {
    [ ... ]
	struct se_api_msg *rx_msg __free(kfree) =
		kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL);
    [ ... ]
}

Does this code safely handle large or negative values for rx_buf_sz?
Since the size comes directly from the user, could a negative value wrap
to a massive size_t and trigger a kernel warning or memory exhaustion when
passed to kzalloc()?

[Severity: Critical]
This is a pre-existing issue, but there seems to be a possible use-after-free
involving ele_msg_send_rcv() and se_ioctl_cmd_snd_rcv_rsp_handler():

drivers/firmware/imx/ele_common.c:ele_msg_send_rcv() {
    [ ... ]
	priv->waiting_rsp_clbk_hdl.dev_ctx = dev_ctx;
	priv->waiting_rsp_clbk_hdl.rx_msg_sz = exp_rx_msg_sz;
	priv->waiting_rsp_clbk_hdl.rx_msg = rx_msg;

	err = ele_msg_send(dev_ctx, tx_msg, tx_msg_sz);
	if (err < 0)
		return err;
    [ ... ]
}

If ele_msg_send() fails, ele_msg_send_rcv() returns immediately while
priv->waiting_rsp_clbk_hdl.dev_ctx remains non-NULL.

Back in se_ioctl_cmd_snd_rcv_rsp_handler():

drivers/firmware/imx/se_ctrl.c:se_ioctl_cmd_snd_rcv_rsp_handler() {
    [ ... ]
	struct se_api_msg *rx_msg __free(kfree) =
		kzalloc(cmd_snd_rcv_rsp_info.rx_buf_sz, GFP_KERNEL);
    [ ... ]
	err = ele_msg_send_rcv(dev_ctx, tx_msg, cmd_snd_rcv_rsp_info.tx_buf_sz,
			       rx_msg, cmd_snd_rcv_rsp_info.rx_buf_sz);
    [ ... ]
}

When the function returns, rx_msg is automatically freed. If a delayed
response later arrives from the firmware, wouldn't se_if_rx_callback()
see a valid dev_ctx and copy the payload into the already-freed rx_msg
pointer, causing a use-after-free?

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