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 <[email protected]> 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/[email protected]?part=1
