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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Calling `of_reserved_mem_device_release` in `se_if_priv_release` causes DMA configuration corruption on driver rebind.
- [High] Use-after-free of `priv_dev_ctx` and its `fops_lock` because `se_if_priv_release` frees the context before cancelling `fw_busy_work`.
- [High] Worker thread deadlocks in `cancel_work_sync()` when dropping the last reference to `priv` from `fw_busy_work`.
--

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

firmware: imx: adds miscdev

This commit adds the driver for communication with the secure-enclave,
enabling message exchange from user-space applications via a character
driver. It includes various checks on incoming message headers to block
invalid commands.

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c

[ ... ]

> +static void se_if_priv_release(struct kref *kref)
> +{
> +	struct se_if_priv *priv = container_of(kref, struct se_if_priv, refcount);
> +
> +	/* Free priv_dev_ctx if it exists */
> +	if (priv->priv_dev_ctx) {

[ ... ]

> +		kfree(priv->priv_dev_ctx->devname);
> +		kfree(priv->priv_dev_ctx);
> +		priv->priv_dev_ctx = NULL;
> +	}
> +	/*
> +	 * No need to check, if reserved memory is allocated
> +	 * before calling for its release. Or clearing the
> +	 * un-set bit.
> +	 */
> +	of_reserved_mem_device_release(priv->dev);

[Severity: High]
Is it safe to release the DMA configuration from the shared platform device
here?

If an unprivileged userspace process keeps the device file descriptor open,
it holds a reference to the priv object. If the device is unbound and then
rebound via sysfs, the new driver instance will initialize its DMA memory
region via of_reserved_mem_device_init().

When the userspace process eventually closes the old file descriptor, this
se_if_priv_release() function in drivers/firmware/imx/se_ctrl.c will execute.
Since priv->dev still points to the shared underlying struct device singleton,
calling of_reserved_mem_device_release() would drop the active DMA
configuration out from under the newly bound, active driver.

Could this lead to DMA allocation failures or memory corruption for the
active instance?

[ ... ]

> +	cancel_work_sync(&priv->fw_busy_work);

[Severity: High]
Does this cancellation sequence introduce a use-after-free and a deadlock?

If an internal command times out on priv_dev_ctx, it is marked busy and
fw_busy_work begins executing se_clear_fw_busy() on a worker thread. If the
device concurrently unbinds, se_if_probe_cleanup() drops the initial priv
refcount.

If no other file descriptors are open, se_if_priv_release() in 
drivers/firmware/imx/se_ctrl.c runs and frees priv->priv_dev_ctx directly via
kfree() earlier in this function before calling cancel_work_sync() here. The
still-running worker thread will then try to acquire dev_ctx->fops_lock in
se_clear_fw_busy(), which was just freed, causing a use-after-free.

Additionally, if a userspace context times out and its file descriptor is
closed concurrently with device unbind, se_clear_fw_busy() drops the last
reference to dev_ctx. This invokes se_if_dev_ctx_release(), which drops the
last reference to priv, invoking se_if_priv_release() inline from the worker
thread. When it reaches cancel_work_sync() here, the worker thread will
synchronously wait for its own completion.

Could this structurally guaranteed deadlock be avoided by canceling the work
earlier, or by structuring the refcounts to prevent the worker thread from
triggering the final release?

> +
> +	/* Free any remaining resources that weren't devm-managed */
> +	kfree(priv);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-imx-se-if-v47-0-b474ec6fc52a@nxp.com?part=5