[PATCH -next] firmware: imx: se_ctrl: fix init_device_context() error path

[PATCH -next] firmware: imx: se_ctrl: fix init_device_context() error path

[Pankaj Gupta]

init_device_context() adds dev_ctx to priv->dev_ctx_list before
init_se_shared_mem() succeeds. If init_se_shared_mem() fails, the code
frees dev_ctx but still leaves the list insertion and active count update
in the function flow.

Move the list insertion and active device context count update to the
success path only, and return immediately after cleanup on error.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202605090441.GzJS1R6y-lkp@intel.com/
Fixes: 106ffe5d78ad ("firmware: imx: add driver for NXP EdgeLock Enclave")
Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/firmware/imx/se_ctrl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
index 1683b259864f..74786ebaaa89 100644
--- a/drivers/firmware/imx/se_ctrl.c
+++ b/drivers/firmware/imx/se_ctrl.c
@@ -481,16 +481,17 @@ static int init_device_context(struct se_if_priv *priv, int ch_id,
 	dev_ctx->priv = priv;
 	*new_dev_ctx = dev_ctx;
 
-	list_add_tail(&dev_ctx->link, &priv->dev_ctx_list);
-	priv->active_devctx_count++;
-
 	ret = init_se_shared_mem(dev_ctx);
 	if (ret < 0) {
 		kfree(dev_ctx->devname);
 		kfree(dev_ctx);
 		*new_dev_ctx = NULL;
+		return ret;
 	}
 
+	list_add_tail(&dev_ctx->link, &priv->dev_ctx_list);
+	priv->active_devctx_count++;
+
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH -next] firmware: imx: se_ctrl: fix init_device_context() error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Use-After-Free of `dev_ctx` during implicit `mutex_unlock` in `se_if_fops_close()`.
- [High] Data race and list corruption on `priv->dev_ctx_list` due to mismatched locks.
- [High] Permanent resource leak and Denial of Service (DoS) in `.release` handler due to interruptible locking.
- [High] Double free, list poisoning, and Use-After-Free vulnerabilities during driver unbind.
- [High] Use-After-Free of asynchronous mailbox messages in `se_if_fops_write()`.
- [High] Data race and memory leak in `se_ioctl()` `SE_IOCTL_ENABLE_CMD_RCV` command.
--

commit 9c0909012d811bb09f3480d4ef3613f8e888ad7a
Author: Pankaj Gupta <pankaj.gupta@nxp.com>
firmware: imx: se_ctrl: fix init_device_context() error path

This commit moves the addition of the new device context to the list 
and the increment of the active count to the success path in 
init_device_context(). It ensures that the insertion and count updates 
are not left behind if init_se_shared_mem() fails.

Link: https://lore.kernel.org/oe-kbuild-all/202605090441.GzJS1R6y-lkp@intel.com/

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index f88e581fc147e..e68ad57c9073f 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
> @@ -481,16 +481,17 @@ static int init_device_context(struct se_if_priv *priv, int ch_id,
>  	dev_ctx->priv = priv;
>  	*new_dev_ctx = dev_ctx;
>  
> -	list_add_tail(&dev_ctx->link, &priv->dev_ctx_list);
> -	priv->active_devctx_count++;
> -
>  	ret = init_se_shared_mem(dev_ctx);
>  	if (ret < 0) {
>  		kfree(dev_ctx->devname);
>  		kfree(dev_ctx);
>  		*new_dev_ctx = NULL;
> +		return ret;
>  	}
>  
> +	list_add_tail(&dev_ctx->link, &priv->dev_ctx_list);

Can this list manipulation cause a data race and list corruption? 
init_device_context() is called by se_if_fops_open() which holds the global 
misc_dev_ctx->fops_lock to protect this list insertion. However, 
se_if_fops_close() calls list_del(&dev_ctx->link) while holding only the 
per-file lock dev_ctx->fops_lock:

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);
        ...
}

If multiple files are closed concurrently, or an open and close happen at the 
same time, couldn't this corrupt the doubly linked list?

> +	priv->active_devctx_count++;
> +
>  	return ret;
>  }

This isn't a bug in the diff, but is there a use-after-free in 
se_if_fops_close()?

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

Because scoped_cond_guard automatically calls mutex_unlock(&dev_ctx->fops_lock)
when the scope block terminates, won't it unlock the mutex after 
kfree(dev_ctx) has already freed the memory containing the lock?


This isn't a bug in the diff, but does the interruptible lock cause a 
permanent memory leak and potential denial of service when closing the file?

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

	return 0;
}

If the calling process has a pending signal, the lock acquisition fails and 
returns -EBUSY, skipping the kfree and DMA cleanup. Since the VFS ignores 
the return value of the release handler and destroys the file unconditionally, 
does this permanently leak dev_ctx and its DMA memory, allowing an attacker 
to exhaust system memory by repeatedly opening and forcefully closing the 
device?


This isn't a bug in the diff, but could driver unbind cause a double free 
or list poisoning?

During driver unbind, devres automatically frees the priv structure. 
Then se_if_probe_cleanup() iterates over the list:

drivers/firmware/imx/se_ctrl.c:se_if_probe_cleanup() {
    ...
	if (priv->dev_ctx_mono_count) {
		list_for_each_entry_safe(dev_ctx, t_dev_ctx, &priv->dev_ctx_list, link) {
			list_del(&dev_ctx->link);
			priv->active_devctx_count--;
		}
	}
    ...
}

Since it removes the links but doesn't free dev_ctx or revoke open file 
descriptors, if a userspace process later closes a surviving file descriptor, 
won't se_if_fops_close() call list_del(&dev_ctx->link) again on the removed 
node, and also dereference the already-freed dev_ctx->priv structure?


This isn't a bug in the diff, but is there a use-after-free for asynchronous 
mailbox messages?

In se_if_fops_write(), tx_msg is allocated and queued:

drivers/firmware/imx/se_ctrl.c:se_if_fops_write() {
    ...
		struct se_api_msg *tx_msg __free(kfree) = memdup_user(buf, size);
		if (IS_ERR(tx_msg))
			return PTR_ERR(tx_msg);
        ...
		err = ele_msg_send(dev_ctx, tx_msg, size);

		return err;
	}
}

The mailbox channel is non-blocking (priv->se_mb_cl.tx_block = false), so 
the framework can queue the message pointer asynchronously. Because tx_msg 
uses the __free(kfree) attribute, it is immediately freed upon returning. 
When the mailbox controller processes the queue later, won't it read from 
the freed memory?


This isn't a bug in the diff, but does SE_IOCTL_ENABLE_CMD_RCV have a data 
race?

The command modifies the global driver state:

drivers/firmware/imx/se_ctrl.c:se_ioctl() {
    ...
	scoped_cond_guard(mutex_intr, return -EBUSY, &dev_ctx->fops_lock) {
		switch (cmd) {
		case SE_IOCTL_ENABLE_CMD_RCV:
			if (!priv->cmd_receiver_clbk_hdl.dev_ctx) {
				if (!priv->cmd_receiver_clbk_hdl.rx_msg) {
					priv->cmd_receiver_clbk_hdl.rx_msg =
						kzalloc(MAX_NVM_MSG_LEN,
							GFP_KERNEL);
        ...
				priv->cmd_receiver_clbk_hdl.dev_ctx = dev_ctx;
    ...
}

Since this section is only protected by the per-file lock dev_ctx->fops_lock, 
if two processes invoke this ioctl concurrently, they hold different locks. 
Can they both bypass the !priv->cmd_receiver_clbk_hdl.dev_ctx check, leading 
to concurrent kzalloc allocations that overwrite the shared pointer and leak 
memory?

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