[PATCH v1] misc: fastrpc: Add reference counting for fastrpc_user structure

[PATCH v1] misc: fastrpc: Add reference counting for fastrpc_user structure

[Anandu Krishnan E]

Add reference counting using kref to the fastrpc_user structure to
prevent use-after-free issues when contexts are freed from workqueue
after device release.

The issue occurs when fastrpc_device_release() frees the user structure
while invoke contexts are still pending in the workqueue. When the
workqueue later calls fastrpc_context_free(), it attempts to access
buf->fl->cctx in fastrpc_buf_free(), leading to a use-after-free:

  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

Implement proper reference counting to fix this:
- Initialize kref in fastrpc_device_open()
- Take a reference in fastrpc_context_alloc() for each context
- Release the reference in fastrpc_context_free() when context is freed
- Release the initial reference in fastrpc_device_release()

This ensures the user structure remains valid as long as there are
contexts holding references to it, preventing the race condition.

Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
Cc: stable@kernel.org
Signed-off-by: Anandu Krishnan E <anandu.e@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 47356a5d5804..3ababcf327d7 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,36 @@ static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
 	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
 }
 
+static void fastrpc_user_free(struct kref *ref)
+{
+	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);
+
+	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]);
@@ -521,6 +544,8 @@ static void fastrpc_context_free(struct kref *ref)
 	kfree(ctx->olaps);
 	kfree(ctx);
 
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 	fastrpc_channel_ctx_put(cctx);
 }
 
@@ -628,6 +653,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;
@@ -658,6 +685,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	spin_lock(&user->lock);
 	list_del(&ctx->node);
 	spin_unlock(&user->lock);
+	fastrpc_user_put(user);
 	fastrpc_channel_ctx_put(cctx);
 	kfree(ctx->maps);
 	kfree(ctx->olaps);
@@ -1606,11 +1634,9 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	}
 
 	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;
 }
@@ -1654,6 +1680,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

Re: [PATCH v1] misc: fastrpc: Add reference counting for fastrpc_user structure

[Bjorn Andersson]

On Thu, Feb 26, 2026 at 08:41:21PM +0530, Anandu Krishnan E wrote:
> Add reference counting using kref to the fastrpc_user structure to
> prevent use-after-free issues when contexts are freed from workqueue
> after device release.

Please follow
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
and start your commit message by clearly establishing the problem, once
that's done you can describe the technical solution.

> 
> The issue occurs when fastrpc_device_release() frees the user structure
> while invoke contexts are still pending in the workqueue. When the
> workqueue later calls fastrpc_context_free(), it attempts to access
> buf->fl->cctx in fastrpc_buf_free(), leading to a use-after-free:

But why does it do that?

The reason why we need buf->fl->cctx in this context is because we need
to mask out the DMA address from the buf->dma_addr (remove the SID bits).

If we instead split "dma_addr" into two separate members of struct
fastrpc_buf, one dma_addr_t dma_addr and one u64 iova_with_sid (?), then
it would be clear throughout the driver which address space we're
talking about (is it the SID-adjusted iova space or the dma_addr_t in
the particular DMA context).

In addition to making this aspect of the driver easier to follow, you no
longer need to call fastrpc_ipa_to_dma_addr() in fastrpc_buf_free() (or
anywhere else for that matter).

You can just pass buf->dma_addr directly to dma_free_coherent().

You're isolating the fact that the firmware needs to see "SID |
dma_addr" to just two places in the driver.

> 
>   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
> 
> Implement proper reference counting to fix this:
> - Initialize kref in fastrpc_device_open()
> - Take a reference in fastrpc_context_alloc() for each context
> - Release the reference in fastrpc_context_free() when context is freed
> - Release the initial reference in fastrpc_device_release()

There's no reason to include a checklist of pseudo-code in the commit
message, describe why and the overall design if this isn't obvious.

> 
> This ensures the user structure remains valid as long as there are
> contexts holding references to it, preventing the race condition.
> 

The life cycles at play in this driver is already very hard to reason
about.

> Fixes: 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
> Cc: stable@kernel.org
> Signed-off-by: Anandu Krishnan E <anandu.e@oss.qualcomm.com>
> ---
>  drivers/misc/fastrpc.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 47356a5d5804..3ababcf327d7 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,36 @@ static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
>  	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
>  }
>  
> +static void fastrpc_user_free(struct kref *ref)
> +{
> +	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);

Unrelated question, what does the "fl" abbreviation actually mean? I
presume 'f' is for "fastrpc", but what is 'l'?

Regards,
Bjorn

> +
> +	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]);
> @@ -521,6 +544,8 @@ static void fastrpc_context_free(struct kref *ref)
>  	kfree(ctx->olaps);
>  	kfree(ctx);
>  
> +	/* Release the reference taken in fastrpc_context_alloc() */
> +	fastrpc_user_put(fl);
>  	fastrpc_channel_ctx_put(cctx);
>  }
>  
> @@ -628,6 +653,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;
> @@ -658,6 +685,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  	spin_lock(&user->lock);
>  	list_del(&ctx->node);
>  	spin_unlock(&user->lock);
> +	fastrpc_user_put(user);
>  	fastrpc_channel_ctx_put(cctx);
>  	kfree(ctx->maps);
>  	kfree(ctx->olaps);
> @@ -1606,11 +1634,9 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
>  	}
>  
>  	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;
>  }
> @@ -1654,6 +1680,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: Add reference counting for fastrpc_user structure

[Claude Code Review Bot]

Overall Series Review

Subject: misc: fastrpc: Add reference counting for fastrpc_user structure
Author: Anandu Krishnan E <anandu.e@oss.qualcomm.com>
Patches: 2
Reviewed: 2026-02-27T11:57:24.514716

---

This single patch adds `kref`-based reference counting to `struct fastrpc_user` to prevent a use-after-free where `fastrpc_device_release()` frees the user structure while invoke contexts are still pending in a workqueue. The approach is fundamentally correct — each context takes a reference on the user in `fastrpc_context_alloc()` and releases it in `fastrpc_context_free()`, ensuring the user struct outlives all its contexts.

However, the patch has a **significant bug** in the placement of `kref_init()` in `fastrpc_device_open()`, which introduces a new race window. The reference counting logic is otherwise sound — the `fastrpc_channel_ctx` get/put accounting remains correctly balanced despite the structural changes.

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: Add reference counting for fastrpc_user structure

[Claude Code Review Bot]

Patch Review

**kref_init placement is after the user becomes visible (BUG)**

In `fastrpc_device_open()`, the patch places `kref_init()` **after** `list_add_tail()`:

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

At this point `fl` is already visible on `cctx->users`. If `fastrpc_rpmsg_remove()` runs on another CPU between `list_add_tail()` and `kref_init()`, it will iterate `cctx->users` and could operate on this user with a zero refcount (from `kzalloc` zero-init). The `kref_init()` should be placed alongside the other initialization calls (near `spin_lock_init`, `mutex_init`, etc.) **before** the user is linked into any list:

```c
	fl->is_secure_dev = fdevice->secure;
+	kref_init(&fl->refcount);

	fl->sctx = fastrpc_session_alloc(fl);
	...
	list_add_tail(&fl->user, &cctx->users);
```

**fastrpc_user_free only does partial cleanup — by design, but deserves a comment**

```c
+static void fastrpc_user_free(struct kref *ref)
+{
+	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);
+
+	fastrpc_channel_ctx_put(fl->cctx);
+	mutex_destroy(&fl->mutex);
+	kfree(fl);
+}
```

This only frees the struct, the mutex, and the cctx reference. All heavyweight cleanup (session free, DSP process release, pending context teardown, map/mmap cleanup) remains in `fastrpc_device_release()`. This is correct — the refcount only extends the *lifetime of the struct* past device release, not the logical ownership. But a brief comment in `fastrpc_user_free()` noting this would help future readers understand that this is intentional and that full teardown is done in `fastrpc_device_release()`.

**Channel context reference accounting is correct**

I verified there is no double-put of `cctx`. The references are:

| Get | Put |
|-----|-----|
| `fastrpc_device_open()` → `fastrpc_channel_ctx_get(cctx)` | `fastrpc_user_free()` → `fastrpc_channel_ctx_put(fl->cctx)` |
| `fastrpc_context_alloc()` → `fastrpc_channel_ctx_get(cctx)` | `fastrpc_context_free()` → `fastrpc_channel_ctx_put(cctx)` (existing, line 524) |

In `fastrpc_context_free()`, the new `fastrpc_user_put(fl)` is called before the existing `fastrpc_channel_ctx_put(cctx)`:

```c
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 	fastrpc_channel_ctx_put(cctx);
```

Even if `fastrpc_user_put()` triggers `fastrpc_user_free()` (which puts the *user's* cctx ref), the *context's* cctx ref still keeps `cctx` alive for the next line. This ordering is safe.

**Error path in fastrpc_context_alloc is correct**

```c
+	fastrpc_user_put(user);
 	fastrpc_channel_ctx_put(cctx);
```

The error path correctly balances the `fastrpc_user_get(user)` taken earlier in the function.

**fastrpc_buf_free safety during workqueue-deferred context_free**

The key UAF scenario the patch addresses: `fastrpc_buf_free()` at `fastrpc.c:417` dereferences `buf->fl->cctx`. In the patched code, when `fastrpc_context_free()` is called from the workqueue, `fastrpc_buf_free(ctx->buf)` runs *before* `fastrpc_user_put(fl)`, so the user struct is guaranteed alive. This is correct.

**Minor: redundant comment on struct member**

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

The `kref` type already implies reference counting. This comment adds no information. Consider dropping it to match the style of `struct kref refcount` used elsewhere in the same file (e.g., in `fastrpc_channel_ctx`).

**Summary of required changes:**

1. **Must fix:** Move `kref_init(&fl->refcount)` before `list_add_tail()` in `fastrpc_device_open()` to close the race window.
2. **Nice to have:** Add a comment in `fastrpc_user_free()` clarifying that full teardown is in `fastrpc_device_release()`, and this only extends struct lifetime.

---
Generated by Claude Code Patch Reviewer