Re: [PATCH] misc: fastrpc: release pending invoke refs on rpmsg removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] Blind asynchronous `put_work` scheduling on 0-refcount contexts causes a Use-After-Free and Double Free.
- [Critical] Premature drop of the send reference breaks callback lifecycle assumptions, leading to a Use-After-Free.
- [High] The patch introduces a build failure by using undefined struct fields, and its logic is broken because the normal callback path does not interact with the new fields.

Pre-existing issues:
- [Critical] A compromised DSP can trigger a Use-After-Free by sending duplicate replies for the same context.
- [High] TOCTOU race condition causing a NULL pointer dereference of `cctx->rpdev` during concurrent endpoint removal.
- [High] Timed-out or interrupted invocations permanently leak contexts due to a circular reference.
--

commit 560fa1b03e675a7c5d337410e599f919e4a647ef
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

misc: fastrpc: release pending invoke refs on rpmsg removal

fastrpc_rpmsg_remove() wakes pending invoke waiters when the rpmsg device
is removed, but it does not release the send references taken before each
request was submitted. Those references normally disappear only when a DSP
reply arrives, which cannot be relied on after endpoint removal.

Walk the channel IDR during removal, mark in-flight contexts completed,
and schedule the send-reference put while waking waiters with -EPIPE. This
prevents disconnected channels from pinning invoke contexts indefinitely.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..95e727fa09cc6 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2484,30 +2484,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  	return err;
>  }
>  
> -static void fastrpc_notify_users(struct fastrpc_user *user)
> +static int fastrpc_notify_context(int id, void *ptr, void *data)
>  {
> -	struct fastrpc_invoke_ctx *ctx;
> +	struct fastrpc_invoke_ctx *ctx = ptr;
>  
> -	spin_lock(&user->lock);
> -	list_for_each_entry(ctx, &user->pending, node) {
> -		ctx->retval = -EPIPE;
> -		complete(&ctx->work);
> +	if (ctx->sent && !ctx->completed) {

[Severity: High]
Does this patch fail to compile?  Looking at struct fastrpc_invoke_ctx, the
fields sent and completed do not appear to exist.

If these fields were manually added to fix the build, does this logic still
introduce a refcount underflow?  If fastrpc_rpmsg_callback() receives a normal
reply, it does not seem to set ctx->completed = true.  Would a concurrent
execution of fastrpc_notify_context() see !ctx->completed and schedule
put_work concurrently with the callback, causing double execution of
fastrpc_context_put()?

> +		ctx->completed = true;
> +		schedule_work(&ctx->put_work);

[Severity: Critical]
Can this schedule put_work on a context that has already reached a refcount
of zero?

When a context's refcount drops to zero, fastrpc_context_free() is executed:

fastrpc_context_free() {
    ...
    spin_lock_irqsave(&cctx->lock, flags);
    idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
    spin_unlock_irqrestore(&cctx->lock, flags);
    ...
}

If fastrpc_rpmsg_remove() acquires cctx->lock first, could idr_for_each() pass
the zero-refcount context to fastrpc_notify_context() before it is removed
from the IDR?  Since schedule_work() is called without using
kref_get_unless_zero(), wouldn't put_work execute fastrpc_context_put() on a
dying object, underflowing the refcount and executing fastrpc_context_free()
again?

[Severity: Critical]
Does prematurely dropping the send reference break the lifecycle assumptions
of the callback?

Looking at fastrpc_rpmsg_callback():

fastrpc_rpmsg_callback() {
    ...
    spin_unlock_irqrestore(&cctx->lock, flags);

    if (!ctx) {
        ...
    }
    ctx->retval = rsp->retval;
    complete(&ctx->work);
    ...
}

The callback drops cctx->lock and relies on the send reference to keep the
context alive while it writes to ctx->retval.

If fastrpc_rpmsg_remove() runs concurrently and schedules put_work, could the
send reference be dropped and the waiter awakened?  If the waiter then drops
its own reference, the context could be freed before fastrpc_rpmsg_callback()
resumes to write to ctx->retval, potentially causing a use-after-free?

[Severity: Critical]
This is a pre-existing issue, but can a compromised DSP trigger a
use-after-free by sending duplicate replies?

Looking at fastrpc_rpmsg_callback(), it looks up a context via ctxid and calls
schedule_work(&ctx->put_work) without verifying if it was already processed.

Since the context is only removed from the IDR when its refcount hits zero,
could duplicate replies from the DSP queue the work item again before the
final reference is dropped, causing an extra fastrpc_context_put() and
underflowing the refcount?

>  	}
> -	spin_unlock(&user->lock);
> +
> +	ctx->retval = -EPIPE;
> +	complete(&ctx->work);
> +
> +	return 0;
>  }
>  
>  static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
>  {
>  	struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev);
>  	struct fastrpc_buf *buf, *b;
> -	struct fastrpc_user *user;
>  	unsigned long flags;
>  
>  	/* No invocations past this point */
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	cctx->rpdev = NULL;

[Severity: High]
This is a pre-existing issue, but does setting cctx->rpdev to NULL here
introduce a TOCTOU race condition that can result in a NULL pointer
dereference?

In fastrpc_internal_invoke(), there is a lockless check for !fl->cctx->rpdev.

If fastrpc_rpmsg_remove() acquires cctx->lock and sets cctx->rpdev = NULL
concurrently, could fastrpc_invoke_send() later unconditionally dereference
cctx->rpdev->ept without acquiring cctx->lock or re-validating the pointer?

fastrpc_invoke_send() {
    ...
    ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg));
    ...
}

> -	list_for_each_entry(user, &cctx->users, user)
> -		fastrpc_notify_users(user);
> +	idr_for_each(&cctx->ctx_idr, fastrpc_notify_context, NULL);
>  	spin_unlock_irqrestore(&cctx->lock, flags);

[Severity: High]
This is a pre-existing issue, but do timed-out or interrupted invocations
permanently leak contexts?

Looking at fastrpc_internal_invoke():

fastrpc_internal_invoke() {
    ...
    if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
        /* We are done with this compute context */
        spin_lock(&fl->lock);
        list_del(&ctx->node);
        spin_unlock(&fl->lock);
        fastrpc_context_put(ctx);
    }
    ...
}

If the invocation is interrupted or times out, it skips calling
fastrpc_context_put(), leaving the context on fl->pending with its first
reference held.

Since the context holds a reference to the fastrpc_user (fl) via
fastrpc_user_get(), would the user's refcount never reach zero, preventing
fastrpc_user_free() from ever running and permanently leaking the context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624192700.5388-1-alhouseenyousef@gmail.com?part=1