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