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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect devres registration order causes Use-After-Free / NULL pointer dereference in the mailbox RX callback.
- [High] Data race on `rx_msg_sz` in receive callback can cause truncated message copies.
- [Medium] Memory leak of the `priv` structure on early probe failure paths.
- [Medium] Unsafe syscall restart on successfully completed non-idempotent hardware operations.
- [Medium] The global `soc_device` singleton's lifecycle is incorrectly tied to the first probed MU interface, causing a data race and premature sysfs deletion.
- [Medium] `ele_msg_rcv` resets the timeout duration when interrupted by a signal, failing to account for elapsed time.
--

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

firmware: imx: add driver for NXP EdgeLock Enclave

This commit introduces a driver for the NXP EdgeLock Enclave (ELE). It exposes
APIs supported by the iMX Secure Enclave HW IP using the SAB protocol via
dedicated Messaging Unit interfaces.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c

[ ... ]

> @@ -49,15 +49,27 @@ 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 = msecs_to_jiffies(SE_RCV_MSG_DEFAULT_TIMEOUT_MS);
>  	do {
>  		if (wait_uninterruptible)
>  			ret = wait_for_completion_timeout(&se_clbk_hdl->done,
>  							  remaining_jiffies);
>  		else
>  			ret = wait_for_completion_interruptible_timeout(&se_clbk_hdl->done,
>  									remaining_jiffies);
>  		if (ret == -ERESTARTSYS) {
>  			/*
>  			 * Record that a signal was observed, then continue waiting non-
>  			 * interruptibly until the response arrives or the timeout
>  			 * expires. The caller can surface the interruption to userspace
>  			 * after the protocol transaction is brought back to a
>  			 * synchronized state.
>  			 */
>  			if (se_clbk_hdl == &priv->waiting_rsp_clbk_hdl &&
>  			    READ_ONCE(se_clbk_hdl->rx_msg)) {
>  				WRITE_ONCE(se_clbk_hdl->signal_rcvd, true);
>  				wait_uninterruptible = true;
>  				continue;
>  			}
>  			break;
>  		}

[Severity: Medium]
When wait_for_completion_interruptible_timeout() is interrupted by a signal,
the loop falls back to an uninterruptible wait.

Since remaining_jiffies is never updated to subtract the time already spent
waiting, does this unintentionally reset the timeout duration, effectively
extending the wait time?

[ ... ]

> @@ -155,27 +167,23 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
>  	err = ele_msg_rcv(priv, &priv->waiting_rsp_clbk_hdl);
>  
>  	if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
>  		/*
>  		 * A signal was received after the command was sent. ele_msg_rcv()
>  		 * kept waiting until the FW/kernel protocol was synchronized again.
>  		 * If the transaction itself completed successfully, report the
>  		 * deferred signal to userspace using normal syscall-restart semantics.
>  		 * Do not hide real firmware/protocol errors such as -ETIMEDOUT.
>  		 */
>  		if (err > 0)
>  			err = -ERESTARTSYS;
>  		priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
>  		dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
>  	}

[Severity: Medium]
When a signal interrupts the wait, the loop catches -ERESTARTSYS.

If the hardware transaction successfully completes afterward (err > 0), the
status is explicitly overwritten with -ERESTARTSYS here.

Could this cause the kernel to re-execute the system call, leading to
non-idempotent hardware operations being sent to the enclave twice?

[ ... ]

> @@ -259,20 +267,17 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  	} else if (header->tag == priv->if_defs->rsp_tag) {
>  		bool exception_for_sz_mismatch = check_hdr_exception_for_sz(priv, header);
>  		u32 exp_rx_msg_sz = 0;
>  
>  		/*
>  		 * waiting_rsp_clbk_hdl.rx_msg is owned by the synchronous sender in
>  		 * ele_msg_send_rcv(). After timeout or error, that path clears rx_msg
>  		 * under clbk_rx_lock before returning to its caller, which may then free
>  		 * the buffer. Check rx_msg under the same lock here so a delayed response
>  		 * can be detected and dropped instead of copying into freed memory.
>  		 *
>  		 * A late response also closes the firmware-busy circuit breaker, allowing
>  		 * future command/response transactions to proceed again.
>  		 */
>  		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
>  		exp_rx_msg_sz = se_clbk_hdl->rx_msg_sz;
>  		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  		if (!se_clbk_hdl->rx_msg) {

[Severity: High]
In se_if_rx_callback(), exp_rx_msg_sz is read directly from
se_clbk_hdl->rx_msg_sz before acquiring clbk_rx_lock.

If a delayed response triggers this callback concurrently with a new
transaction updating rx_msg_sz under the lock, could the callback read a
stale size value?

Might this result in copying truncated or corrupted response data once the
lock is acquired?

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> @@ -140,9 +140,11 @@ static void se_soc_device_unregister(void *data)
>  {
>  	struct soc_device *sdev = data;
>  
>  	soc_device_unregister(sdev);
>  	var_se_info.soc_dev_registered = false;
>  }

[Severity: Medium]
In se_soc_device_unregister(), var_se_info.soc_dev_registered is cleared
without acquiring the se_var_info_lock.

Does this mutation of global state create a data race against concurrent
probe threads?

> @@ -155,25 +157,21 @@ static int se_soc_dev_register(struct se_if_priv *priv, u16 soc_rev,
>  
>  	attr->serial_number = devm_kasprintf(priv->dev, GFP_KERNEL, "%016llX",
>  					     GET_SERIAL_NUM_FROM_UID(uid, MAX_UID_SIZE >> 2));
>  	if (!attr->serial_number)
>  		return -ENOMEM;
>  
>  	sdev = soc_device_register(attr);
>  	if (IS_ERR(sdev))
>  		return PTR_ERR(sdev);
>  
>  	err = devm_add_action_or_reset(priv->dev, se_soc_device_unregister, sdev);

[Severity: Medium]
The global SoC device singleton is registered using devm_add_action_or_reset()
tied to the first probed MU interface (priv->dev).

If this specific MU interface is unbound, does it prematurely destroy the
global singleton for any other active MU interfaces?

[ ... ]

> @@ -322,25 +320,18 @@ static int se_if_probe(struct platform_device *pdev)
>  
>  	se_info = if_node->se_info;
>  
>  	priv = kzalloc_obj(*priv, GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->dev = dev;
>  	priv->if_defs = &if_node->if_defs;
>  	dev_set_drvdata(dev, priv);
>  
>  	mutex_init(&priv->se_if_cmd_lock);
>  	spin_lock_init(&priv->cmd_receiver_clbk_hdl.clbk_rx_lock);
>  	spin_lock_init(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock);
>  	atomic_set(&priv->fw_busy, 0);
>  	init_completion(&priv->waiting_rsp_clbk_hdl.done);
>  	init_completion(&priv->cmd_receiver_clbk_hdl.done);
>  
>  	/* Mailbox client configuration */
>  	priv->se_mb_cl.dev		= dev;
>  	priv->se_mb_cl.tx_block		= false;
>  	priv->se_mb_cl.knows_txdone	= false;
>  	priv->se_mb_cl.rx_callback	= se_if_rx_callback;
>  
>  	ret = se_if_request_channel(dev, &priv->tx_chan, &priv->se_mb_cl, MBOX_TX_NAME);
>  	if (ret)
>  		return ret;

[Severity: Medium]
In se_if_probe(), the priv structure is allocated with kzalloc_obj() which
is not device-managed.

If the first se_if_request_channel() call fails, the function returns
immediately.

Since se_if_probe_cleanup() is not registered until later, does this leave
the priv structure leaked on these early error paths?

>  
>  	ret = se_if_request_channel(dev, &priv->rx_chan, &priv->se_mb_cl, MBOX_RX_NAME);
>  	if (ret)
>  		return ret;
>  
>  	ret = devm_add_action_or_reset(dev, se_if_probe_cleanup, pdev);
>  	if (ret)
>  		return ret;

[Severity: High]
Because devres actions execute in reverse order (LIFO) during teardown,
registering se_if_probe_cleanup() after requesting the mailbox channels means
the cleanup function will run before the RX channel is freed.

The cleanup function frees the priv structure.

If a hardware interrupt fires during this window, wouldn't
se_if_rx_callback() retrieve a NULL pointer via dev_get_drvdata() and trigger
a crash when dereferencing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-imx-se-if-v29-0-04a362f4fcca@nxp.com?part=3