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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The mailbox receive callback `se_if_rx_callback` dereferences `msg` without checking `IS_ERR()`, leading to a kernel panic if the mailbox controller returns an error.
- [High] `ele_msg_rcv()` uses `MAX_SCHEDULE_TIMEOUT` causing an infinite uninterruptible wait and subsystem deadlock if the firmware hangs.
- [Medium] Global state `var_se_info.soc_rev` prevents SoC device registration and per-device IMEM state initialization on driver rebind or secondary device instances.
- [Medium] Casting a 1-byte aligned stack array to `struct ele_dev_info` and `u32` pointers causes undefined behavior and potential unaligned memory access faults.
- [Medium] The driver fails to expose the file descriptor interfaces claimed in the commit message and Kconfig.
- [Low] The SoC revision is incorrectly formatted as minor.major instead of major.minor.
--

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

Add MU-based communication interface for secure enclave.
The secure enclave FW communicates with Linux over single or multiple
dedicated messaging unit(MU) based interface(s).

Link: https://sashiko.dev/#/patchset/20260715-imx-se-if-v27-0-bb7c45952f06@nxp.com?part=3

> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 127ad752acf86..e3cb7f965e70e 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig

[ ... ]

> @@ -55,3 +55,16 @@ config IMX_SCMI_MISC_DRV
>  	  core that could provide misc functions such as board control.
>  
>  	  This driver can also be built as a module.
> +
> +config IMX_SEC_ENCLAVE
> +	tristate "i.MX Embedded Secure Enclave - EdgeLock Enclave Firmware driver."
> +	depends on MAILBOX && ((IMX_MBOX && ARCH_MXC && ARM64) || COMPILE_TEST)
> +	select FW_LOADER
> +	default m if ARCH_MXC
> +
> +	help
> +	  Exposes APIs supported by the iMX Secure Enclave HW IP called:
> +	  - EdgeLock Enclave Firmware (for i.MX8ULP, i.MX93),
> +	    like base, HSM, V2X & SHE using the SAB protocol via the shared Messaging
> +	    Unit. This driver exposes these interfaces via a set of file descriptors
> +	    allowing to configure shared memory, send and receive messages.

[Severity: Medium]
Is this Kconfig description accurate? It mentions exposing interfaces via a
set of file descriptors allowing to configure shared memory, send and receive
messages. However, there is no character device registration or file
operations implemented in this patch.

> diff --git a/drivers/firmware/imx/ele_base_msg.h b/drivers/firmware/imx/ele_base_msg.h
> new file mode 100644
> index 0000000000000..4c3699543e879
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.h

[ ... ]

> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2025 NXP
> + *
> + * Header file for the EdgeLock Enclave Base API(s).
> + */

[ ... ]

> +#define GET_SERIAL_NUM_FROM_UID(x, uid_word_sz) ({\
> +	const u32 *__x = (const u32 *)(x); \
> +	size_t __sz = (uid_word_sz); \
> +	((u64)__x[__sz - 1] << 32) | __x[0]; \
> +	})

[Severity: Medium]
Can this macro cause undefined behavior? When called from get_se_soc_info(),
the x parameter points into a 1-byte aligned stack array (u8 data). Casting
this to a u32 pointer and dereferencing it could trigger unaligned memory
access faults on architectures with strict alignment requirements.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> new file mode 100644
> index 0000000000000..ca8a32a82689b
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -0,0 +1,470 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */

[ ... ]

> +int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl)
> +{
> +	bool wait_uninterruptible = false;
> +	unsigned long remaining_jiffies;
> +	unsigned long flags;
> +	int ret;
> +
> +	remaining_jiffies = MAX_SCHEDULE_TIMEOUT;
> +	do {
> +		if (wait_uninterruptible)
> +			ret = wait_for_completion_timeout(&se_clbk_hdl->done,
> +							  remaining_jiffies);

[Severity: High]
Can this loop cause a subsystem deadlock? Since remaining_jiffies is set to
MAX_SCHEDULE_TIMEOUT, wait_for_completion_timeout() blocks indefinitely. If
the firmware hangs and wait_uninterruptible is set to true after a signal,
the thread will enter an infinite uninterruptible sleep while holding the
se_if_cmd_lock mutex. The timeout fallback and circuit breaker logic become
unreachable.

> +		else
> +			ret = wait_for_completion_interruptible_timeout(&se_clbk_hdl->done,
> +									remaining_jiffies);

[ ... ]

> +void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> +{
> +	struct se_clbk_handle *se_clbk_hdl;
> +	struct device *dev = mbox_cl->dev;
> +	struct se_msg_hdr *header;
> +	bool sz_mismatch = false;
> +	struct se_if_priv *priv;
> +	unsigned long flags;
> +	u32 rx_msg_sz;
> +
> +	priv = dev_get_drvdata(dev);
> +
> +	/* The function can be called with NULL msg */
> +	if (!msg) {
> +		dev_err(dev, "Message is invalid\n");
> +		return;
> +	}
> +
> +	header = msg;
> +	rx_msg_sz = header->size << 2;

[Severity: High]
Does this code safely handle mailbox errors? If the mailbox controller
returns an error (like a timeout), it typically passes ERR_PTR(err) to the
callback. The check for !msg allows ERR_PTR to bypass the check, which
would lead to a kernel panic when msg is cast to struct se_msg_hdr and
dereferenced here.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> new file mode 100644
> index 0000000000000..9bb59c200911b
> --- /dev/null
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2026 NXP
> + */

[ ... ]

> +static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se_info)
> +{
> +	struct se_fw_load_info *load_fw = get_load_fw_instance(priv);
> +	struct soc_device_attribute *attr;
> +	u8 data[MAX_SOC_INFO_DATA_SZ];
> +	struct ele_dev_info *s_info;
> +	struct soc_device *sdev;
> +	int err = 0;
> +
> +	/*
> +	 * This function should be called once.
> +	 * Check if the se_soc_rev is zero to continue.
> +	 */
> +	guard(mutex)(&var_se_info.se_var_info_lock);
> +	if (var_se_info.soc_rev)
> +		return err;

[Severity: Medium]
Will this early return prevent initialization on driver rebind? When the
driver is unbound, soc_device_unregister() removes the SoC device, but the
global var_se_info.soc_rev is never cleared. On a subsequent bind, a
non-zero soc_rev causes an early return, bypassing soc_device_register()
and leaving load_fw->imem.state uninitialized.

> +
> +	err = ele_fetch_soc_info(priv, &data);

[ ... ]

> +	if (FIELD_GET(DEV_GETINFO_MIN_VER_MASK, var_se_info.soc_rev))
> +		attr->revision = devm_kasprintf(priv->dev, GFP_KERNEL, "%x.%x",
> +						FIELD_GET(DEV_GETINFO_MIN_VER_MASK,
> +							  var_se_info.soc_rev),
> +						FIELD_GET(DEV_GETINFO_MAJ_VER_MASK,
> +							  var_se_info.soc_rev));

[Severity: Low]
Is this string format correct? It prints MIN_VER_MASK before MAJ_VER_MASK,
resulting in minor.major instead of major.minor. This might break userspace
parsing expectations.

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