Re: [PATCH v45 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] Memory allocations with GFP_KERNEL during suspend/resume callbacks can cause system deadlocks.
- [Medium] The return value of dma_set_mask_and_coherent() is ignored, which can lead to cryptic DMA failures.
- [Low] Incorrect format specifier `%pap` used for `dma_addr_t`, which could leak stack memory into the kernel log.
--

commit 5b128ec8a940b8c6d0e2e25fd2b73df2c1028b07
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
firmware: imx: add driver for NXP EdgeLock Enclave

This commit adds a messaging unit (MU) based communication interface driver
for the NXP EdgeLock Enclave (ELE) on i.MX hardware. It implements base
messaging protocols, device info retrieval, and power management callbacks.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 0000000000000..d00315d36e8ad
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_base_msg.c
[ ... ]
> +int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
> +{
> +	dma_addr_t get_info_addr = 0;
> +	void *get_info_data = NULL;
> +	u32 get_info_len;
> +	int ret;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	memset(s_info, 0x0, sizeof(*s_info));
> +
> +	struct se_api_msg *tx_msg __free(kfree) =
> +		kzalloc(ELE_GET_INFO_REQ_MSG_SZ, GFP_KERNEL);

[Severity: High]
Can this allocation cause a system deadlock during suspend?

Since ele_get_info() is called from the suspend path:
se_suspend() -> se_save_imem_state() -> ele_get_info()

Using GFP_KERNEL here allows direct memory reclaim, which might attempt to
flush dirty pages to storage. If the storage device (like dm-crypt) is a
consumer of this crypto hardware and has already been suspended, the system
could permanently deadlock.

Should this use GFP_NOIO or pre-allocate the buffer instead?

[ ... ]
> +int ele_service_swap(struct se_if_priv *priv,
> +		     dma_addr_t addr,
> +		     u32 addr_size, u16 flag)
> +{
> +	int ret;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	if (upper_32_bits(addr)) {
> +		dev_err(priv->dev,
> +			"ELE service-swap address exceeds 32-bit range: %pad\n",
> +			&addr);
> +		return -ERANGE;
> +	}
> +
> +	struct se_api_msg *tx_msg __free(kfree)	=
> +		kzalloc(ELE_SERVICE_SWAP_REQ_MSG_SZ, GFP_KERNEL);

[Severity: High]
Similar to the ele_get_info() allocation above, this is also called from the
suspend path via se_save_imem_state(). Could this GFP_KERNEL allocation also
deadlock if the system is under memory pressure during suspend?

[ ... ]
> +int ele_fw_authenticate(struct se_if_priv *priv, dma_addr_t contnr_addr,
> +			dma_addr_t img_addr)
> +{
> +	int ret;
> +
> +	if (!priv)
> +		return -EINVAL;
> +
> +	if (upper_32_bits(contnr_addr) || upper_32_bits(img_addr)) {
> +		dev_err(priv->dev, "Wrong address: %pap %pap\n", &contnr_addr, &img_addr);

[Severity: Low]
Does this leak stack memory into the kernel log on 64-bit platforms?

The variables contnr_addr and img_addr are of type dma_addr_t. The %pap
format specifier expects a pointer to phys_addr_t. If sizeof(dma_addr_t) is
4 bytes and sizeof(phys_addr_t) is 8 bytes on the architecture, printk will
read past the pointers and print unrelated stack memory into dmesg.

Would it be safer to use %pad for dma_addr_t pointers?

> +		return -EINVAL;
> +	}
[ ... ]
> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 0000000000000..9e3f81a9a2728
> --- /dev/null
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> +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]
Is it safe to ignore the return value of dma_set_mask_and_coherent() here?

If the architecture rejects the 32-bit mask and falls back to 64-bit, later
dma_alloc_coherent() calls could return 64-bit addresses. Since the driver
truncates these into 32-bit fields using lower_32_bits() before sending
them to the firmware, could this result in cryptic DMA failures or memory
corruption?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-imx-se-if-v45-0-6e400879d8ec@nxp.com?part=3