Re: [PATCH v13 1/4] misc: fastrpc: Move fdlist to invoke context structure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Arbitrary kernel memory read to userspace via untrusted `rpra` pointers.
- [Critical] Use-After-Free via DSP-manipulated `fdlist` dropping active map references.
- [Medium] Memory leak of `fastrpc_map` objects on timeout or interruption.
--

--- Patch [1]: [PATCH v13 1/4] misc: fastrpc: Move fdlist to invoke context structure ---
commit 3273bebbc8c035be416508d67400b312f049c6e6
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Move fdlist to invoke context structure

This commit moves the fdlist from being recalculated during
fastrpc_put_args() to being stored directly in the fastrpc_invoke_ctx
structure. This centralizes its handling and simplifies the meta buffer
preparation and reading logic.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d86e79134c681..02c2765e1188c 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -1165,18 +1167,11 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
>  	union fastrpc_remote_arg *rpra = ctx->rpra;
>  	struct fastrpc_user *fl = ctx->fl;
>  	struct fastrpc_map *mmap = NULL;
> -	struct fastrpc_invoke_buf *list;
> -	struct fastrpc_phy_page *pages;
> -	u64 *fdlist;
> -	int i, inbufs, outbufs, handles;
> +	u64 *fdlist = ctx->fdlist;
> +	int i, inbufs;
>  	int ret = 0;
>  
>  	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> -	outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
> -	handles = REMOTE_SCALARS_INHANDLES(ctx->sc) + REMOTE_SCALARS_OUTHANDLES(ctx->sc);
> -	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
> -	pages = fastrpc_phy_page_start(list, ctx->nscalars);
> -	fdlist = (uint64_t *)(pages + inbufs + outbufs + handles);
>  
>  	for (i = inbufs; i < ctx->nbufs; ++i) {
>  		if (!ctx->maps[i]) {

[Severity: Medium]
This is a pre-existing issue, but does this code leak the fastrpc_map
objects if an invoke operation times out or is interrupted?

If fastrpc_internal_invoke() exits early for timeouts or signals, it skips
fastrpc_put_args() and leaves the context in the pending list:

drivers/misc/fastrpc.c: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);
    }
    ...
}

When the DSP eventually replies, the completion callback asynchronously frees
the context via fastrpc_context_free(). However, fastrpc_context_free() does
not execute the fdlist cleanup logic:

drivers/misc/fastrpc.c:fastrpc_context_free() {
    ...
    for (i = 0; i < ctx->nbufs; i++)
        fastrpc_map_put(ctx->maps[i]);
    ...
}

Are maps the DSP returned in fdlist to be released permanently leaked in this
scenario?

[Severity: Critical]
This is a pre-existing issue, but can a compromised DSP cause a
use-after-free by manipulating the untrusted fdlist?

In fastrpc_put_args(), the kernel iterates over fdlist and drops references
for maps without verifying if they are actively in use by the current
context:

drivers/misc/fastrpc.c:fastrpc_put_args() {
    ...
    for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
        if (!fdlist[i])
            break;
        if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
            fastrpc_map_put(mmap);
    }
    ...
}

If a malicious DSP injects file descriptors of actively used maps (e.g.,
those in ctx->maps[i]), it could cause their reference count to drop to zero
prematurely. When the context is subsequently destroyed, the kernel drops
references again:

drivers/misc/fastrpc.c:fastrpc_context_free() {
    ...
    for (i = 0; i < ctx->nbufs; i++)
        fastrpc_map_put(ctx->maps[i]);
    ...
}

Could this lead to memory corruption when ctx->maps[i] is freed twice?

[Severity: Critical]
This is a pre-existing issue, but does this code allow arbitrary kernel memory
disclosure via untrusted rpra pointers?

The rpra array resides within ctx->buf->virt, a DMA-mapped buffer accessible
by the DSP. In fastrpc_put_args(), the kernel blindly reads the source pointer
and length directly from this shared memory:

drivers/misc/fastrpc.c:fastrpc_put_args() {
    ...
            void *src = (void *)(uintptr_t)rpra[i].buf.pv;
            void *dst = (void *)(uintptr_t)ctx->args[i].ptr;
            u64 len = rpra[i].buf.len;

            if (!kernel) {
                if (copy_to_user((void __user *)dst, src, len)) {
    ...
}

If a compromised DSP manipulates rpra[i].buf.pv and rpra[i].buf.len to point
to arbitrary kernel addresses, will the kernel pass these directly to
copy_to_user(), leaking sensitive data to userspace?

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