[PATCH] misc: fastrpc: fix UAF and kernel panic during cleanup on process abort

[PATCH] misc: fastrpc: fix UAF and kernel panic during cleanup on process abort

[Jianping Li]

When a userspace FastRPC client is abruptly terminated, FastRPC
cleanup paths can race with device and session teardown.

This results in kernel panics in different release paths:
- fastrpc_release() when using remote heap, originating from
  fastrpc_buf_free()
- fastrpc_device_release() when using system heap, originating from
  fastrpc_free_map()

In addition, fastrpc_map_put() may trigger refcount use-after-free
due to concurrent cleanup without proper synchronization.

The root cause is that buffer and map cleanup paths may access map
and buf resources after the associated device or session has
already been released.

Fix this by:
- Introducing mutex protection for map and buf lifetime
- Serializing buffer and map cleanup against device teardown
- Skipping buffer and map operations when the device is already gone

These changes ensure cleanup paths are safe against unexpected
process aborts and prevent use-after-free and kernel panic scenarios.

Fixes: c68cfb718c8f9 ("misc: fastrpc: Add support for context Invoke method")
Cc: stable@kernel.org
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 56 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1080f9acf70a..3df771a4a216 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -255,6 +255,8 @@ struct fastrpc_session_ctx {
 	int sid;
 	bool used;
 	bool valid;
+	bool allocated;
+	struct mutex mutex;
 };
 
 struct fastrpc_soc_data {
@@ -333,9 +335,14 @@ static inline u64 fastrpc_sid_offset(struct fastrpc_channel_ctx *cctx,
 static void fastrpc_free_map(struct kref *ref)
 {
 	struct fastrpc_map *map;
+	struct fastrpc_user *fl;
 
 	map = container_of(ref, struct fastrpc_map, refcount);
 
+	fl = map->fl;
+	if (!fl)
+		return;
+
 	if (map->table) {
 		if (map->attr & FASTRPC_ATTR_SECUREMAP) {
 			struct qcom_scm_vmperm perm;
@@ -354,10 +361,16 @@ static void fastrpc_free_map(struct kref *ref)
 				return;
 			}
 		}
+		mutex_lock(&fl->sctx->mutex);
+		if (!fl->sctx->dev) {
+			mutex_unlock(&fl->sctx->mutex);
+			return;
+		}
 		dma_buf_unmap_attachment_unlocked(map->attach, map->table,
 						  DMA_BIDIRECTIONAL);
 		dma_buf_detach(map->buf, map->attach);
 		dma_buf_put(map->buf);
+		mutex_unlock(&fl->sctx->mutex);
 	}
 
 	if (map->fl) {
@@ -414,9 +427,17 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
 
 static void fastrpc_buf_free(struct fastrpc_buf *buf)
 {
-	dma_free_coherent(buf->dev, buf->size, buf->virt,
-			  fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr));
-	kfree(buf);
+	struct fastrpc_user *fl = buf->fl;
+
+	if (!fl)
+		return;
+	mutex_lock(&fl->sctx->mutex);
+	if (fl->sctx->dev) {
+		dma_free_coherent(buf->dev, buf->size, buf->virt,
+				  FASTRPC_PHYS(buf->phys));
+		kfree(buf);
+	}
+	mutex_unlock(&fl->sctx->mutex);
 }
 
 static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
@@ -439,8 +460,10 @@ static int __fastrpc_buf_alloc(struct fastrpc_user *fl, struct device *dev,
 	buf->dev = dev;
 	buf->raddr = 0;
 
-	buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr,
-				       GFP_KERNEL);
+	mutex_lock(&fl->sctx->mutex);
+	if (fl->sctx->dev)
+		buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys, GFP_KERNEL);
+	mutex_unlock(&fl->sctx->mutex);
 	if (!buf->virt) {
 		mutex_destroy(&buf->lock);
 		kfree(buf);
@@ -483,6 +506,10 @@ static void fastrpc_channel_ctx_free(struct kref *ref)
 	struct fastrpc_channel_ctx *cctx;
 
 	cctx = container_of(ref, struct fastrpc_channel_ctx, refcount);
+	for (int i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
+		if (cctx->session[i].allocated)
+			mutex_destroy(&cctx->session[i].mutex);
+	}
 
 	kfree(cctx);
 }
@@ -800,19 +827,29 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
 		goto get_err;
 	}
 
+	mutex_lock(&fl->sctx->mutex);
+	if (!fl->sctx->dev) {
+		err = -ENODEV;
+		mutex_unlock(&fl->sctx->mutex);
+		goto attach_err;
+	}
+
 	map->attach = dma_buf_attach(map->buf, sess->dev);
 	if (IS_ERR(map->attach)) {
 		dev_err(sess->dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(map->attach);
+		mutex_unlock(&fl->sctx->mutex);
 		goto attach_err;
 	}
 
 	table = dma_buf_map_attachment_unlocked(map->attach, DMA_BIDIRECTIONAL);
 	if (IS_ERR(table)) {
 		err = PTR_ERR(table);
+		mutex_unlock(&fl->sctx->mutex);
 		goto map_err;
 	}
 	map->table = table;
+	mutex_unlock(&fl->sctx->mutex);
 
 	if (attr & FASTRPC_ATTR_SECUREMAP)
 		map->dma_addr = sg_phys(map->table->sgl);
@@ -2217,6 +2254,8 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 	sess->used = false;
 	sess->valid = true;
 	sess->dev = dev;
+	mutex_init(&sess->mutex);
+	sess->allocated = true;
 	dev_set_drvdata(dev, sess);
 
 	if (cctx->domain_id == CDSP_DOMAIN_ID)
@@ -2233,6 +2272,8 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 				break;
 			dup_sess = &cctx->session[cctx->sesscount++];
 			memcpy(dup_sess, sess, sizeof(*dup_sess));
+			mutex_init(&dup_sess->mutex);
+			dup_sess->allocated = true;
 		}
 	}
 	spin_unlock_irqrestore(&cctx->lock, flags);
@@ -2255,6 +2296,11 @@ static void fastrpc_cb_remove(struct platform_device *pdev)
 	spin_lock_irqsave(&cctx->lock, flags);
 	for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
 		if (cctx->session[i].sid == sess->sid) {
+			spin_unlock_irqrestore(&cctx->lock, flags);
+			mutex_lock(&cctx->session[i].mutex);
+			cctx->session[i].dev = NULL;
+			mutex_unlock(&cctx->session[i].mutex);
+			spin_lock_irqsave(&cctx->lock, flags);
 			cctx->session[i].valid = false;
 			cctx->sesscount--;
 		}
-- 
2.43.0

Claude review: misc: fastrpc: fix UAF and kernel panic during cleanup on process abort

[Claude Code Review Bot]

Overall Series Review

Subject: misc: fastrpc: fix UAF and kernel panic during cleanup on process abort
Author: Jianping Li <jianping.li@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-04-28T14:37:41.890684

---

This is a single patch attempting to fix use-after-free and kernel panics during FastRPC cleanup when a userspace process is abruptly terminated. The approach of serializing buffer/map cleanup against device teardown using a mutex on `fastrpc_session_ctx` is directionally reasonable, but the implementation has **critical bugs** that would prevent compilation and introduce resource leaks. The patch should not be applied in its current form.

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: fix UAF and kernel panic during cleanup on process abort

[Claude Code Review Bot]

Patch Review

**Critical: Won't compile — `buf->phys` and `FASTRPC_PHYS` don't exist**

The `fastrpc_buf` struct (line 192) has a field `dma_addr` of type `dma_addr_t`, but the patch introduces references to a nonexistent `buf->phys` field and an undefined `FASTRPC_PHYS` macro:

In `__fastrpc_buf_alloc`:
```c
buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys, GFP_KERNEL);
```

In `fastrpc_buf_free`:
```c
dma_free_coherent(buf->dev, buf->size, buf->virt,
                  FASTRPC_PHYS(buf->phys));
```

Neither `buf->phys` nor `FASTRPC_PHYS` are defined anywhere. The original code correctly uses `buf->dma_addr` and calls `fastrpc_ipa_to_dma_addr()` to extract the SMMU physical address. This is a clear build failure. It appears the patch was developed against an out-of-tree or vendor kernel with a different `fastrpc_buf` definition.

**Bug: Memory leak in `fastrpc_buf_free` when device is gone**

```c
static void fastrpc_buf_free(struct fastrpc_buf *buf)
{
    ...
    mutex_lock(&fl->sctx->mutex);
    if (fl->sctx->dev) {
        dma_free_coherent(buf->dev, buf->size, buf->virt,
                          FASTRPC_PHYS(buf->phys));
        kfree(buf);
    }
    mutex_unlock(&fl->sctx->mutex);
}
```

When `fl->sctx->dev` is NULL, `kfree(buf)` is never called. The `buf` allocation is leaked. The DMA memory may be reasonably left alone (the device is gone), but the `struct fastrpc_buf` itself must still be freed. `kfree(buf)` should be moved outside the `if` block.

**Bug: Resource leak in `fastrpc_free_map` when device is gone**

```c
mutex_lock(&fl->sctx->mutex);
if (!fl->sctx->dev) {
    mutex_unlock(&fl->sctx->mutex);
    return;
}
dma_buf_unmap_attachment_unlocked(map->attach, map->table, DMA_BIDIRECTIONAL);
dma_buf_detach(map->buf, map->attach);
dma_buf_put(map->buf);
mutex_unlock(&fl->sctx->mutex);
```

When `fl->sctx->dev` is NULL, the function returns early without: (1) putting the dma_buf reference (`dma_buf_put`), (2) removing the map from the user's list, or (3) freeing the map struct via `kfree(map)`. These happen later in the function at lines 376–383, but the early return skips them. This will leak both the `fastrpc_map` and hold dma_buf references indefinitely. The early return should jump past the DMA operations but still execute the list removal and kfree.

**Issue: `fastrpc_cb_remove` drops and re-acquires spinlock in a loop**

```c
spin_lock_irqsave(&cctx->lock, flags);
for (i = 0; i < FASTRPC_MAX_SESSIONS; i++) {
    if (cctx->session[i].sid == sess->sid) {
        spin_unlock_irqrestore(&cctx->lock, flags);
        mutex_lock(&cctx->session[i].mutex);
        cctx->session[i].dev = NULL;
        mutex_unlock(&cctx->session[i].mutex);
        spin_lock_irqsave(&cctx->lock, flags);
        cctx->session[i].valid = false;
        cctx->sesscount--;
    }
}
spin_unlock_irqrestore(&cctx->lock, flags);
```

Dropping the spinlock mid-loop means another thread could modify the session array while the loop is iterating. Since the loop checks `sid` to identify matching sessions, and other code could change session state while the lock is dropped, this introduces a TOCTOU window. Additionally, `sesscount` is modified without consistent lock protection (decremented under the spinlock but after the mutex-protected `dev = NULL` already occurred without it). This pattern is fragile. A cleaner approach would be to collect matching session indices first, then release the spinlock and take each mutex individually.

**Issue: `allocated` flag is redundant**

The new `allocated` boolean tracks whether `mutex_init` was called so that `mutex_destroy` can be called conditionally in `fastrpc_channel_ctx_free`. However, `mutex_destroy` on an uninitialized (zero-filled) mutex is a no-op in production kernels (it's only meaningful with `CONFIG_DEBUG_MUTEXES`). Since `fastrpc_channel_ctx` is allocated with `kzalloc`, the sessions array is zeroed. The `allocated` flag adds complexity without clear benefit. If it's kept, it should at least be set under the spinlock consistently with other session initialization.

**Issue: `memcpy` of session copies the mutex**

```c
dup_sess = &cctx->session[cctx->sesscount++];
memcpy(dup_sess, sess, sizeof(*dup_sess));
mutex_init(&dup_sess->mutex);
dup_sess->allocated = true;
```

The `memcpy` copies the entire `fastrpc_session_ctx` including the now-embedded mutex, then immediately re-initializes it. While the `mutex_init` overwrites the copied mutex data, copying a live mutex is technically undefined behavior and may trigger lockdep warnings with `CONFIG_DEBUG_MUTEXES`. The initialization order should be explicit: copy the relevant fields individually, or at minimum document that the mutex is intentionally re-initialized after the copy.

**Minor: `(dma_addr_t *)&buf->phys` cast**

Even ignoring the wrong field name, casting an address to `(dma_addr_t *)` is a type-punning concern if the underlying field isn't actually `dma_addr_t`. The original code correctly passes `&buf->dma_addr` without a cast.

**Summary**: The patch addresses a real problem (race between cleanup paths and device teardown), but introduces compilation failures (`buf->phys`, `FASTRPC_PHYS`), memory/resource leaks (buf and map not freed when device is gone), and questionable locking patterns. It needs significant rework before it's suitable for merging.

---
Generated by Claude Code Patch Reviewer