Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Use-After-Free in fastrpc_rpmsg_callback due to lockless context access without reference increment. - [High] Data race and list corruption when handling -ERESTARTSYS in fastrpc_internal_invoke. -- --- Patch [1]: [PATCH v2] misc: fastrpc: Drop unhandled DSP PD exit notification --- commit c653a512607ab348a2abca70e47797b728162a44 Author: Shawn Guo <[email protected]> misc: fastrpc: Drop unhandled DSP PD exit notification Newer DSP firmware implements a PD (Protection Domain) notification framework that sends PD state notifications upon request. This commit drops the unhandled PD exit notification to prevent it from being incorrectly matched against the context idr. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f3a49384586d1..a970d0995e2cb 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2542,6 +2553,14 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device > *rpdev, void *data, > if (!cctx) > return -ENODEV; > > + /* > + * A PD exit notification from the DSP PD notification framework carries > + * this sentinel rather than a real context. Drop it: a real context is > + * (idr_index << 4) | pd and can never collide with this value. > + */ > + if (rsp->ctx == FASTRPC_DSP_PD_NOTIFY_CTX) > + return 0; > + > ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4); > > spin_lock_irqsave(&cctx->lock, flags); [Severity: Critical] This is a pre-existing issue, but does dropping the lock immediately after this without incrementing the reference count of the context risk a use-after-free? drivers/misc/fastrpc.c:fastrpc_rpmsg_callback() { ... ctx = idr_find(&cctx->ctx_idr, ctxid); spin_unlock_irqrestore(&cctx->lock, flags); if (!ctx) { dev_err(&rpdev->dev, "No context ID matches response\n"); return -ENOENT; } ctx->retval = rsp->retval; complete(&ctx->work); ... } If a response arrives exactly as the context is being freed by a concurrent timeout or signal, couldn't the context be freed immediately after the lock is dropped, causing the write to ctx->retval to corrupt memory? I also noticed another pre-existing issue in the same file that might be worth looking at: drivers/misc/fastrpc.c:fastrpc_internal_invoke() { ... if (err == -ERESTARTSYS) { list_for_each_entry_safe(buf, b, &fl->mmaps, node) { list_del(&buf->node); list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps); } } ... } [Severity: High] This is a pre-existing issue, but are these list operations safe to perform locklessly here? If multiple threads sharing the same file descriptor issue concurrent ioctls, wait in fastrpc_internal_invoke(), and receive a signal like SIGINT, they would all concurrently modify fl->mmaps and fl->cctx->invoke_interrupted_mmaps. Could this lead to list corruption since neither fl->lock nor cctx->lock appears to be held? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
