Re: [PATCH v28 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:

New issues:
- [Critical] ele_get_info() uses an uninitialized variable for memory size and fails to check for allocation failure.
- [High] Unconditional and lockless clearing of rx_msg_sz in se_if_fops_read causes state corruption and message loss.
- [High] Returning -ERESTARTSYS after a successful firmware transaction causes non-idempotent commands to be re-executed and discards successful responses.
- [High] se_if_fops_read() performs shared memory cleanup without holding the fops_lock, leading to double-free and list corruption.
- [High] Mailbox channels are freed before the misc device is deregistered due to inverted devres ordering, allowing use-after-free on channels.
- [High] TOCTOU race between se_if_fops_open and se_if_probe_cleanup allows a device context to escape teardown and access freed hardware resources.
- [High] init_misc_device_context() leaks DMA memory on error paths and falsely reports success if gate allocation fails.
- [High] The DMA coherent memory allocated for priv_dev_ctx is never freed during driver teardown.
- [High] ele_get_info_cleanup() frees gen_pool memory immediately on timeout, allowing the firmware to corrupt reallocated memory.
- [High] Data race and memory corruption in shared memory allocator get_shared_mem_slot() for priv_dev_ctx.
- [High] TOCTOU race in se_if_fops_read allows dereferencing a freed rx_msg buffer.
- [High] The misc device is registered before driver probe completes, exposing uninitialized state.

Pre-existing issues:
- [High] User IOCTL interrupted by a signal can trigger the hung-task watchdog due to an excessive 5000-second uninterruptible sleep.
--

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

firmware: drivers: imx: adds miscdev

Adds the driver for communication interface to secure-enclave, enabling
userspace applications via a character driver. It implements the ABI for
shared memory management, command sending, and file operations.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
> @@ -32,6 +32,9 @@ static void ele_get_info_cleanup(struct se_if_priv *priv, u32 *buf, dma_addr_t d
>  				 size_t size)
>  {
>  	if (priv->mem_pool)
>  		gen_pool_free(priv->mem_pool, (unsigned long)buf, size);

[Severity: High]
Does this unconditionally free the DMA buffer when priv->mem_pool is true,
even if the firmware is still busy processing a timed-out command?
    
If the enclave command times out, the firmware might still perform delayed
DMA writes to this buffer. Releasing it immediately allows the memory to be
reallocated while the hardware is still accessing it, potentially leading
to memory corruption.
 
[ ... ]

> @@ -74,9 +74,9 @@ int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
>  
>  	if (priv->mem_pool) {
>  		get_info_data = gen_pool_dma_alloc(priv->mem_pool,
>  						   ELE_GET_INFO_BUFF_SZ,
>  						   &get_info_addr);
>  	} else {

[Severity: Critical]
Does this leave get_info_len uninitialized and skip checking if get_info_data
is NULL when priv->mem_pool is true?

If gen_pool_dma_alloc() fails, the code continues and calls memcpy() on a
NULL pointer. 

Further down, it passes the uninitialized get_info_len to
ele_get_info_cleanup(), which would cause gen_pool_free() to corrupt the
allocator state by freeing a random size.

[ ... ]

> 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
> @@ -107,6 +107,10 @@ int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk
>  		if (wait_uninterruptible)
>  			ret = wait_for_completion_timeout(&se_clbk_hdl->done,
>  							  remaining_jiffies);

[Severity: High]
This is a pre-existing issue, but can this trigger the hung-task watchdog
when a user IOCTL is interrupted by a signal?
    
The userspace contexts are assigned a timeout of 5,000,000 ms in
init_device_context(). 

If wait_for_completion_interruptible_timeout() returns -ERESTARTSYS, it
switches to wait_uninterruptible for the remainder of that 5000-second
timeout. If the hardware does not respond, this uninterruptible sleep will
exceed the 120-second kernel hung-task threshold and panic the system.
 
[ ... ]

> @@ -216,6 +216,8 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  		     int tx_msg_sz, void *rx_msg, int exp_rx_msg_sz)
>  {
[ ... ]
> +		if (err > 0)
> +			err = -ERESTARTSYS;

[Severity: High]
Does returning -ERESTARTSYS here discard a successful firmware response
and cause non-idempotent commands to be re-executed?

If a signal is received but the firmware transaction still completes
successfully, the success code (bytes received) appears to be overwritten
with -ERESTARTSYS near the end of this function. 

This would cause the kernel to auto-restart the IOCTL, re-executing the
command on the hardware, which could corrupt state for operations like key
generation or lifecycle changes.
 
[ ... ]

> 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
> @@ -487,14 +487,18 @@ static int init_misc_device_context(struct se_if_priv *priv, int ch_id,
>  	gate = kzalloc_obj(*gate, GFP_KERNEL);
>  	if (!gate)
>  		goto exit;
>  
>  [ ... ]
>  exit:
>  	*new_dev_ctx = NULL;
>  	if (gate) {
>  		priv->open_gate = NULL;
>  		se_if_open_gate_put(gate);
>  	}
>  	kfree(dev_ctx->devname);
>  	kfree(dev_ctx);
>  	return dev_err_probe(priv->dev, ret, "%s", err_str);
>  }

[Severity: High]
Can this leak DMA memory and falsely report success if the gate allocation
fails?
    
If kzalloc_obj() for gate fails, the code jumps to the exit label where
ret is still 0 (from the successful init_se_shared_mem() call).

The function returns 0, leading se_if_probe() to interpret it as a
success, but the 128KB DMA buffer allocated for the shared memory is
never cleaned up. Subsequent driver operations might then trigger a NULL
pointer dereference since priv_dev_ctx is NULL.

[ ... ]

> @@ -531,10 +531,9 @@ static void se_if_priv_release(struct kref *kref)
>  	if (priv->priv_dev_ctx) {
>  		kfree(priv->priv_dev_ctx->devname);
>  		kfree(priv->priv_dev_ctx);
>  		priv->priv_dev_ctx = NULL;
>  	}

[Severity: High]
Is the DMA coherent memory allocated for priv_dev_ctx leaked here?
    
During probe, init_misc_device_context() allocates 128KB of DMA shared
memory for priv_dev_ctx. When se_if_priv_release() frees the structure,
it seems to skip calling cleanup_se_shared_mem(), permanently leaking
this memory whenever the module is unloaded or probe fails.
 
[ ... ]

> @@ -993,6 +993,8 @@ int get_shared_mem_slot(struct se_if_device_ctx *dev_ctx,
>  	/* Allocate space in shared memory. 8 bytes aligned. */
>  	pos = shared_mem->pos;
>  	shared_mem->pos += aligned_len;

[Severity: High]
Can this lockless increment of shared_mem->pos cause a data race and
memory corruption when called for the internal priv_dev_ctx?
    
The userspace IOCTLs protect this via dev_ctx->fops_lock, but kernel
threads calling ele_get_info() on priv->priv_dev_ctx do not acquire this
lock. 

If a concurrent operation (like se_load_firmware) happens at the
same time as ele_get_info, both threads could modify pos simultaneously,
causing overlapping DMA memory allocations and payload corruption.
 
[ ... ]

> @@ -1180,6 +1180,9 @@ static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
>  		if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
>  			priv->cmd_receiver_clbk_hdl.rx_msg_sz = 0;
>  			se_dev_ctx_shared_mem_cleanup(dev_ctx);
>  			return -EINVAL;
>  		}

[Severity: High]
Does this allow unprivileged processes to locklessly corrupt the command
receiver's state?
    
If the calling dev_ctx is not the registered command receiver,
rx_msg_sz is cleared without holding clbk_rx_lock. This allows any
process capable of calling read() to disrupt the NVM daemon's state.

[ ... ]

> @@ -1222,6 +1222,8 @@ static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
>  	spin_unlock_irqrestore(&priv->cmd_receiver_clbk_hdl.clbk_rx_lock, flags);
>  
>  	/* 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_mutex_unlock;
>  
>  	/* Copy data from the buffer */
>  	print_hex_dump_debug("to user ", DUMP_PREFIX_OFFSET, 4, 4,
>  			     priv->cmd_receiver_clbk_hdl.rx_msg,
>  			     priv->cmd_receiver_clbk_hdl.rx_msg_sz,
>  			     false);
>  
>  	copy_len = min(size, priv->cmd_receiver_clbk_hdl.rx_msg_sz);
>  
>  	if (copy_to_user(buf, priv->cmd_receiver_clbk_hdl.rx_msg, copy_len))

[Severity: High]
Can this lead to a use-after-free of the rx_msg buffer?
    
The rx_msg pointer is validated while holding clbk_rx_lock, but the lock
is dropped before passing rx_msg to copy_to_user(). 

If driver unbind runs concurrently, unset_dev_ctx_as_command_receiver()
will acquire the lock, set rx_msg to NULL, and immediately free the buffer.
The read() path will then copy freed kernel memory to userspace.
 
[ ... ]

> @@ -1248,6 +1248,8 @@ exit_mutex_unlock:
>  	mutex_unlock(&dev_ctx->fops_lock);
>  
>  exit:
>  	spin_lock_irqsave(&priv->cmd_receiver_clbk_hdl.clbk_rx_lock, flags);
>  	priv->cmd_receiver_clbk_hdl.rx_msg_sz = 0;
>  	spin_unlock_irqrestore(&priv->cmd_receiver_clbk_hdl.clbk_rx_lock, flags);
>  	se_dev_ctx_shared_mem_cleanup(dev_ctx);

[Severity: High]
Does this unconditionally clear rx_msg_sz and perform lockless shared memory
cleanup on the exit path?
    
If ele_msg_rcv() is interrupted by a signal, rx_msg_sz is cleared, which
will permanently drop a concurrently arriving message by overwriting its
size with 0. 
    
Additionally, se_dev_ctx_shared_mem_cleanup() modifies the pending lists
and frees their entries without holding fops_lock. If se_if_fops_close()
runs concurrently (e.g. process termination), it can cause list corruption
and double-frees.

[ ... ]

> @@ -1282,10 +1282,11 @@ static int se_if_fops_open(struct inode *nd, struct file *fp)
>  	priv = gate->priv;
>  	mutex_unlock(&gate->lock);
>  
>  	misc_dev_ctx = priv->priv_dev_ctx;
>  
>  	if (mutex_lock_interruptible(&misc_dev_ctx->fops_lock)) {
>  		err = -EBUSY;
>  		goto out_put_priv;
>  	}
>  
>  	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 race condition here that allows a new context to escape teardown?
    
By dropping gate->lock before creating and adding the new device context
to priv->dev_ctx_list, se_if_probe_cleanup() can concurrently drain all
existing contexts. 

After teardown finishes, this function will append the newly allocated
context to the list and return a valid fd to userspace. This would allow
userspace to access hardware resources like mailbox channels that have
already been destroyed by devres.

[ ... ]

> @@ -1546,6 +1546,9 @@ static int se_if_probe(struct platform_device *pdev)
>  
>  	ret = devm_add_action_or_reset(dev, se_if_probe_cleanup, pdev);
>  	if (ret)
>  		return ret;
>  
>  	/* 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);

[Severity: High]
Will devres free the mailbox channels before deregistering the misc device?
    
Because se_if_probe_cleanup is registered as a devres action before the
mailbox channels are requested, it will run after the channels are freed
(LIFO order). 

During this window, the character device is still active. If userspace
issues a write() or IOCTL, the driver will use the freed channel pointers,
resulting in a use-after-free.

[ ... ]

> @@ -1588,6 +1588,11 @@ static int se_if_probe(struct platform_device *pdev)
>  	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.",
>  				     ret);
>  
>  	if (if_node->if_defs.se_if_type == SE_TYPE_ID_HSM) {
>  		ret = get_se_soc_info(priv, se_info);
>  		if (ret)
>  			return dev_err_probe(dev, ret, "Failed to fetch SoC Info.");
>  	}
>  
>  	/* By default, there is no pending FW to be loaded.*/
>  	if (se_info->se_fw_img_nm.seco_fw_nm_in_rfs) {
>  		load_fw = get_load_fw_instance(priv);
>  		mutex_init(&load_fw->load_fw_lock);

[Severity: High]
Is the misc device exposed to userspace before critical state is initialized?
    
init_misc_device_context() registers the misc device, making it instantly
available. However, load_fw->load_fw_lock and var_se_info.soc_rev are
initialized later in the probe sequence. 

A fast userspace process could open the device and trigger
se_load_firmware() via an IOCTL, which would attempt to lock the
uninitialized load_fw_lock, causing a panic.

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