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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jul 17, 2026 at 11:10:09AM +0000, sashiko-bot@kernel.org wrote:
> 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.

please check this

>
> > 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.

Can you use reasonable timeout value to avoid deadlock?

>
> > +		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.

Please double check this ? if yes, should use IS_ERR()

>
> > 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.

Make sense, please fix it.

Frank

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