Re: [PATCH v13 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Circular reference between fastrpc_user and fastrpc_invoke_ctx leads to permanent memory leak on interrupted invokes
- [High] IDR lookup without reference count acquisition in fastrpc_rpmsg_callback leads to Use-After-Free
- [High] DMA buffers moved to invoke_interrupted_mmaps are permanently leaked
- [High] DSP's reference count on fastrpc_invoke_ctx is never dropped during rpmsg device removal
- [High] fastrpc_invoke_ctx is exposed to IDR before its refcount is initialized
--

--- Patch [3]: [PATCH v13 3/4] misc: fastrpc: Expand context ID mask for DSP polling mode support ---
commit 3f54bd616159b6758a0a91f71a64fbcdf225579a
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Expand context ID mask for DSP polling mode support

Current FastRPC context uses a 12-bit mask for context ID and PD type.
This expands it to a 16-bit layout for DSP polling mode support, ensuring
the async mode bit doesn't overlap with odd context IDs.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 41c2ec0bc6281..78bd5b8f67f85 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -38,7 +38,7 @@
>  #define FASTRPC_CTX_MAX (256)
>  #define FASTRPC_INIT_HANDLE	1
>  #define FASTRPC_DSP_UTILITIES_HANDLE	2
> -#define FASTRPC_CTXID_MASK GENMASK(11, 4)
> +#define FASTRPC_CTXID_MASK GENMASK(15, 8)

[Severity: High]
This isn't a bug introduced by this patch, but seeing the context ID mask
update prompted me to look at how the IDR handles contexts, and I noticed a
potential circular reference.

Looking at fastrpc_internal_invoke(), if an RPC invoke is interrupted
(-ERESTARTSYS) or times out, it skips dropping the context's initial reference
and leaves it in fl->pending indefinitely:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
    ...
    if (err == -ERESTARTSYS) {
    ...
}

However, ctx holds a strong reference to the fastrpc_user object (fl) created
in fastrpc_context_alloc():

drivers/misc/fastrpc.c:fastrpc_context_alloc() {
    ...
    /* Take a reference to user, released in fastrpc_context_free() */
    fastrpc_user_get(user);
    ...
}

Does this create a circular reference where fl's refcount can never reach 0,
meaning fastrpc_user_free() is never called to clean up the abandoned contexts?

Can this be triggered by userspace sending a signal to a waiting thread?

[Severity: High]
This is a pre-existing issue, but I also noticed a potential Use-After-Free
race condition regarding the context IDR.

In fastrpc_rpmsg_callback(), the context is retrieved without acquiring a
reference count (e.g., using kref_get_unless_zero) before dropping the lock:

drivers/misc/fastrpc.c:fastrpc_rpmsg_callback() {
    ...
    spin_lock_irqsave(&cctx->lock, flags);
    ctx = idr_find(&cctx->ctx_idr, ctxid);
    spin_unlock_irqrestore(&cctx->lock, flags);
    ...
}

If the context's refcount concurrently reaches zero, fastrpc_context_free() is
called, but the object is removed from the IDR relatively late, after freeing
buffers:

drivers/misc/fastrpc.c:fastrpc_context_free() {
    ...
    spin_lock_irqsave(&cctx->lock, flags);
    idr_remove(&cctx->ctx_idr, FIELD_GET(FASTRPC_CTXID_MASK, ctx->ctxid));
    spin_unlock_irqrestore(&cctx->lock, flags);
    ...
}

Is there a wide race window here where a duplicate or delayed DSP response
could find the context in the IDR, and then write to ctx->retval or schedule a
work item on the freed object?

[Severity: High]
This isn't a bug introduced by this patch, but do the DMA buffers moved to
invoke_interrupted_mmaps get permanently leaked?

When fastrpc_internal_invoke() is interrupted by a signal, buffers are moved
to the interrupted list:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
    ...
            list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps);
    ...
}

But when the DSP channel is closed or crashes, fastrpc_rpmsg_remove() iterates
over this list and removes the nodes without calling fastrpc_buf_free():

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
        list_del(&buf->node);
    ...
}

Are these memory map buffers deliberately dropped, or is this leaking the
underlying kernel DMA memory?

[Severity: High]
This is a pre-existing issue, but is the DSP's outstanding reference count on
fastrpc_invoke_ctx ever dropped during rpmsg device removal?

If the DSP crashes and fastrpc_rpmsg_remove() runs, it unblocks waiting threads
by setting -EPIPE and signaling completions:

drivers/misc/fastrpc.c:fastrpc_notify_users() {
    ...
        ctx->retval = -EPIPE;
        complete(&ctx->work);
    ...
}

Previously, fastrpc_invoke_send() acquired a strong reference on ctx on
behalf of the DSP, which is normally dropped by fastrpc_rpmsg_callback() when
the DSP replies.

Does setting the completion here without dropping that reference leave the
contexts with a non-zero refcount forever?

[Severity: High]
This isn't a bug introduced by this patch, but could fastrpc_invoke_ctx be
exposed to the IDR before its refcount is initialized?

In fastrpc_context_alloc(), the ctx is inserted into the IDR and the lock is
dropped before initializing the refcount:

drivers/misc/fastrpc.c:fastrpc_context_alloc() {
    ...
    spin_unlock_irqrestore(&cctx->lock, flags);

    kref_init(&ctx->refcount);
    ...
}

Could a compromised DSP send a duplicate response matching this newly recycled
ID, triggering fastrpc_rpmsg_callback() to look up the ID, schedule put_work,
and decrement the uninitialized zero refcount (from kzalloc) before the
allocation thread initializes it to 1?

>  #define INIT_FILELEN_MAX (2 * 1024 * 1024)
>  #define INIT_FILE_NAMELEN_MAX (128)
>  #define FASTRPC_DEVICE_NAME	"fastrpc"

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713052236.3033744-1-ekansh.gupta@oss.qualcomm.com?part=3