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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Per-instance IMEM state is gated behind a global static variable, silently breaking boot firmware loading on subsequent probes or module reload.
- [High] Overriding a successful, completed firmware transaction with `-ERESTARTSYS` due to an earlier signal causes hazardous double-execution of non-idempotent operations.
--

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

firmware: imx: add driver for NXP EdgeLock Enclave

Add MU-based communication interface for secure enclave.
Exists on i.MX SoC(s) like i.MX8ULP, i.MX93, i.MX95 etc.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 0000000000000..04ee5112d3463 100644
> --- /dev/null
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> +	err = ele_msg_rcv(priv, &priv->waiting_rsp_clbk_hdl);
> +
> +	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]
Does overriding a successful hardware response (`err > 0`) with
`-ERESTARTSYS` cause hazardous double-execution of non-idempotent hardware
operations (e.g., key generation, counter increments)?

If a transaction completes successfully in hardware, but userspace receives
`-EINTR` (from `-ERESTARTSYS`), it may reissue the system call, causing the
hardware to execute the same state-changing operation a second time. Should
returning success take precedence over the pending signal to keep the hardware
state consistent with the userspace view?

> +		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
> +		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", 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
> index 0000000000000..df3d1da28a759 100644
> --- /dev/null
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> +	s_info = (struct ele_dev_info *)data;
> +
> +	if (!var_se_info.soc_rev) {
> +		var_se_info.soc_rev = s_info->d_info.soc_rev;
> +
> +		/*
> +		 * Only update IMEM state when the load_fw path is active;
> +		 * on SoCs without IMEM management (e.g. i.MX93) the field
> +		 * is not meaningful.
> +		 */
> +		if (load_fw->imem_mgmt)
> +			load_fw->imem.state = s_info->d_addn_info.imem_state;
> +	}

[Severity: High]
Will gating the per-instance `priv->load_fw.imem.state` initialization inside
`if (!var_se_info.soc_rev)` silently break boot firmware loading on subsequent
probes or module reload?

Since `var_se_info` is a global static variable whose `soc_rev` field is never
cleared across driver unbinds, any subsequent interface probe (or re-probe
after unbind via sysfs) will find `var_se_info.soc_rev` already populated.

This causes the driver to skip updating `load_fw->imem.state`, leaving it at
`0`. Later, `se_load_firmware()` checks `if (load_fw->imem.state ==
ELE_IMEM_STATE_BAD)` to decide whether to load the primary boot firmware.
Since the state is 0 instead of `0xfe`, does it silently skip the load and
break firmware initialization for that instance?

> +
> +	if (se_info->soc_name && !var_se_info.soc_dev_regn.soc_dev_registered) {
> +		err = se_soc_dev_register(priv, var_se_info.soc_rev,

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