[PATCH v1 0/3] misc: fastrpc: fix UAF and Oops around SSR teardown

[PATCH v1 0/3] misc: fastrpc: fix UAF and Oops around SSR teardown

[Jianping Li]

On Hamoa (X1E80100) IoT EVK the ADSP can restart repeatedly, and every
restart has a chance of taking the kernel down with it:

  Unable to handle kernel paging request at virtual address fffffdffc1ffffc0
  Internal error: Oops: 0000000096000006
  CPU: 5 UID: 0 PID: 2766 Comm: adsprpcd
  pc : ___free_pages+0x24/0xe0
  Call trace:
    ___free_pages
    __free_pages
    __dma_direct_free_pages
    dma_direct_free
    dma_free_attrs
    fastrpc_context_free [fastrpc]
    fastrpc_internal_invoke [fastrpc]
    fastrpc_device_ioctl [fastrpc]

The faulting address decodes to a struct page for a PFN that has no
vmemmap backing. It comes from dma_free_coherent() being called against
a qcom,fastrpc-compute-cb device that of_platform_depopulate() has
already unbound: with the IOMMU torn down, the call falls through to
dma_direct_free(), which takes the buffer's IOVA for a physical address
and hands the resulting page to the page allocator.

fastrpc_rpmsg_remove() wakes every pending invoke with -EPIPE and then
immediately depopulates the context banks, with no synchronisation in
between, so woken threads race the teardown on their way to
fastrpc_context_free().
The series is ordered so each patch stands on its own:

  1/3 is an independent probe-time bug found while debugging this: the
      misc device is exposed before the channel refcount is initialised,
      so an open() racing probe hits "refcount_t: addition on 0".

  2/3 makes fastrpc_notify_users() wake poll-mode waiters, which today
      keep spinning on a buffer the teardown is about to reclaim.

  3/3 counts in-flight invokes and drains them before touching any
      channel resource.


Jianping Li (3):
  misc: fastrpc: initialise channel refcount before exposing the misc
    device
  misc: fastrpc: wake poll-mode waiters on SSR
  misc: fastrpc: drain in-flight invokes before tearing down context
    banks

 drivers/misc/fastrpc.c | 63 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] misc: fastrpc: initialise channel refcount before exposing the misc device

[Jianping Li]

fastrpc_device_register() calls misc_register(), which immediately makes
/dev/fastrpc-<domain> visible to userspace. However, kref_init() on the
channel context refcount only runs after the whole domain switch()
completes, several statements later.

Any process that opens the device in that window reaches
fastrpc_device_open() -> fastrpc_channel_ctx_get() -> kref_get() on a
refcount that has never been initialised and is still zero, which
refcount_t correctly reports as a use-after-free:

  refcount_t: addition on 0; use-after-free.
  WARNING: CPU: 0 PID: 760 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x144
  CPU: 0 UID: 0 PID: 760 Comm: adsprpcd
  Call trace:
    refcount_warn_saturate
    fastrpc_device_open [fastrpc]
    misc_open
    chrdev_open
    do_dentry_open
    vfs_open
    path_openat
    do_filp_open
    do_sys_openat2
    __arm64_sys_openat

This is easy to hit after a subsystem restart, when the DSP daemon
reopens the device as soon as it observes the PD coming back up, racing
with fastrpc_rpmsg_probe() on the rebind path.

Move kref_init() ahead of the device registration so the refcount is
always valid by the time the node is reachable from userspace.

Fixes: f6f9279f2bf0 ("misc: fastrpc: Add Qualcomm fastrpc basic driver model")
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index af18ff1992ee..05b2e7e4ad3b 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2599,6 +2599,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	data->poll_mode_supported = soc_data->poll_mode_supported ||
 		of_machine_get_match(fastrpc_poll_supported_machines);
 
+	kref_init(&data->refcount);
 	switch (domain_id) {
 	case ADSP_DOMAIN_ID:
 	case MDSP_DOMAIN_ID:
@@ -2626,7 +2627,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		goto err_free_data;
 	}
 
-	kref_init(&data->refcount);
 	atomic_set(&data->ctx_seq, 0);
 
 	rdev->dma_mask = &data->dma_mask;
-- 
2.43.0

Re: [PATCH v1 1/3] misc: fastrpc: initialise channel refcount before exposing the misc device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] misc: fastrpc: uninitialized spin_lock access due to premature registration
- [High] misc: fastrpc: UAF on probe error path due to direct kfree
- [High] misc: fastrpc: UAF in fastrpc_rpmsg_probe error path due to raw kfree
--

commit 7fb377963b47742ec8445ae7a99aea90b1ae5375
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: initialise channel refcount before exposing the misc device

This patch moves kref_init() before fastrpc_device_register() to fix a
refcount warning when userspace opens the misc device concurrently during
probe.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index af18ff1992eed..05b2e7e4ad3b7 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2599,6 +2599,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	data->poll_mode_supported = soc_data->poll_mode_supported ||
>  		of_machine_get_match(fastrpc_poll_supported_machines);
>  
> +	kref_init(&data->refcount);
>  	switch (domain_id) {
>  	case ADSP_DOMAIN_ID:
>  	case MDSP_DOMAIN_ID:

[ ... ]

> @@ -2626,7 +2627,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		goto err_free_data;
>  	}
>  
> -	kref_init(&data->refcount);
>  	atomic_set(&data->ctx_seq, 0);
>  
>  	rdev->dma_mask = &data->dma_mask;

[Severity: High]
Does exposing the device to userspace before initializing the spinlock result
in a fatal crash if the device is opened immediately?

In fastrpc_rpmsg_probe(), spin_lock_init(&data->lock) is called further down,
after the switch statement. If userspace opens the device immediately after
fastrpc_device_register(), the open path calls fastrpc_session_alloc(), which
attempts to acquire the uninitialized lock:

fastrpc_device_open()
   fastrpc_session_alloc()
      spin_lock_irqsave(&cctx->lock, flags);

[Severity: High]
Does the probe error path cause a use-after-free if userspace has successfully
opened the device?

Since kref_init() now executes before the device registration, an early open
can successfully acquire a reference via fastrpc_channel_ctx_get().

If a subsequent probe step fails (such as the second fastrpc_device_register()
call for CDSP/GDSP, or of_platform_populate()), the probe error path directly
frees the channel context:

err_free_data:
	kfree(data);
	return err;

This unconditional kfree() bypasses the kref lifecycle. When userspace later
closes the file descriptor, fastrpc_device_release() calls
fastrpc_channel_ctx_put(), which will dereference the freed memory.

Should the error path use fastrpc_channel_ctx_put() once the kref has been
initialized, instead of calling kfree() directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007084447.922-1-jianping.li@oss.qualcomm.com?part=1

[PATCH v1 2/3] misc: fastrpc: wake poll-mode waiters on SSR

[Jianping Li]

fastrpc_notify_users() sets ctx->retval to -EPIPE and completes
ctx->work for every pending and interrupted context, which is enough to
release a thread blocked in fastrpc_wait_for_response().

A thread using polling mode does not wait on that completion. It spins
in poll_for_remote_response(), whose exit condition is:

  (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done

Since the DSP is already down, it will never write FASTRPC_POLL_RESPONSE
into the poll address, and is_work_done is left untouched by
fastrpc_notify_users(). The thread therefore keeps spinning until
FASTRPC_POLL_MAX_TIMEOUT_US expires instead of bailing out immediately.

Worse, the poll address lives inside ctx->buf, so a polling thread that
has not been told to stop is still reading a buffer that the SSR
teardown path is about to reclaim.

Set is_work_done along with retval so polling waiters observe the
termination on their next iteration, exactly like completion waiters do.

Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 05b2e7e4ad3b..c54c450cb571 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2663,6 +2663,7 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
 	spin_lock(&user->lock);
 	list_for_each_entry(ctx, &user->pending, node) {
 		ctx->retval = -EPIPE;
+		ctx->is_work_done = true;
 		complete(&ctx->work);
 	}
 	spin_unlock(&user->lock);
-- 
2.43.0

Re: [PATCH v1 2/3] misc: fastrpc: wake poll-mode waiters on SSR

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] misc/fastrpc: missing memory barrier and data race on ctx->is_work_done allows stale error codes
--

commit ec0bf5065c708ee8356d5c1feae24849edc520bd
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: wake poll-mode waiters on SSR

This commit updates fastrpc_notify_users() to set ctx->is_work_done along
with ctx->retval. This ensures that threads spinning in polling mode will
observe the termination and exit, instead of waiting for a timeout when the
DSP goes down.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 05b2e7e4ad3b7..c54c450cb5714 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2663,6 +2663,7 @@ static void fastrpc_notify_users(struct fastrpc_user *user)
>  	spin_lock(&user->lock);
>  	list_for_each_entry(ctx, &user->pending, node) {
>  		ctx->retval = -EPIPE;
> +		ctx->is_work_done = true;

[Severity: Medium]
Is there a missing write memory barrier here?

If the stores to ctx->retval and ctx->is_work_done are reordered by the
compiler or the CPU, a concurrent thread executing poll_for_remote_response()
might read ctx->is_work_done as true but read a stale value for ctx->retval,
such as the initialization value -1 or -EPERM.

In poll_for_remote_response(), a concurrent thread evaluates this locklessly:

        ...
        (val == FASTRPC_POLL_RESPONSE) || ctx->is_work_done

Even if the polling thread uses dma_rmb() before reading ctx->retval, that
only ensures read-read hardware ordering. It does not prevent this writer
thread from reordering the stores. Would it be safer to use
smp_store_release() for ctx->is_work_done here and smp_load_acquire() in the
polling loop?

Additionally, does this introduce a plain C data race?

poll_for_remote_response() appears to locklessly write to ctx->is_work_done
on DSP polling completion. This could cause undefined behavior when racing
with the lockless reads and this new write in fastrpc_notify_users().

>  		complete(&ctx->work);
>  	}
>  	spin_unlock(&user->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007084447.922-1-jianping.li@oss.qualcomm.com?part=2

[PATCH v1 3/3] misc: fastrpc: drain in-flight invokes before tearing down context banks

[Jianping Li]

fastrpc_rpmsg_remove() clears cctx->rpdev, wakes every pending invoke
with -EPIPE through fastrpc_notify_users(), and then goes straight on to
of_platform_depopulate() the context bank devices, with nothing
synchronising the two:

 - The only thing stopping new work is the unlocked rpdev check at the
   top of fastrpc_internal_invoke(). A thread can observe a valid rpdev,
   get preempted, and resume in the middle of the teardown.

 - Invokes that were already admitted are woken and head for the bail:
   label, where fastrpc_context_put() releases ctx->buf through
   dma_free_coherent(). If the depopulate wins, the call runs against an
   unbound context bank and falls through to dma_direct_free(), which
   treats the IOVA as a physical address.

Close both windows under cctx->lock:

 - Add cctx->teardown, set at the start of fastrpc_rpmsg_remove().
   fastrpc_internal_invoke() checks it and, in the same critical
   section, increments cctx->invoke_cnt, so an invoke is either rejected
   or counted. The count is dropped on every return path.

 - fastrpc_rpmsg_remove() waits for invoke_cnt to reach zero before
   clearing rpdev and before touching any other channel resource.

The gate lives in fastrpc_internal_invoke() rather than in the ioctl
dispatcher so that it also covers fastrpc_device_release() ->
fastrpc_release_current_dsp_process(), which sends INIT_RELEASE outside
of any ioctl.

Teardown is kept separate from rpdev because the two have different
lifetimes: the gate must close immediately, whereas rpdev has to stay
valid until the admitted invokes have finished rpmsg_send(). Clearing
rpdev up front, as the code does today, lets an invoke that was sleeping
in an allocation dereference a NULL rpdev:

  Unable to handle kernel NULL pointer dereference at virtual address
0000000000000358
  CPU: 2 UID: 0 PID: 2003 Comm: adsprpcd
  pc : fastrpc_internal_invoke+0xb40/0x1398 [fastrpc]
  Call trace:
    fastrpc_internal_invoke [fastrpc]
    fastrpc_device_release [fastrpc]
    __fput
    __arm64_sys_close

The faulting address is the offset of ept within struct rpmsg_device,
i.e. the cctx->rpdev->ept dereference in rpmsg_send().

Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 60 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index c54c450cb571..3036925632d0 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -328,6 +328,15 @@ struct fastrpc_channel_ctx {
 	atomic_t ctx_seq;
 	u64 dma_mask;
 	const struct fastrpc_soc_data *soc_data;
+	/*
+	 * Set once fastrpc_rpmsg_remove() starts tearing the channel down.
+	 * Checked under @lock so that no new invoke can begin afterwards.
+	 */
+	atomic_t teardown;
+	/* Number of in-flight invokes; guarded by @lock */
+	int invoke_cnt;
+	/* Woken whenever @invoke_cnt drops to zero */
+	wait_queue_head_t ssr_wait_queue;
 };
 
 struct fastrpc_device {
@@ -1350,12 +1359,23 @@ static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx,
 	return fastrpc_wait_for_response(ctx, kernel);
 }
 
+/* Caller must hold cctx->lock */
+static void fastrpc_channel_update_invoke_cnt(struct fastrpc_channel_ctx *cctx,
+					      bool enter)
+{
+	if (enter)
+		cctx->invoke_cnt++;
+	else if (--cctx->invoke_cnt == 0)
+		wake_up(&cctx->ssr_wait_queue);
+}
+
 static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 				   u32 handle, u32 sc,
 				   struct fastrpc_invoke_args *args)
 {
 	struct fastrpc_invoke_ctx *ctx = NULL;
 	struct fastrpc_buf *buf, *b;
+	unsigned long flags;
 
 	int err = 0;
 
@@ -1365,14 +1385,25 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 	if (!fl->cctx->rpdev)
 		return -EPIPE;
 
+	spin_lock_irqsave(&fl->cctx->lock, flags);
+	if (atomic_read(&fl->cctx->teardown)) {
+		spin_unlock_irqrestore(&fl->cctx->lock, flags);
+		return -EPIPE;
+	}
+	fastrpc_channel_update_invoke_cnt(fl->cctx, true);
+	spin_unlock_irqrestore(&fl->cctx->lock, flags);
+
 	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
 		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
-		return -EPERM;
+		err = -EPERM;
+		goto out;
 	}
 
 	ctx = fastrpc_context_alloc(fl, kernel, sc, args);
-	if (IS_ERR(ctx))
-		return PTR_ERR(ctx);
+	if (IS_ERR(ctx)) {
+		err = PTR_ERR(ctx);
+		goto out;
+	}
 
 	err = fastrpc_get_args(kernel, ctx);
 	if (err)
@@ -1428,6 +1459,10 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
 
 	if (err)
 		dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
+out:
+	spin_lock_irqsave(&fl->cctx->lock, flags);
+	fastrpc_channel_update_invoke_cnt(fl->cctx, false);
+	spin_unlock_irqrestore(&fl->cctx->lock, flags);
 
 	return err;
 }
@@ -2635,6 +2670,9 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	INIT_LIST_HEAD(&data->invoke_interrupted_mmaps);
 	spin_lock_init(&data->lock);
 	idr_init(&data->ctx_idr);
+	atomic_set(&data->teardown, 0);
+	data->invoke_cnt = 0;
+	init_waitqueue_head(&data->ssr_wait_queue);
 	data->domain_id = domain_id;
 	data->rpdev = rpdev;
 	dev_set_drvdata(&rpdev->dev, data);
@@ -2678,11 +2716,25 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 
 	/* No invocations past this point */
 	spin_lock_irqsave(&cctx->lock, flags);
-	cctx->rpdev = NULL;
+	atomic_set(&cctx->teardown, 1);
 	list_for_each_entry(user, &cctx->users, user)
 		fastrpc_notify_users(user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
+	/*
+	 * Wait for every invoke that was already past the gate to finish.
+	 * They have all just been woken with -EPIPE, and no new one can be
+	 * counted, so this is guaranteed to make progress.
+	 */
+	spin_lock_irqsave(&cctx->lock, flags);
+	while (cctx->invoke_cnt > 0) {
+		spin_unlock_irqrestore(&cctx->lock, flags);
+		wait_event(cctx->ssr_wait_queue, cctx->invoke_cnt == 0);
+		spin_lock_irqsave(&cctx->lock, flags);
+	}
+	cctx->rpdev = NULL;
+	spin_unlock_irqrestore(&cctx->lock, flags);
+
 	if (cctx->fdevice)
 		misc_deregister(&cctx->fdevice->miscdev);
 
-- 
2.43.0

Re: [PATCH v1 3/3] misc: fastrpc: drain in-flight invokes before tearing down context banks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] fastrpc: deadlock in fastrpc_rpmsg_remove() due to TOCTOU race with context allocation
--

commit cc1c8d918750aea3d9a92e77f3c2258f941ddc99
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: drain in-flight invokes before tearing down context banks

This commit introduces a synchronization mechanism to drain in-flight invokes
before tearing down context banks. It adds a teardown flag and an invoke counter
to wait until all active invokes complete during rpmsg removal. It also signals
pending invokes with -EPIPE to wake them up.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index c54c450cb5714..3036925632d04 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -1365,14 +1385,25 @@ static int fastrpc_internal_invoke(struct fastrpc_user *fl,  u32 kernel,
>  	if (!fl->cctx->rpdev)
>  		return -EPIPE;
>  
> +	spin_lock_irqsave(&fl->cctx->lock, flags);
> +	if (atomic_read(&fl->cctx->teardown)) {
> +		spin_unlock_irqrestore(&fl->cctx->lock, flags);
> +		return -EPIPE;
> +	}
> +	fastrpc_channel_update_invoke_cnt(fl->cctx, true);
> +	spin_unlock_irqrestore(&fl->cctx->lock, flags);

[Severity: High]
Does this create a critical race window? If the thread is preempted right after
unlocking here, it has incremented invoke_cnt but hasn't yet called
fastrpc_context_alloc() to add its context to the user->pending list.

>  
>  	if (handle == FASTRPC_INIT_HANDLE && !kernel) {
>  		dev_warn_ratelimited(fl->sctx->dev, "user app trying to send a kernel RPC message (%d)\n",  handle);
> -		return -EPERM;
> +		err = -EPERM;
> +		goto out;
>  	}
>  
>  	ctx = fastrpc_context_alloc(fl, kernel, sc, args);

[Severity: High]
Could fastrpc_rpmsg_remove() run concurrently while the first thread is
preempted? If so, when fastrpc_context_alloc() is finally called, it seems it
is too late to receive the teardown notification since fastrpc_rpmsg_remove()
has already swept the pending list.

[ ... ]
> @@ -2678,11 +2716,25 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>  
>  	/* No invocations past this point */
>  	spin_lock_irqsave(&cctx->lock, flags);
> -	cctx->rpdev = NULL;
> +	atomic_set(&cctx->teardown, 1);
>  	list_for_each_entry(user, &cctx->users, user)
>  		fastrpc_notify_users(user);
>  	spin_unlock_irqrestore(&cctx->lock, flags);

[Severity: High]
Does this notify loop miss the context that is still suspended between the
counter increment and allocation? fastrpc_notify_users() only iterates over
the user->pending list, which the preempted thread has not yet reached.

>  
> +	/*
> +	 * Wait for every invoke that was already past the gate to finish.
> +	 * They have all just been woken with -EPIPE, and no new one can be
> +	 * counted, so this is guaranteed to make progress.
> +	 */
> +	spin_lock_irqsave(&cctx->lock, flags);
> +	while (cctx->invoke_cnt > 0) {
> +		spin_unlock_irqrestore(&cctx->lock, flags);
> +		wait_event(cctx->ssr_wait_queue, cctx->invoke_cnt == 0);

[Severity: High]
Will this wait forever? The preempted context missed the explicit abort sweep,
so it will block indefinitely in fastrpc_wait_for_completion(). Because the DSP
is tearing down, it will never reply, meaning invoke_cnt never drops to 0, which
would permanently deadlock the removal thread.

> +		spin_lock_irqsave(&cctx->lock, flags);
> +	}
> +	cctx->rpdev = NULL;
> +	spin_unlock_irqrestore(&cctx->lock, flags);
> +
>  	if (cctx->fdevice)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007084447.922-1-jianping.li@oss.qualcomm.com?part=3