Re: [PATCH v30 5/7] firmware: drivers: imx: adds miscdev

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] Driver unbind forcefully wakes waiting ioctls and clears firmware busy state, causing DMA Use-After-Free by the hardware.
- [High] Late mailbox interrupts can schedule `fw_busy_work` after it has been canceled, leading to a Use-After-Free of `priv` when the workqueue executes.
- [High] Unbind deadlock/hang due to concurrent ioctl threads blocked on `se_if_cmd_lock` bypassing the `complete_all()` wake-up.
- [Medium] The 128KB DMA shared memory buffer allocated for the internal `priv_dev_ctx` is permanently leaked on device unbind.

Pre-existing issues:
- [High] Concurrency race between se_if_rx_callback and se_if_fops_read on the shared command receiver buffer (rx_msg) leading to data corruption.
- [High] Dangling pointer priv->dev is passed to dma_free_coherent() and dev_warn() if a file descriptor is closed after device unbind.
--

--- Patch [5]: [PATCH v30 5/7] firmware: drivers: imx: adds miscdev ---
commit c7550c8a49060a4114eb071db0048e14879f71ab
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

    firmware: drivers: imx: adds miscdev

This commit adds a miscdev character driver for the i.MX secure enclave to
allow user-space applications to exchange messages with NXP secure enclave
hardware. It implements ABI documentation, device contexts, and command
header validation to restrict unsupported operations.

> 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
[ ... ]
> @@ -219,6 +219,9 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg, int tx_msg_
>  		return err;
>  	}
>  
>  	guard(mutex)(&priv->se_if_cmd_lock);
>  
>  	if (atomic_read(&priv->fw_busy)) {
>  		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n", dev_ctx->devname);
>  		return -EBUSY;
>  	}
>  	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
>  	spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);

[Severity: High]
Does this create an unbind deadlock if multiple ioctls are running concurrently?

When se_if_probe_cleanup() runs, it calls complete_all() to wake up the blocked
waiter in ele_msg_rcv(). However, concurrent firmware transactions are serialized
by priv->se_if_cmd_lock.

If Thread A holds se_if_cmd_lock and is blocked in ele_msg_rcv(), and Thread B is
blocked waiting to acquire se_if_cmd_lock, Thread B will miss the complete_all()
signal. Once Thread A finishes and releases the lock, Thread B will acquire it,
call reinit_completion() here, and wait for the full timeout. Meanwhile, the
unbind thread will block indefinitely trying to acquire dev_ctx->fops_lock
(which Thread B already holds from se_ioctl).

[ ... ]
> @@ -311,6 +314,10 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  		/*
>  		 * Clamp the copy length to the pre-allocated receiver buffer (MAX_NVM_MSG_LEN).
>  		 */
>  		se_clbk_hdl->rx_msg_sz = min_t(u32, rx_msg_sz, MAX_NVM_MSG_LEN);
>  		devname = se_clbk_hdl->dev_ctx->devname;
>  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
>  		complete(&se_clbk_hdl->done);
>  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);

[Severity: High]
This is a pre-existing issue, but can this lead to data corruption when
userspace is actively reading the response?

When se_if_fops_read() wakes up, it acquires clbk_rx_lock to snapshot the
pointers, then drops the lock and calls copy_to_user(). If the firmware sends
another command message while copy_to_user() is running, se_if_rx_callback()
will blindly overwrite the shared rx_msg buffer here via memcpy() since it
doesn't check if the buffer is currently being consumed.

[ ... ]
> @@ -354,6 +361,9 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
>  		se_clbk_hdl = &priv->waiting_rsp_clbk_hdl;
>  		spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
>  		if (!se_clbk_hdl->rx_msg) {
>  			if (atomic_read(&priv->fw_busy))
>  				schedule_fw_busy_work = true;
>  			spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
>  
>  			if (schedule_fw_busy_work)
>  				schedule_work(&priv->fw_busy_work);

[Severity: High]
Can a late mailbox interrupt cause a Use-After-Free of priv by scheduling this
work after the driver has been unbound?

In se_if_probe_cleanup(), cancel_work_sync(&priv->fw_busy_work) is called
before the mailbox channels are freed (mbox_free_channel). If a late mailbox
message arrives right before or during the unbind process, this callback can
evaluate the condition and schedule the work after it was supposedly canceled,
leading to the workqueue accessing freed memory when the unbind completes.

> 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
[ ... ]
> @@ -410,6 +410,9 @@ static void cleanup_se_shared_mem(struct se_if_device_ctx *dev_ctx)
>  	 * happen before the DMA backing memory is released to avoid a leak.
>  	 */
>  	se_dev_ctx_shared_mem_cleanup(dev_ctx);
>  
>  	dma_free_coherent(priv->dev, MAX_DATA_SIZE_PER_USER,
>  			  se_shared_mem_mgmt->non_secure_mem.ptr,
>  			  se_shared_mem_mgmt->non_secure_mem.dma_addr);

[Severity: High]
This is a pre-existing issue, but could this pass a dangling priv->dev
pointer to dma_free_coherent() if a file descriptor is closed after unbind?

The driver stores a pointer to the platform device in priv->dev in se_if_probe()
but does not pin it with get_device(). When the device is unbound, the
miscdevice is deregistered, dropping the VFS reference to the parent device.
If userspace still holds an open file descriptor, priv is kept alive, but the
underlying struct device might be freed. When close() is finally called, this
uses the dangling pointer.

[ ... ]
> @@ -650,6 +653,10 @@ static void se_if_priv_release(struct kref *kref)
>  
>  	/* Free priv_dev_ctx if it exists */
>  	if (priv->priv_dev_ctx) {
>  		/*
>  		 * miscdev storage belongs to open_gate, not directly to
>  		 * priv_dev_ctx. The gate should already have been detached
>  		 * from priv during teardown.
>  		 */
>  		cleanup_dev_ctx(priv->priv_dev_ctx, false);
>  
>  		kfree(priv->priv_dev_ctx->devname);
>  		kfree(priv->priv_dev_ctx);
>  		priv->priv_dev_ctx = NULL;
>  	}

[Severity: Medium]
Does this leak the 128KB DMA buffer allocated for priv_dev_ctx during unbind?

When se_if_probe_cleanup() runs, it sets priv->priv_dev_ctx->cleanup_done = true.
When se_if_priv_release() later calls cleanup_dev_ctx(priv->priv_dev_ctx, false),
cleanup_dev_ctx() skips freeing the DMA memory via cleanup_se_shared_mem()
because it sees cleanup_done is already true. The context struct is freed, but
the DMA memory is permanently leaked.

[ ... ]
> @@ -685,6 +692,10 @@ static void se_clear_fw_busy(struct se_if_priv *priv)
>  
>  	dev_ctx = priv->fw_busy_dev_ctx;
>  	priv->fw_busy_dev_ctx = NULL;
>  	atomic_set(&priv->fw_busy, 0);
>  	spin_unlock_irqrestore(&priv->fw_busy_lock, flags);
>  
>  	if (!dev_ctx)
>  		return;
>  
>  	scoped_guard(mutex, &dev_ctx->fops_lock) {
>  		if (dev_ctx->cleanup_done)
>  			cleanup_se_shared_mem(dev_ctx);
>  	}

[Severity: Critical]
Is it safe to clear the circuit breaker and unconditionally free the DMA memory
for legitimately timed-out contexts here?

During teardown, se_if_probe_cleanup() forcefully wakes up any waiting ioctls
using complete_all(). The woken thread in ele_msg_rcv() observes a positive
remaining timeout, thinks it succeeded, and thus skips setting the fw_busy
circuit breaker. It then proceeds to free the DMA memory.

Furthermore, se_clear_fw_busy() is called at the end of unbind, unconditionally
clearing the busy flag and freeing DMA memory for any context that timed out
previously. In both cases, the hardware enclave might still be executing and
actively DMA-writing to that freed physical memory, causing memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-imx-se-if-v30-0-ce8ba256692c@nxp.com?part=5