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

[sashiko-bot@kernel.org]

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