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

Ignore the last response.

Resending the disposition email.

Pankaj

[ ... ]

quoted

+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) {

[ ... ]

quoted

+           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?

Accepted. Will fix in v48.
- of_reserved_mem_device_release(dev): moved from se_if_priv_release() to
  se_if_probe_cleanup(), after cleanup_se_shared_mem() and before the
  initial kref_put.
Problem with old placement:
  - Last-fd-close could call the release on a struct device that a newly
    rebound driver instance had already reconfigured via
    of_reserved_mem_device_init(), corrupting the new instance's DMA setup.
    Moving it to probe_cleanup ensures the release always pairs with the
    init at unbind time, serialized with re-probe by the driver core.>

[ ... ]

quoted

+   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?

quoted

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

Accepted. Will fix in v48.
Moved cancel_work_sync(&priv->fw_busy_work) from se_if_priv_release() to se_if_probe_cleanup(), after se_clear_fw_busy() and before the initial kref_put.
Fixes two races that existed when it was called at last-fd-close:
  - Worker self-deadlock: fw_busy_work -> se_clear_fw_busy() ->
    kref_put(dev_ctx) -> se_if_dev_ctx_release() -> kref_put(priv)
    could reach zero and invoke se_if_priv_release() from inside
    the worker itself. cancel_work_sync() would then wait for the
    current thread to finish, hanging the kernel.

  - UAF of priv_dev_ctx->fops_lock: se_if_priv_release() freed
    priv->priv_dev_ctx (including its fops_lock) before calling
    cancel_work_sync(). A concurrently running fw_busy_work with
    dev_ctx == priv_dev_ctx would then acquire the already-freed
    mutex, causing use-after-free memory corruption.

  - Moving the cancel to probe_cleanup keeps probe_cleanup's own priv
    ref alive across the cancel, so the worker cannot trigger
    se_if_priv_release(). going_away is set at the top of
    probe_cleanup under clbk_rx_lock, preventing any new
    schedule_work() after the cancel returns.

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

NXP Confidential