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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] DMA use-after-free on firmware timeout in `ele_get_info()` and driver unbind.
- [Medium] `se_resume()` ignores the failure of `se_restore_imem_state()`, breaking expected power management error handling semantics.
- [Low] Incorrect iteration bound computation in `ele_debug_dump` leads to reading past the received payload.
--

--- Patch [3]: [PATCH v38 3/7] firmware: imx: add driver for NXP EdgeLock Enclave ---
commit b60827d987987a5036c02b256c938fa4f0af210a
Author: Pankaj Gupta <[email protected]>

firmware: imx: add driver for NXP EdgeLock Enclave

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

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 0000000000000..724f6e913ce79 100644
> --- /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)
> +{

[ ... ]

> +	ret = ele_msg_send_rcv(priv, tx_msg, ELE_GET_INFO_REQ_MSG_SZ, rx_msg,
> +			       ELE_GET_INFO_RSP_MSG_SZ);
> +	if (ret < 0) {
> +		ele_get_info_cleanup(priv, get_info_data, get_info_addr, get_info_len);

[Severity: High]
If an enclave command times out, ele_msg_send_rcv() returns -ETIMEDOUT and
the error path immediately calls ele_get_info_cleanup(), which frees the DMA
buffer. If the firmware is still executing and completes the transaction
later, could it perform a DMA write into physical memory that has been
reallocated to another subsystem?

> +		return ret;
> +	}

[ ... ]

> +int ele_debug_dump(struct se_if_priv *priv)
> +{

[ ... ]

> +		rx_msg->header.size -= 2;
> +
> +		if (rx_msg->header.size > 2)
> +			rx_msg->header.size--;
> +
> +		for (i = 0; i < rx_msg->header.size; i += 2)
> +			dev_info(priv->dev, "%s%02x_%02x: 0x%08x 0x%08x",
> +				 FW_DBG_DUMP_FIXED_STR,	msg_ex_cnt, i,
> +				 rx_msg->data[i + 1], rx_msg->data[i + 2]);

[Severity: Low]
Does this loop read past the actual message payload length? If the firmware
returns an odd number of payload words, the remaining size is not even. The
loop body would execute when exactly 1 word remains but print both
rx_msg->data[i + 1] and rx_msg->data[i + 2].

> +
> +		msg_ex_cnt++;
> +	} while (keep_logging);
> +
> +	return ret;
> +}

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 0000000000000..53091cc37e7e0 100644
> --- /dev/null
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> +static void se_if_probe_cleanup(void *plat_dev)
> +{
> +	struct platform_device *pdev = plat_dev;
> +	struct device *dev = &pdev->dev;
> +	struct se_if_priv *priv;
> +
> +	priv = dev_get_drvdata(dev);
> +	if (!priv)
> +		return;
> +
> +	if (priv->rx_chan)
> +		mbox_free_channel(priv->rx_chan);
> +	if (priv->tx_chan)
> +		mbox_free_channel(priv->tx_chan);
> +
> +	/*
> +	 * Being device managed buffer, no need to free the buffer allocated
> +	 * in se probe to store encrypted IMEM.
> +	 */
> +
> +	/*
> +	 * No need to check, if reserved memory is allocated
> +	 * before calling for its release. Or clearing the
> +	 * un-set bit.
> +	 */
> +	of_reserved_mem_device_release(dev);
> +
> +	dev_set_drvdata(dev, NULL);
> +
> +	kfree(priv);

[Severity: High]
Similarly, does this teardown path free device-managed DMA memory without
waiting for or aborting a potentially busy firmware transaction?

> +}

[ ... ]

> +static int se_resume(struct device *dev)
> +{
> +	struct se_if_priv *priv = dev_get_drvdata(dev);
> +	struct se_fw_load_info *load_fw;
> +	int ret = 0;
> +
> +	load_fw = get_load_fw_instance(priv);
> +
> +	if (load_fw->imem_mgmt) {
> +		ret = se_restore_imem_state(priv, &load_fw->imem);
> +		if (ret)
> +			dev_warn(dev, "Failure restoring IMEM state[0x%x]", ret);
> +	}
> +
> +	return 0;

[Severity: Medium]
Should this return the actual error instead of 0? se_restore_imem_state()
documents that it returns -EIO so the PM subsystem knows the enclave is
non-functional after resume, but the failure is caught here and 0 is
unconditionally returned, breaking the expected power management error
handling semantics.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3