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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jul 17, 2026 at 11:12:55AM +0000, sashiko-bot@kernel.org wrote:
> 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.

This is hard to fix.  DMA may still runnings and can't sync with cleanup
function. You can add comments here.

If timeout value is bigger enough, it is statistically impossible

>
> [ ... ]
>
> > @@ -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.

This need check return value.

>
> 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.

doube check this one

>
> [ ... ]
>
> > 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).

Yes, need set ret = -ENOMEN

>
> 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.

most likely correct.

>
> [ ... ]
>
> > @@ -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.

make sense, please check it.

Anyways, it is much better than before. Please continue follow sashiko
comments.

Frank
>
> 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