Re: [PATCH] misc: fastrpc: initialize invoke refcount before publish

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch transforms a refcount underflow warning into an exploitable Use-After-Free (UAF) and list corruption by prematurely freeing the context on an early DSP response.

Pre-existing issues:
- [Critical] `fastrpc_rpmsg_callback()` is vulnerable to duplicate or late DSP responses, leading to Use-After-Free vulnerabilities.
- [High] If `fastrpc_internal_invoke()` is interrupted by a signal or times out, the `fastrpc_invoke_ctx` and its associated `fastrpc_user` are permanently leaked, leading to local DoS.
- [High] When the DSP crashes or is removed, pending `fastrpc_invoke_ctx` objects are permanently leaked.
--

commit 285340b375fb7017cf7735f77379c860ac74a67c
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

misc: fastrpc: initialize invoke refcount before publish

fastrpc_context_alloc() adds a new invoke context to the pending list
and the channel IDR before initializing its refcount. Initialize the
refcount before publishing the context to prevent a racing DSP
response from dropping a reference before kref_init() has run.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..9f2929cb39d93 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -690,6 +690,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  	ctx->cctx = cctx;
>  	init_completion(&ctx->work);
>  	INIT_WORK(&ctx->put_work, fastrpc_context_put_wq);
> +	kref_init(&ctx->refcount);
>  
>  	spin_lock(&user->lock);
>  	list_add_tail(&ctx->node, &user->pending);
> @@ -705,8 +706,6 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  	ctx->ctxid = ret << 4;

[Severity: Critical]
By moving kref_init() before idr_alloc_cyclic(), does this introduce a
Use-After-Free?

If a delayed or duplicate DSP response arrives immediately after
idr_alloc_cyclic() returns, fastrpc_rpmsg_callback() can find the context
in the IDR and schedule put_work. This would drop the refcount to 0,
prematurely freeing the context while the allocating thread continues execution:

drivers/misc/fastrpc.c:fastrpc_context_alloc() {
    ...
    kref_init(&ctx->refcount);
    ...
    ret = idr_alloc_cyclic(&cctx->ctx_idr, ctx, ...);
    
    // DSP response arrives here, drops the only reference, freeing ctx
    
    ctx->ctxid = ret << 4; // Use-After-Free
}

Prior to this change, the uninitialized refcount prevented the free.

>  	spin_unlock_irqrestore(&cctx->lock, flags);
>  
> -	kref_init(&ctx->refcount);
> -
>  	return ctx;

[Severity: Critical]
This is a pre-existing issue, but is fastrpc_rpmsg_callback() vulnerable to
duplicate or late DSP responses?

When a response arrives, it looks up the context and blindly schedules put_work
without checking if a response is expected or removing the context from the IDR:

drivers/misc/fastrpc.c:fastrpc_rpmsg_callback() {
    ...
    ctx = idr_find(&cctx->ctx_idr, ctxid);
    ...
    schedule_work(&ctx->put_work);
}

If a faulty DSP sends multiple responses for the same ID, could this schedule
put_work multiple times and lead to a Use-After-Free?


[Severity: High]
This isn't a bug introduced by this patch, but does fastrpc_internal_invoke()
leak the context on signals or timeouts?

If wait_for_completion_interruptible() returns -ERESTARTSYS or -ETIMEDOUT, the
error path intentionally bypasses dropping the context reference:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
    ...
    if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
        spin_lock(&fl->lock);
        list_del(&ctx->node);
        spin_unlock(&fl->lock);
        fastrpc_context_put(ctx);
    }
}

Since fastrpc_context_alloc() takes a reference on fastrpc_user, does this
create a permanent reference cycle that prevents fastrpc_user_free() from ever
being called, potentially allowing an unprivileged user to exhaust IDR slots?


[Severity: High]
This is a pre-existing issue, but are pending contexts permanently leaked when
the DSP crashes or is removed?

fastrpc_invoke_send() takes a reference on the context on behalf of the DSP:

drivers/misc/fastrpc.c:fastrpc_invoke_send() {
    ...
    fastrpc_context_get(ctx);
    ret = rpmsg_send(...);
}

When the DSP is removed, fastrpc_rpmsg_remove() notifies users but doesn't
appear to drop this extra reference:

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    list_for_each_entry(user, &cctx->users, user)
        fastrpc_notify_users(user);
}

Since the DSP will never respond, does this leave the in-flight contexts and
their parent structures permanently leaked?

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