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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] The `read()` path drops `fops_lock` and `clbk_rx_lock` before accessing shared lists and `rx_msg`, leading to Use-After-Free and list corruption.
- [High] KASAN slab-out-of-bounds read in `se_val_rsp_hdr_n_status` due to insufficient response buffer size validation.
- [High] Indefinite hang during module unload or device unbind when userspace is blocked in `SE_IOCTL_CMD_SEND_RCV_RSP`.
- [High] Hung task panic triggered by signal interruption during long firmware waits in `ele_msg_rcv()`.
- [High] The `SE_IOCTL_CMD_SEND_RCV_RSP` ioctl silently swallows kernel-level validation errors, returning success to userspace.
- [High] Memory leak of dynamically allocated `se_buf_desc` structures when a device fd is closed without cleaning up pending buffers.
- [High] IOCTL error path zeroes out shared DMA memory buffers while the firmware might still be actively accessing them due to a timeout.
- [High] Fatal kernel crash during probe failure due to `dma_free_coherent` on an uninitialized pointer for the internal device context.
- [High] Unsynchronized clearing of `rx_msg_sz` in `se_if_fops_read()` causes lost commands and `-ENODEV` when interrupted by a signal.
- [High] TOCTOU race during device context creation in `open()` against unbind cleanup allows file operations on an unbound driver.
- [High] Race window during driver unbind between `cancel_work_sync()` and `se_clear_fw_busy()` allows work scheduling on freed memory.
- [Medium] Race condition exposing device node before probe initialization completes.
- [Medium] Mixing `goto`-based error handling with scope-based cleanup (`__free` and `guard`) violates kernel cleanup rules.
--

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

firmware: drivers: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, that
enables exchanging messages with NXP secure enclave HW IP(s)
like EdgeLock Enclave, from User-Space Applications via character driver.

Link: https://lkml.kernel.org/r/20260629-imx-se-if-v26-0-146446285744@nxp.com?part=5

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 955b7409ff9fc..7deff38c46132 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -67,6 +122,9 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
>  			 * synchronized state.
>  			 */
>  			if (READ_ONCE(priv->waiting_rsp_clbk_hdl.dev_ctx)) {
> +				if (!is_rsp_wait_with_timeout)
> +					return ret;
> +
>  				WRITE_ONCE(se_clbk_hdl->signal_rcvd, true);
>  				wait_uninterruptible = true;
>  				continue;

[Severity: High]
Can this sequence trigger a hung task panic on systems with the hung task
watchdog enabled? 

In ele_msg_rcv(), if a signal interrupts the wait, the code sets 
wait_uninterruptible = true and continues waiting. Since the timeout for
a standard command response is set to 5000 seconds, wouldn't waiting in
TASK_UNINTERRUPTIBLE for this long reliably crash the system?

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 25f9cd5441f90..fb82df5c58d56 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
> @@ -197,11 +204,234 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
>  	return 0;
>  }
>  
> +static void cleanup_se_shared_mem(struct se_if_device_ctx *dev_ctx)
> +{
> +	struct se_shared_mem_mgmt_info *se_shared_mem_mgmt = &dev_ctx->se_shared_mem_mgmt;
> +	struct se_if_priv *priv = dev_ctx->priv;
> +
> +	/* Free non-secure shared buffer. */
> +	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]
Could this leak memory by failing to free the se_buf_desc items in the
pending_in and pending_out lists when the device file descriptor is closed?

[Severity: High]
Can this cause a kernel crash during probe failure? 

If an internal command times out during probe, the internal context 
priv_dev_ctx might be marked fw_busy before shared memory is allocated 
(ptr is NULL). 

Calling dma_free_coherent() with a NULL pointer and a size of 65536 bytes 
could trigger a bug in the DMA subsystem.

> +static void se_dev_ctx_shared_mem_cleanup(struct se_if_device_ctx *dev_ctx)
> +{
> +	struct se_shared_mem_mgmt_info *se_shared_mem_mgmt = &dev_ctx->se_shared_mem_mgmt;
> +	struct list_head *pending_lists[] = {&se_shared_mem_mgmt->pending_in,
> +						&se_shared_mem_mgmt->pending_out};
> +	struct se_buf_desc *b_desc, *temp;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pending_lists); i++) {
> +		list_for_each_entry_safe(b_desc, temp, pending_lists[i], link) {
> +			if (b_desc->shared_buf_ptr)
> +				memset(b_desc->shared_buf_ptr, 0, b_desc->size);

[Severity: High]
Does this zero out the shared DMA memory buffers while the firmware might
still be actively accessing them? 

If ele_msg_send_rcv() times out, this cleanup is called on the error path. 
Modifying active DMA buffers or reusing them for the next command could 
corrupt firmware execution.

> +static void cleanup_dev_ctx(struct se_if_device_ctx *dev_ctx, bool is_fclose)
> +{
> +	scoped_guard(mutex, &dev_ctx->fops_lock) {
> +		if (dev_ctx->cleanup_done)
> +			goto exit;

[Severity: High]
Can this cause an indefinite hang during unbind? 

If userspace is blocked in ele_msg_rcv() waiting for a firmware response, 
it holds fops_lock via scoped_cond_guard in se_ioctl(). This cleanup path 
uninterruptibly blocks trying to acquire the same fops_lock, deadlocking 
the unbind thread until the firmware replies or the timeout expires.

[Severity: Medium]
Does this violate the kernel cleanup subsystem rules? 

Using a goto to jump completely out of a scoped_guard() block creates 
confusing ownership semantics and can lead to bugs. The same issue appears 
in ele_get_info() with __free(kfree).

> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	if (cmd_snd_rcv_rsp_info.rx_buf_sz < sizeof(struct se_msg_hdr) ||
> +	    cmd_snd_rcv_rsp_info.rx_buf_sz > MAX_ALLOWED_RX_MSG_SZ) {
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this validation allow a user to allocate an rx_msg buffer of exactly
4 bytes? 

In se_val_rsp_hdr_n_status(), msg->data[0] (bytes 4-7) is unconditionally 
read. If the command bypasses size-mismatch validation (like 
ELE_DEBUG_DUMP_REQ), this could cause a KASAN out-of-bounds read.

> +	rsp_status_err =
> +		se_val_rsp_hdr_n_status(priv, rx_msg, tx_msg->header.command,
> +					cmd_snd_rcv_rsp_info.rx_buf_sz,
> +					tx_msg->header.ver == priv->if_defs->base_api_ver);
> +
> +	if (!rsp_status_err) {
> +		err = se_dev_ctx_cpy_out_data(dev_ctx);
> +		if (err < 0) {
> +			se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +			return err;
> +		}
> +	}

[Severity: High]
Is there a missing assignment to err here? 

If se_val_rsp_hdr_n_status() fails, rsp_status_err is populated but never 
assigned to err. The function falls through and returns the positive success 
byte count from ele_msg_send_rcv(), silently swallowing the validation error 
and misleading userspace.

> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
> +			       loff_t *ppos)
> +{
[ ... ]
> +	/* We may need to copy the output data to user before
> +	 * delivering the completion message.
> +	 */
> +	err = se_dev_ctx_cpy_out_data(dev_ctx);
> +	if (err < 0)
> +		goto exit;

[Severity: High]
Can this result in a use-after-free and list corruption? 

The fops_lock is dropped before the blocking wait in ele_msg_rcv(). When 
resuming, this code unconditionally accesses the pending_in and pending_out 
lists in se_dev_ctx_cpy_out_data() without reacquiring fops_lock. If the 
device was closed concurrently, the DMA memory and rx_msg could already be 
freed.

> +exit:
> +	priv->cmd_receiver_clbk_hdl.rx_msg_sz = 0;
> +	se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +
> +	return err;
> +}

[Severity: High]
Does clearing rx_msg_sz locklessly here cause lost firmware commands? 

If the wait in ele_msg_rcv() is interrupted by a signal, this jumps to exit 
and clears the size. If a mailbox interrupt concurrently receives a message 
and sets the size, this will overwrite it with 0. When userspace restarts 
the read(), it will reject the valid message because rx_msg_sz is 0.

> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
[ ... ]
> +	if (misc_dev_ctx->cleanup_done) {
> +		err = -ENODEV;
> +		goto out_unlock_misc;
> +	}
> +
> +	priv->dev_ctx_mono_count++;
> +	err = init_device_context(priv, priv->dev_ctx_mono_count, &dev_ctx);

[Severity: High]
Is there a TOCTOU race here against unbind cleanup? 

The fops_lock is dropped right after checking cleanup_done, but before 
init_device_context() adds the new context to dev_ctx_list under modify_lock. 
If se_if_probe_cleanup() runs in between, it could unbind the driver, 
resulting in open() returning a file descriptor for a fully unbound device.

> +static void se_if_probe_cleanup(void *plat_dev)
> +{
[ ... ]
> +	cancel_work_sync(&priv->fw_busy_work);
> +	/*
> +	 * A timed-out synchronous command may have retained a dev_ctx through
> +	 * priv->fw_busy_dev_ctx even after the fd was closed and the context was
> +	 * removed from dev_ctx_list. If no late response arrived, release that
> +	 * retained context during driver teardown.
> +	 *
> +	 * se_clear_fw_busy() is idempotent and internally checks
> +	 * priv->fw_busy_dev_ctx under fw_busy_lock.
> +	 */
> +	se_clear_fw_busy(priv);

[Severity: High]
Could this race with a late mailbox interrupt to cause a use-after-free? 

cancel_work_sync() is called before se_clear_fw_busy() clears the fw_busy
state flag. If a mailbox interrupt arrives between these two calls, it will
see fw_busy is still set and call schedule_work() on the soon-to-be-freed
priv->fw_busy_work structure.

> @@ -312,15 +1449,22 @@ static int se_if_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret,
>  				    "Failed to init reserved memory region.");
>  	}
>  
> -	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx);
> +	ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx, &se_if_fops);
>  	if (ret)
>  		return dev_err_probe(dev, ret,
>  				     "Failed[0x%x] to create device contexts.",

[Severity: Medium]
Could this expose the device node to userspace before probe initialization
completes? 

misc_register() is called inside init_misc_device_context(), but
se_if_probe() continues to initialize load_fw and SoC info after this returns.
If userspace immediately opens the device and sends a command, it could bypass
the se_load_firmware() step.

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