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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Overriding a successfully completed hardware transaction with `-ERESTARTSYS` corrupts system state.
- [High] DMA buffer payload structures lack proper endianness annotations and are accessed via unaligned stack pointers.
- [Medium] The driver ignores the return value of `dma_set_mask_and_coherent()`.
--

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

firmware: imx: add driver for NXP EdgeLock Enclave

This adds a driver for the NXP EdgeLock Enclave (ELE) which exposes a MU-based
communication interface for secure enclaves on i.MX SoC(s). The secure enclave
firmware communicates with Linux over single or multiple dedicated messaging
unit (MU) interfaces.

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

[ ... ]

> +struct dev_info {
> +	u8  cmd;
> +	u8  ver;
> +	u16 length;
> +	u16 soc_id;
> +	u16 soc_rev;
> +	u16 lmda_val;
> +	u8  ssm_state;
> +	u8  dev_atts_api_ver;

[Severity: High]
Should these hardware payload fields use __le16 instead of host-endian u16?

On big-endian systems, reading these fields without endianness conversions
might result in incorrect values.

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

[ ... ]

> +int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
> +		     void *rx_msg, int exp_rx_msg_sz)
> +{

[ ... ]

> +	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
> +		/*
> +		 * Signal was deferred until the FW/kernel protocol resynchronized.
> +		 * On success report -ERESTARTSYS for the interrupted wait; the
> +		 * command is not re-sent. Keep real errors like -ETIMEDOUT.
> +		 */
> +		if (err > 0)
> +			err = -ERESTARTSYS;

[Severity: High]
Is it safe to override a successful hardware response with -ERESTARTSYS?

If ele_msg_send_rcv() successfully sends a command to the hardware but a
signal arrives during the wait, the function waits for the hardware to
finish, retrieves the response, and then overrides the success code.

The VFS or userspace application will interpret -ERESTARTSYS as an
instruction to restart the syscall. Could this result in non-idempotent
hardware commands (such as firmware authentication or fuse writing) being
executed multiple times and corrupting device state?

> +		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> +		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.\n", err);
> +	}
> +
> +	ele_msg_send_rcv_cleanup(priv);
> +
> +	return err;
> +}

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

[ ... ]

> +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);
> +	u8 data[MAX_SOC_INFO_DATA_SZ];
> +	struct ele_dev_info *s_info;
> +	int err;

[ ... ]

> +	err = ele_fetch_soc_info(priv, &data);
> +	if (err < 0)
> +		return dev_err_probe(priv->dev, err, "Failed to fetch SoC Info.\n");
> +
> +	s_info = (struct ele_dev_info *)data;
> +
> +	if (!var_se_info.soc_rev)
> +		var_se_info.soc_rev = s_info->d_info.soc_rev;

[Severity: High]
Can casting a byte-aligned stack array to a structure pointer cause issues
here?

The u8 array data is cast to struct ele_dev_info * and then dereferenced.
On architectures with strict alignment requirements, could this unaligned
access cause an exception?

[ ... ]

> +static int se_if_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	if (if_node->reserved_dma_ranges) {
> +		ret = of_reserved_mem_device_init(dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to init reserved memory region.\n");
> +	}
> +
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));

[Severity: Medium]
Should the return value of dma_set_mask_and_coherent() be checked?

If DMA mask configuration fails, the driver proceeds with DMA allocations
anyway. Could this lead to truncated DMA addresses or failure of device
communication if the system cannot accommodate the 32-bit mask?

> +
> +	/*
> +	 * Initialize load_fw_lock before registering the misc device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-imx-se-if-v42-0-2e8efac0bb16@nxp.com?part=3