Re: [PATCH -next] firmware: imx: se_ctrl: detect round_up() overflow in iobuf setup

Re: [PATCH -next] firmware: imx: se_ctrl: detect round_up() overflow in iobuf setup

[sashiko-bot]

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

Pre-existing issues:
- [High] Returning -EBUSY from the file release handler on signal interruption causes permanent memory and resource leaks.
- [High] Concurrent modification of shared linked list `priv->dev_ctx_list` under a per-instance lock leads to list corruption.
- [High] Use-After-Free of `dev_ctx->fops_lock` in `se_if_fops_close` upon scope exit.
- [High] Infinite kernel loop in `ele_msg_rcv` when interrupted by a pending signal.
- [High] Unprivileged `read()` operations corrupt the global command receiver state.
--

commit 274dde5deb429b8b8fa60b9158c528c3a6c4f3bc
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: se_ctrl: detect round_up() overflow in iobuf setup

se_ioctl_setup_iobuf_handler() aligns io.length with round_up(..., 8)
before checking the available shared memory space. On 32-bit builds,
round_up() can overflow for large io.length values and wrap aligned_len
to a smaller value, which can bypass the bounds check while later
memset() still uses the original unbounded io.length.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 8fab3b7767b7a..5b23485fe42c8 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but in se_if_fops_close(), could returning
-EBUSY on signal interruption cause memory and resource leaks?

drivers/firmware/imx/se_ctrl.c:se_if_fops_close() {
    ...
	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
    ...

If the process closing the file has a pending signal, mutex_lock_interruptible
fails and returns -EINTR, causing scoped_cond_guard to immediately return
-EBUSY. 

The VFS layer ignores error returns from release handlers and will not retry.
This drops the file structure but permanently bypasses the cleanup block,
permanently leaking the dev_ctx memory and its associated shared DMA buffers. 

Additionally, if the leaked context was the designated command receiver,
priv->cmd_receiver_clbk_hdl.dev_ctx remains pointing to it forever,
preventing any other process from acquiring the command receiver role.


[Severity: High]
This isn't a bug introduced by this patch, but could this concurrent list
modification lead to corruption?

drivers/firmware/imx/se_ctrl.c:se_if_fops_close() {
    ...
	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
        ...
		list_del(&dev_ctx->link);
        ...

These shared structure modifications are protected solely by dev_ctx->fops_lock,
which is a per-file-descriptor lock local to the current dev_ctx instance. 

If multiple processes close their file descriptors concurrently, each acquires
its own distinct fops_lock and they concurrently execute list_del on the
shared priv->dev_ctx_list.


[Severity: High]
This is a pre-existing issue, but does this scoped_cond_guard usage
trigger a use-after-free?

drivers/firmware/imx/se_ctrl.c:se_if_fops_close() {
    ...
	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
        ...
		kfree(dev_ctx);
	}
    ...

Because dev_ctx is freed inside the guarded block, when the block ends,
the implicitly generated scope destructor executes
mutex_unlock(&dev_ctx->fops_lock).

This dereferences a freed pointer and writes into freed memory to update
the mutex state.


[Severity: High]
This isn't a bug introduced by this patch, but can an unprivileged read()
corrupt the global command receiver state?

drivers/firmware/imx/se_ctrl.c:se_if_fops_read() {
    ...
	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
		if (dev_ctx != priv->cmd_receiver_clbk_hdl.dev_ctx) {
			err = -EINVAL;
			goto exit;
		}
        ...
exit:
		priv->cmd_receiver_clbk_hdl.rx_msg_sz = 0;
        ...

If a process that is not the designated command receiver calls read(), it
gets -EINVAL and jumps to exit, unconditionally clearing the rx_msg_sz field
of the legitimate command receiver.

Could a malicious reader zero out the size concurrently while the legitimate
receiver is fetching a response, causing it to read a corrupted size of 0?


[Severity: High]
This is a pre-existing issue, but can ele_msg_rcv() get stuck in an
infinite loop?

drivers/firmware/imx/ele_common.c:ele_msg_rcv() {
    ...
	do {
        ...
		ret = wait_for_completion_interruptible_timeout(&se_clbk_hdl->done, timeout_ms);
		if (ret == -ERESTARTSYS) {
			if (dev_ctx->priv->waiting_rsp_clbk_hdl.dev_ctx) {
				dev_ctx->priv->waiting_rsp_clbk_hdl.signal_rcvd = true;
				continue;
			}
            ...
		}
        ...
	} while (ret < 0);
    ...

If wait_for_completion_interruptible_timeout() returns -ERESTARTSYS due to
a pending signal, the code sets signal_rcvd and executes continue.

Since the signal remains pending on the kernel thread, the next call to
wait_for_completion_interruptible_timeout() will instantly return
-ERESTARTSYS again, creating an infinite tight loop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611154148.3182727-1-pankaj.gupta@oss.nxp.com?part=1

[PATCH -next] firmware: imx: se_ctrl: detect round_up() overflow in iobuf setup

[pankaj.gupta]

From: Pankaj Gupta <pankaj.gupta@nxp.com>

se_ioctl_setup_iobuf_handler() aligns io.length with round_up(..., 8)
before checking the available shared memory space. On 32-bit builds,
round_up() can overflow for large io.length values and wrap aligned_len
to a smaller value, which can bypass the bounds check while later
memset() still uses the original unbounded io.length.

Detect the overflow by checking whether the aligned value became smaller
than the original length. Valid alignment must never reduce the value,
so this catches wraparound without relying on a SIZE_MAX-based check that
triggers tautological-compare warnings on 64-bit builds

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/firmware/imx/se_ctrl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
index 8fab3b7767b7..5b23485fe42c 100644
--- a/drivers/firmware/imx/se_ctrl.c
+++ b/drivers/firmware/imx/se_ctrl.c
@@ -666,6 +666,11 @@ static int se_ioctl_setup_iobuf_handler(struct se_if_device_ctx *dev_ctx,
 	}
 
 	aligned_len = round_up((size_t)io.length, 8);
+	if (aligned_len < io.length) {
+		dev_err(dev_ctx->priv->dev, "%s: Invalid buffer length.",
+			dev_ctx->devname);
+		return -EINVAL;
+	}
 
 	/* No specific requirement for this buffer. */
 	shared_mem = &dev_ctx->se_shared_mem_mgmt.non_secure_mem;
-- 
2.43.0