[PATCH v2] misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

[PATCH v2] misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

[Anandu Krishnan E]

There is a race between fastrpc_device_release() and the workqueue
that processes DSP responses. When the user closes the file descriptor,
fastrpc_device_release() frees the fastrpc_user structure. Concurrently,
an in-flight DSP invocation can complete and fastrpc_rpmsg_callback()
schedules context cleanup via schedule_work(&ctx->put_work). If the
workqueue runs fastrpc_context_free() in parallel with or after
fastrpc_device_release() has freed the user structure, it dereferences
the freed fastrpc_user. Depending on the state of the context at the
time of the race, any one of the following accesses can be hit:

 1. fastrpc_buf_free() calls fastrpc_ipa_to_dma_addr(buf->fl->cctx, ...)
    to strip the SID bits from the stored IOVA before passing the
    physical address to dma_free_coherent().

 2. fastrpc_free_map() reads map->fl->cctx->vmperms[0].vmid to
    reconstruct the source permission bitmask needed for the
    qcom_scm_assign_mem() call that returns memory from the DSP VM
    back to HLOS.

 3. fastrpc_free_map() acquires map->fl->lock to safely remove the
    map node from the fl->maps list.

The resulting use-after-free manifests as:

  pc : fastrpc_buf_free+0x38/0x80 [fastrpc]
  lr : fastrpc_context_free+0xa8/0x1b0 [fastrpc]
  fastrpc_context_free+0xa8/0x1b0 [fastrpc]
  fastrpc_context_put_wq+0x78/0xa0 [fastrpc]
  process_one_work+0x180/0x450
  worker_thread+0x26c/0x388

Add kref-based reference counting to fastrpc_user. Have each invoke
context take a reference on the user at allocation time and release it
when the context is freed. Release the initial reference in
fastrpc_device_release() at file close. Move the teardown of the user
structure — freeing pending contexts, maps, mmaps, and the channel
context reference — into the kref release callback fastrpc_user_free(),
so that it runs only when the last reference is dropped, regardless of
whether that happens at device close or after the final in-flight
context completes.

Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
Cc: stable@kernel.org
Signed-off-by: Anandu Krishnan E <anandu.e@oss.qualcomm.com>
---
Changes in v2:
 - Rewrote commit message to establish the problem first per review
   feedback; identified all three UAF dereference sites explicitly
 - Moved resource cleanup (pending contexts, maps, mmaps) into
   fastrpc_user_free() so teardown is consolidated in the kref
   release callback
- Link to v1: https://lore.kernel.org/all/20260226151121.818852-1-anandu.e@oss.qualcomm.com/

 drivers/misc/fastrpc.c | 75 +++++++++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 22 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1080f9acf70a..7afbd470c9fd 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -310,6 +310,8 @@ struct fastrpc_user {
 	spinlock_t lock;
 	/* lock for allocations */
 	struct mutex mutex;
+	/* Reference count */
+	struct kref refcount;
 };
 
 /* Extract SMMU PA from consolidated IOVA */
@@ -497,15 +499,57 @@ static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
 	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
 }
 
+static void fastrpc_context_put(struct fastrpc_invoke_ctx *ctx);
+
+static void fastrpc_user_free(struct kref *ref)
+{
+	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);
+	struct fastrpc_invoke_ctx *ctx, *n;
+	struct fastrpc_map *map, *m;
+	struct fastrpc_buf *buf, *b;
+
+	if (fl->init_mem)
+		fastrpc_buf_free(fl->init_mem);
+
+	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
+		list_del(&ctx->node);
+		fastrpc_context_put(ctx);
+	}
+
+	list_for_each_entry_safe(map, m, &fl->maps, node)
+		fastrpc_map_put(map);
+
+	list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+		list_del(&buf->node);
+		fastrpc_buf_free(buf);
+	}
+
+	fastrpc_channel_ctx_put(fl->cctx);
+	mutex_destroy(&fl->mutex);
+	kfree(fl);
+}
+
+static void fastrpc_user_get(struct fastrpc_user *fl)
+{
+	kref_get(&fl->refcount);
+}
+
+static void fastrpc_user_put(struct fastrpc_user *fl)
+{
+	kref_put(&fl->refcount, fastrpc_user_free);
+}
+
 static void fastrpc_context_free(struct kref *ref)
 {
 	struct fastrpc_invoke_ctx *ctx;
 	struct fastrpc_channel_ctx *cctx;
+	struct fastrpc_user *fl;
 	unsigned long flags;
 	int i;
 
 	ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount);
 	cctx = ctx->cctx;
+	fl = ctx->fl;
 
 	for (i = 0; i < ctx->nbufs; i++)
 		fastrpc_map_put(ctx->maps[i]);
@@ -522,6 +566,8 @@ static void fastrpc_context_free(struct kref *ref)
 	kfree(ctx);
 
 	fastrpc_channel_ctx_put(cctx);
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 }
 
 static void fastrpc_context_get(struct fastrpc_invoke_ctx *ctx)
@@ -628,6 +674,8 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 
 	/* Released in fastrpc_context_put() */
 	fastrpc_channel_ctx_get(cctx);
+	/* Take a reference to user, released in fastrpc_context_free() */
+	fastrpc_user_get(user);
 
 	ctx->sc = sc;
 	ctx->retval = -1;
@@ -659,6 +707,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	list_del(&ctx->node);
 	spin_unlock(&user->lock);
 	fastrpc_channel_ctx_put(cctx);
+	fastrpc_user_put(user);
 	kfree(ctx->maps);
 	kfree(ctx->olaps);
 	kfree(ctx);
@@ -1579,9 +1628,6 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 {
 	struct fastrpc_user *fl = (struct fastrpc_user *)file->private_data;
 	struct fastrpc_channel_ctx *cctx = fl->cctx;
-	struct fastrpc_invoke_ctx *ctx, *n;
-	struct fastrpc_map *map, *m;
-	struct fastrpc_buf *buf, *b;
 	unsigned long flags;
 
 	fastrpc_release_current_dsp_process(fl);
@@ -1590,29 +1636,13 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	list_del(&fl->user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
-	if (fl->init_mem)
-		fastrpc_buf_free(fl->init_mem);
-
-	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
-		list_del(&ctx->node);
-		fastrpc_context_put(ctx);
-	}
-
-	list_for_each_entry_safe(map, m, &fl->maps, node)
-		fastrpc_map_put(map);
-
-	list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
-		list_del(&buf->node);
-		fastrpc_buf_free(buf);
-	}
-
 	fastrpc_session_free(cctx, fl->sctx);
-	fastrpc_channel_ctx_put(cctx);
 
-	mutex_destroy(&fl->mutex);
-	kfree(fl);
 	file->private_data = NULL;
 
+	/* Release the reference taken in fastrpc_device_open */
+	fastrpc_user_put(fl);
+
 	return 0;
 }
 
@@ -1655,6 +1685,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	spin_lock_irqsave(&cctx->lock, flags);
 	list_add_tail(&fl->user, &cctx->users);
 	spin_unlock_irqrestore(&cctx->lock, flags);
+	kref_init(&fl->refcount);
 
 	return 0;
 }
-- 
2.34.1

Claude review: misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

[Claude Code Review Bot]

Overall Series Review

Subject: misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context
Author: Anandu Krishnan E <anandu.e@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-04-30T10:52:17.356682

---

This single-patch series adds kref-based reference counting to `fastrpc_user` to fix a real use-after-free race: when a user closes the file descriptor while a DSP invocation is still in flight, `fastrpc_device_release()` frees the `fastrpc_user` structure, and then the workqueue-scheduled `fastrpc_context_free()` dereferences the freed user through `ctx->fl`. The approach — have each invoke context take a user reference at allocation and release it at context free — is correct in principle and follows the existing kref pattern already used for `fastrpc_channel_ctx` and `fastrpc_invoke_ctx`.

However, the patch has two bugs: a **kref initialization ordering issue** and, more seriously, a **use-after-free on the pending context list** in the new `fastrpc_user_free()`. The latter is a direct consequence of moving the pending-list cleanup into the kref release callback without accounting for the fact that contexts freed via the workqueue path are `kfree()`'d without being removed from `fl->pending`. The result is that `fastrpc_user_free()` iterates a list containing already-freed entries — trading one UAF for another.

The fix is close but needs another revision to address these issues before it can be merged.

---

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

[Claude Code Review Bot]

Patch Review

**Commit message**: Well-written. Clearly establishes the race, identifies all three UAF dereference sites, explains the fix approach, and includes the crash trace. Good use of Fixes/Cc: stable tags.

**kref_init ordering (bug)**

In `fastrpc_device_open()`, the kref is initialized **after** the user is added to the channel's user list:

```c
	spin_lock_irqsave(&cctx->lock, flags);
	list_add_tail(&fl->user, &cctx->users);
	spin_unlock_irqrestore(&cctx->lock, flags);
+	kref_init(&fl->refcount);
```

The refcount must be initialized **before** the structure becomes visible to other code paths. `fastrpc_rpmsg_remove()` traverses `cctx->users` under the same lock, and could race with open. Move `kref_init(&fl->refcount)` before the `list_add_tail`, ideally next to `spin_lock_init`/`mutex_init` where other initialization happens.

**Use-after-free on fl->pending in fastrpc_user_free() (bug)**

This is the most significant issue. The patch moves the pending context cleanup from `fastrpc_device_release()` into the kref release callback `fastrpc_user_free()`:

```c
+static void fastrpc_user_free(struct kref *ref)
+{
+	...
+	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
+		list_del(&ctx->node);
+		fastrpc_context_put(ctx);
+	}
```

But `fastrpc_context_free()` (the context kref release) does `kfree(ctx)` **without** removing `ctx->node` from `fl->pending`, then calls `fastrpc_user_put()`:

```c
 static void fastrpc_context_free(struct kref *ref)
 {
 	...
 	kfree(ctx);
 
 	fastrpc_channel_ctx_put(cctx);
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 }
```

Consider the "abandoned invoke" scenario (invoker interrupted with `-ERESTARTSYS` or timed out with `-ETIMEDOUT`):

1. Invoker sets `is_work_done = true` (or equivalent), does **not** call `list_del(&ctx->node)` or `fastrpc_context_put(ctx)`. Context remains on `fl->pending` with refcount 1; user refcount is 2 (initial + context).
2. `fastrpc_device_release()` calls `fastrpc_user_put()` → user refcount drops to 1.
3. DSP eventually responds; callback schedules `ctx->put_work`.
4. Workqueue runs `fastrpc_context_put_wq()` → `fastrpc_context_free()`:
   - `kfree(ctx)` — context memory freed, but `ctx->node` is **still linked** into `fl->pending`.
   - `fastrpc_user_put(fl)` → user refcount drops to 0 → `fastrpc_user_free()` runs.
5. `fastrpc_user_free()` does `list_for_each_entry_safe` over `fl->pending` — dereferences the just-freed `ctx`. **Use-after-free.**

With the patch's refcounting model, every context on `fl->pending` holds a user reference. For `fastrpc_user_free()` to execute, **all** those contexts must have already been freed (their `fastrpc_context_free()` called `fastrpc_user_put()`). So the pending list iteration in `fastrpc_user_free()` can never find a live context — it either finds an empty list or freed entries.

To fix this, `fastrpc_context_free()` must remove `ctx->node` from `fl->pending` before `kfree(ctx)`. Since the normal invoke path (`fastrpc_internal_invoke` bail label) already does `list_del(&ctx->node)`, you'd need to avoid double-delete. A clean approach:

- Change `fastrpc_internal_invoke`'s `list_del(&ctx->node)` to `list_del_init(&ctx->node)`.
- In `fastrpc_context_free()`, before `kfree(ctx)`, add:
  ```c
  spin_lock(&fl->lock);
  if (!list_empty(&ctx->node))
      list_del(&ctx->node);
  spin_unlock(&fl->lock);
  ```
- Then the `fl->pending` iteration in `fastrpc_user_free()` becomes a safety net for any contexts that somehow weren't cleaned up, rather than a guaranteed UAF.

**fastrpc_session_free ordering (minor observation)**

`fastrpc_session_free(cctx, fl->sctx)` remains in `fastrpc_device_release()` rather than moving to `fastrpc_user_free()`. This means if the user structure outlives `device_release` (because a context still holds a reference), the session is already freed when the deferred cleanup runs. This appears safe since `fastrpc_buf_free()` and `fastrpc_free_map()` dereference `fl->cctx`, not `fl->sctx`, but it deserves an explicit comment noting the intentional ordering.

**Trivial comment nit**

```c
+	/* Reference count */
+	struct kref refcount;
```

This comment adds no information beyond what `struct kref refcount` already conveys. Either drop it or say something about the lifetime contract (e.g., `/* one ref per open + one per in-flight ctx */`).

**Forward declaration**

```c
+static void fastrpc_context_put(struct fastrpc_invoke_ctx *ctx);
```

Needed because `fastrpc_user_free()` is defined before `fastrpc_context_put()`. This is acceptable but worth noting: if the pending list iteration is removed from `fastrpc_user_free()` (because it's unreachable with valid data), this forward declaration can also be removed.

---
Generated by Claude Code Patch Reviewer