Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] The kernel trusts pointer and length values read directly from the DSP-shared DMA buffer, allowing an arbitrary kernel read/write vulnerability. - [High] Context is published to the IDR before its refcount is initialized, allowing a concurrent DSP callback to trigger a Use-After-Free. - [High] `cctx->rpdev` is read locklessly after an initial check, leading to a NULL pointer dereference if `fastrpc_rpmsg_remove` runs concurrently. - [High] Integer truncation of `mlen` breaks buffer serialization and leads to DMA memory corruption. -- commit 00821efcac58f87521dc0b1c609d5329a9afc752 Author: Yousef Alhouseen <[email protected]> misc: fastrpc: reject overflowing invoke buffer ranges This commit adds an overflow check to fastrpc_get_buff_overlaps(). It verifies that adding the buffer length to its start address does not overflow, returning -EOVERFLOW if it does. This prevents the use of wrapped end addresses that could misstate payload sizes or feed invalid ranges into invoke metadata. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f3a49384586d1..ba4ade8747833 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -675,7 +681,13 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc( > return ERR_PTR(-ENOMEM); > } > ctx->args = args; > - fastrpc_get_buff_overlaps(ctx); > + ret = fastrpc_get_buff_overlaps(ctx); > + if (ret) { > + kfree(ctx->olaps); > + kfree(ctx->maps); > + kfree(ctx); > + return ERR_PTR(ret); > + } > } > > /* Released in fastrpc_context_put() */ [Severity: High] This isn't a bug introduced by this patch, but since we are modifying fastrpc_context_alloc(), could the context be published to the IDR before it is fully initialized? Further down in this function, the context is published: ret = idr_alloc_cyclic(&cctx->ctx_idr, ctx, 1, FASTRPC_CTX_MAX, GFP_ATOMIC); But the reference count is initialized later: kref_init(&ctx->refcount); If an asynchronous callback like fastrpc_rpmsg_callback() retrieves this newly published context from the IDR and drops its reference, could it free the context before kref_init() executes, leading to a use-after-free? [Severity: High] This is a pre-existing issue, but looking at fastrpc_internal_invoke(), is there a lockless check that could lead to a NULL pointer dereference? if (!fl->cctx->rpdev) return -EPIPE; If this check passes, and a concurrent device unbind clears the pointer in fastrpc_rpmsg_remove() under a spinlock: spin_lock_irqsave(&cctx->lock, flags); cctx->rpdev = NULL; ... spin_unlock_irqrestore(&cctx->lock, flags); Could the invocation thread then dereference the NULL pointer when it calls fastrpc_invoke_send()? ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg)); [Severity: Critical] This is a pre-existing issue, but does fastrpc_put_args() trust pointers and lengths read directly from the DSP-shared DMA buffer without validation? In 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 supplies arbitrary addresses in this shared region, could this result in an arbitrary kernel read or write? [Severity: High] This is a pre-existing issue, but does fastrpc_get_args() truncate the 64-bit overlap size into a 32-bit signed integer? for (oix = 0; oix < ctx->nbufs; ++oix) { int mlen; ... mlen = ctx->olaps[oix].mend - ctx->olaps[oix].mstart; if (rlen < mlen) goto bail; rpra[i].buf.pv = args - ctx->olaps[oix].offset; ... args = args + mlen; If the overlap exceeds 2GB, mlen could truncate to a negative value or a small positive value. Could this cause unsigned promotion to erroneously reject the buffer, or fail to advance the payload pointer sufficiently, overwriting subsequent buffers? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
