* Re: [PATCH -next] firmware: imx: se_ctrl: fix init_device_context() error path
2026-05-14 9:04 [PATCH -next] firmware: imx: se_ctrl: fix init_device_context() error path Pankaj Gupta
@ 2026-05-14 17:51 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-05-14 17:51 UTC (permalink / raw)
To: Pankaj Gupta; +Cc: imx, Frank.Li
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
^ permalink raw reply [flat|nested] 2+ messages in thread