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

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Implicit padding in `struct se_ioctl_cmd_snd_rcv_rsp_info` causes an ABI mismatch between 32-bit userspace and 64-bit kernel.
- [High] The driver returns -EBUSY when file operations are interrupted by a signal, violating standard POSIX syscall semantics.
- [High] If a signal interrupts `se_if_fops_read()` after it successfully receives a message, the message is stranded and the firmware protocol permanently deadlocks.
- [High] A successful firmware transaction is reported to userspace as interrupted (`-EINTR`) if a signal is pending, discarding the successful response payload.
- [High] A TOCTOU race between `ele_msg_send_rcv` and `se_if_probe_cleanup` causes a lost wakeup, resulting in a 1.3-hour unbind hang.
- [High] Kernel crash in the error path of `init_misc_device_context` due to accessing an uninitialized `dev_ctx->priv` pointer.
- [High] Unbounded DMA memory allocation in the device `open` path allows unprivileged users to exhaust contiguous DMA memory.
- [High] The dynamically allocated `se_if_open_gate` structure can be accessed after it is freed if a concurrent `open()` races with device teardown.
--

--- Patch [5]: [PATCH v32 5/7] firmware: imx: adds miscdev ---
commit b46f4f0d3fc0745184ba130cd15fbe6c57e10cbe
Author: Pankaj Gupta <[email protected]>
Subject: firmware: imx: adds miscdev

This commit adds a misc character driver to communicate with NXP secure 
enclave hardware. It enables userspace applications to exchange messages 
with the enclave using an ioctl-based API, handling shared memory management 
and payload verification.

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 7254fc717e91d..7ea7cf0631ec0 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> @@ -188,10 +258,21 @@ int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
>  
>  	guard(mutex)(&priv->se_if_cmd_lock);
>  
> +	/*
> +	 * Teardown has begun: do not arm a new transaction. A thread that was
> +	 * blocked on se_if_cmd_lock while se_if_probe_cleanup() ran its
> +	 * complete_all() would otherwise miss that wake-up, re-arm the
> +	 * completion below and wait the full timeout, while unbind blocks
> +	 * indefinitely on this thread's fops_lock. Bail out instead.
> +	 */
> +	if (atomic_read(&priv->going_away))
> +		return -ENODEV;
> +
>  	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);

[Severity: High]
Could this lead to a TOCTOU race and a lost wakeup during unbind?

In ele_msg_send_rcv(), atomic_read(&priv->going_away) is checked locklessly 
before calling reinit_completion(). Concurrently, se_if_probe_cleanup() 
sets going_away to 1 and calls complete_all() to forcefully wake any waiters:

se_if_probe_cleanup() {
    atomic_set(&priv->going_away, 1);
    ...
    complete_all(&priv->waiting_rsp_clbk_hdl.done);
}

Since se_if_probe_cleanup() does not acquire se_if_cmd_lock, it can execute
immediately after ele_msg_send_rcv() checks going_away. The reinit_completion()
call here would then overwrite the forced wakeup. This leaves ele_msg_rcv()
waiting indefinitely, which can cause the device unbind process to hang.

>  	/* Publish rx_msg/rx_msg_sz under the lock read by se_if_rx_callback(). */
>  	spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
[ ... ]
> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index a8974eef190b6..8698d77172465 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> @@ -302,13 +618,51 @@ static int init_misc_device_context(struct se_if_priv *priv, int ch_id,
>  	if (!dev_ctx->devname)
>  		goto exit;
>  
> +	mutex_init(&dev_ctx->fops_lock);
> +	lockdep_set_class(&dev_ctx->fops_lock, &se_priv_ctx_fops_key);
> +
>  	dev_ctx->priv = priv;
> +	kref_init(&dev_ctx->refcount);
> +	dev_ctx->cleanup_done = false;
>  	*new_dev_ctx = dev_ctx;
> +	set_se_rcv_msg_timeout(dev_ctx, SE_RCV_MSG_DEFAULT_TIMEOUT_MS);
> +
> +	ret = init_se_shared_mem(dev_ctx);
> +	if (ret < 0)
> +		goto exit;
> +
> +	gate = kzalloc_obj(*gate, GFP_KERNEL);
> +	if (!gate) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	mutex_init(&gate->lock);
> +	kref_init(&gate->refcount);    /* device-owned reference */
> +	gate->priv = priv;
> +	gate->dying = false;
> +	priv->open_gate = gate;
> +
> +	/*
> +	 * The miscdevice storage is now owned by the open gate object.
> +	 * priv->priv_dev_ctx still keeps a pointer to that miscdevice.
> +	 */
> +	dev_ctx->miscdev = &gate->miscdev;
> +
> +	dev_ctx->miscdev->name = dev_ctx->devname;
> +	dev_ctx->miscdev->minor = MISC_DYNAMIC_MINOR;
> +	dev_ctx->miscdev->fops = se_if_fops;
> +	dev_ctx->miscdev->parent = priv->dev;
>  
>  	return 0;
>  exit:
>  	*new_dev_ctx = NULL;
>  
> +	if (gate) {
> +		priv->open_gate = NULL;
> +		se_if_open_gate_put(gate);
> +	}
> +	cleanup_se_shared_mem(dev_ctx, true);

[Severity: High]
Can this error path trigger a NULL pointer dereference?

If kasprintf() fails to allocate memory for dev_ctx->devname earlier in
init_misc_device_context(), the code jumps to the exit label. At this point, 
dev_ctx->priv has not yet been assigned and is NULL.

This error path calls cleanup_se_shared_mem(dev_ctx, true), which does:

cleanup_se_shared_mem() {
    struct se_if_priv *priv = dev_ctx->priv;
    ...
    if (priv->mem_pool)
        ...
}

This will unconditionally dereference the uninitialized priv pointer,
causing an immediate kernel panic.

>  	kfree(dev_ctx->devname);
>  	kfree(dev_ctx);
>  	return dev_err_probe(priv->dev, ret, "%s", err_str);
[ ... ]
> +static int se_ioctl_cmd_snd_rcv_rsp_handler(struct se_if_device_ctx *dev_ctx,
> +					    void __user *uarg)
> +{
[ ... ]
> +	err = ele_msg_send_rcv(dev_ctx, tx_msg, cmd_snd_rcv_rsp_info.tx_buf_sz,
> +			       rx_msg, cmd_snd_rcv_rsp_info.rx_buf_sz);
> +	if (err < 0) {
> +		se_ioctl_cmd_snd_rcv_cleanup(dev_ctx, uarg, &cmd_snd_rcv_rsp_info);
> +
> +		/*
> +		 * -ERESTARTSYS here means the wait was interrupted by a signal
> +		 * after the command had already been handed to (and possibly
> +		 * executed by) the firmware. Returning -ERESTARTSYS lets the VFS
> +		 * transparently restart the ioctl, which would re-run the command
> +		 * with the just cleaned-up (zeroed) shared input buffers. Report
> +		 * -EINTR instead so the syscall is not auto-restarted; userspace
> +		 * can decide whether to reissue it.
> +		 */
> +		if (err == -ERESTARTSYS)
> +			err = -EINTR;

[Severity: High]
Is it safe to discard a successful firmware response when a signal is pending?

If a successful firmware transaction was interrupted by a signal, the lower
layers might overwrite the success return code with -ERESTARTSYS (which is
translated to -EINTR here). When this happens, the successful response payload
is discarded in se_ioctl_cmd_snd_rcv_cleanup().

Because the kernel successfully retrieved the response and the firmware has
committed the action, returning -EINTR breaks idempotency for state-mutating
commands. Userspace receives -EINTR and might unsafely retry commands that
were actually successfully executed by the firmware. Should the kernel return
the successful response to userspace instead?

> +
> +		return err;
> +	}
[ ... ]
> +/* Write a message to the MU. */
> +static ssize_t se_if_fops_write(struct file *fp, const char __user *buf,
> +				size_t size, loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	struct se_if_priv *priv;
> +	int err;
> +
> +	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {

[Severity: High]
Should this return -ERESTARTSYS instead of -EBUSY when interrupted?

When the interruptible lock acquisition fails due to a pending signal,
masking the error with -EBUSY breaks the standard POSIX syscall restart
mechanism. Userspace receives a hard error (EBUSY) instead of an
interrupt (EINTR), which prevents applications from properly handling
signals or transparently retrying the syscall.

> +		if (dev_ctx->cleanup_done)
> +			return -ENODEV;
[ ... ]
> +/*
> + * Read a message from the MU.
> + * Blocking until a message is available.
> + */
> +static ssize_t se_if_fops_read(struct file *fp, char __user *buf, size_t size,
> +			       loff_t *ppos)
> +{
> +	struct se_if_device_ctx *dev_ctx = fp->private_data;
> +	u8 rx_msg_snap[MAX_NVM_MSG_LEN];
> +	struct se_if_priv *priv;
> +	unsigned long flags;
> +	size_t copy_len;
> +	int err;
> +
> +	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
> +		priv = dev_ctx->priv;
[ ... ]
> +	err = ele_msg_rcv(dev_ctx, &priv->cmd_receiver_clbk_hdl);
> +	if (err < 0) {
> +		if (err != -ERESTARTSYS)
> +			dev_err(priv->dev,
> +				"%s: Er[0x%x]: Signal Interrupted. Current act-dev-ctx count: %d.",
> +				dev_ctx->devname, err, dev_ctx->priv->active_devctx_count);
> +		return err;
> +	}
> +
> +	/*
> +	 * Reacquire fops_lock before touching any dev_ctx state (pending lists,
> +	 * rx_msg) after the blocking wait. fops_lock was dropped before calling
> +	 * ele_msg_rcv(). If cleanup_dev_ctx() ran concurrently it could have
> +	 * freed the DMA buffers and the pending lists, leading to UAF and list
> +	 * corruption. Re-checking cleanup_done under fops_lock prevents that.
> +	 */
> +	if (mutex_lock_interruptible(&dev_ctx->fops_lock))
> +		return -ERESTARTSYS;

[Severity: High]
Does this risk permanently stranding the message if a signal arrives?

In se_if_fops_read(), the driver blocks in ele_msg_rcv() waiting for a
message from the firmware. When a message arrives, the completion is
consumed and the message is staged in the driver's internal buffer.

The driver then attempts to reacquire the fops_lock here using an
interruptible primitive. If a signal is pending at this exact moment, the
lock acquisition fails and returns -ERESTARTSYS.

Because the completion has already been consumed, the message is left
stranded in the buffer. When userspace retries the read() syscall, it will
block on ele_msg_rcv() again waiting for a new completion. Because the
firmware protocol is half-duplex, it will wait for userspace to process
the already delivered message, causing a permanent deadlock.

> +
> +	if (dev_ctx->cleanup_done) {
> +		mutex_unlock(&dev_ctx->fops_lock);
> +		return -ENODEV;
> +	}
[ ... ]
> +/* Open a character device. */
> +static int se_if_fops_open(struct inode *nd, struct file *fp)
> +{
> +	struct miscdevice *miscdev = fp->private_data;
> +	struct se_if_open_gate *gate;
> +	struct se_if_device_ctx *misc_dev_ctx;
> +	struct se_if_device_ctx *dev_ctx;
> +	struct se_if_priv *priv;
> +	int err = 0;
> +
> +	gate = container_of(miscdev, struct se_if_open_gate, miscdev);
> +
> +	if (!se_if_open_gate_get(gate))

[Severity: High]
Could this lead to a Use-After-Free if a concurrent open races with device
teardown?

During device unbind, se_if_probe_cleanup() calls misc_deregister(). While
misc_deregister() removes the device from the active misc device list, it
does not prevent a racing VFS open (which already found the miscdev but was
preempted before calling fops->open) from continuing.

If unbind completes, se_if_priv_release() drops the final reference to the
open_gate structure, freeing it. When the delayed open call resumes here,
it calculates the gate pointer using container_of() and calls
se_if_open_gate_get(gate). This reads from the freed gate->refcount,
causing a Use-After-Free.

> +		return -ENODEV;
> +
> +	if (mutex_lock_interruptible(&gate->lock)) {
> +		se_if_open_gate_put(gate);
> +		return -EBUSY;
> +	}
[ ... ]
> +	priv->dev_ctx_mono_count++;
> +	err = init_device_context(priv, priv->dev_ctx_mono_count, &dev_ctx);

[Severity: High]
Is there a limit on how much DMA memory can be allocated through this open
path?

When a user process opens the device file, se_if_fops_open() invokes
init_device_context(), which calls init_se_shared_mem(). This unconditionally
allocates 128KB (MAX_DATA_SIZE_PER_USER) of DMA coherent memory using
dma_alloc_coherent() per device context.

Since there is no global limit on the number of dev_ctx instances, a single
process could rapidly open the device repeatedly. This could instantly consume
hundreds of megabytes of contiguous DMA memory, which can exhaust the pool
and prevent other drivers from functioning, leading to a Denial of Service.

> +	if (err) {
> +		dev_err(priv->dev, "Failed[0x%x] to create dev-ctx.", err);
> +		goto out_unlock_misc;
> +	}
[ ... ]
> diff --git a/include/uapi/linux/se_ioctl.h b/include/uapi/linux/se_ioctl.h
> new file mode 100644
> index 0000000000000..ea14cec05020d
> --- /dev/null
> +++ b/include/uapi/linux/se_ioctl.h
> @@ -0,0 +1,97 @@
[ ... ]
> +struct se_ioctl_cmd_snd_rcv_rsp_info {
> +	__u64 tx_buf;
> +	__u32 tx_buf_sz;
> +	__u64 rx_buf;
> +	__u32 rx_buf_sz;
> +};

[Severity: High]
Does this structure introduce an ABI mismatch between 32-bit userspace and
a 64-bit kernel?

The se_ioctl_cmd_snd_rcv_rsp_info struct places a __u64 immediately after
a __u32. On 64-bit architectures, this induces 4 bytes of padding before
the __u64 and at the end of the struct, resulting in a 32-byte struct. On
some 32-bit architectures, __u64 requires only 4-byte alignment, producing
a 24-byte struct.

Because the SE_IOCTL_CMD_SEND_RCV_RSP macro incorporates the struct's size,
the generated IOCTL number will differ between a 32-bit application and the
64-bit kernel. When compat_ptr_ioctl passes the 32-bit IOCTL command to
se_ioctl(), it will fail to match the 64-bit switch case, causing a -ENOTTY
error for 32-bit compat userspace applications.

> +
> +struct se_ioctl_get_soc_info {
> +	__u16 soc_id;
> +	__u16 soc_rev;
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5