Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Critical] Blind asynchronous `put_work` scheduling on 0-refcount contexts causes a Use-After-Free and Double Free. - [Critical] Premature drop of the send reference breaks callback lifecycle assumptions, leading to a Use-After-Free. - [High] The patch introduces a build failure by using undefined struct fields, and its logic is broken because the normal callback path does not interact with the new fields. Pre-existing issues: - [Critical] A compromised DSP can trigger a Use-After-Free by sending duplicate replies for the same context. - [High] TOCTOU race condition causing a NULL pointer dereference of `cctx->rpdev` during concurrent endpoint removal. - [High] Timed-out or interrupted invocations permanently leak contexts due to a circular reference. -- commit 560fa1b03e675a7c5d337410e599f919e4a647ef Author: Yousef Alhouseen <[email protected]> misc: fastrpc: release pending invoke refs on rpmsg removal fastrpc_rpmsg_remove() wakes pending invoke waiters when the rpmsg device is removed, but it does not release the send references taken before each request was submitted. Those references normally disappear only when a DSP reply arrives, which cannot be relied on after endpoint removal. Walk the channel IDR during removal, mark in-flight contexts completed, and schedule the send-reference put while waking waiters with -EPIPE. This prevents disconnected channels from pinning invoke contexts indefinitely. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index f3a49384586d1..95e727fa09cc6 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -2484,30 +2484,31 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > return err; > } > > -static void fastrpc_notify_users(struct fastrpc_user *user) > +static int fastrpc_notify_context(int id, void *ptr, void *data) > { > - struct fastrpc_invoke_ctx *ctx; > + struct fastrpc_invoke_ctx *ctx = ptr; > > - spin_lock(&user->lock); > - list_for_each_entry(ctx, &user->pending, node) { > - ctx->retval = -EPIPE; > - complete(&ctx->work); > + if (ctx->sent && !ctx->completed) { [Severity: High] Does this patch fail to compile? Looking at struct fastrpc_invoke_ctx, the fields sent and completed do not appear to exist. If these fields were manually added to fix the build, does this logic still introduce a refcount underflow? If fastrpc_rpmsg_callback() receives a normal reply, it does not seem to set ctx->completed = true. Would a concurrent execution of fastrpc_notify_context() see !ctx->completed and schedule put_work concurrently with the callback, causing double execution of fastrpc_context_put()? > + ctx->completed = true; > + schedule_work(&ctx->put_work); [Severity: Critical] Can this schedule put_work on a context that has already reached a refcount of zero? When a context's refcount drops to zero, fastrpc_context_free() is executed: fastrpc_context_free() { ... spin_lock_irqsave(&cctx->lock, flags); idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4); spin_unlock_irqrestore(&cctx->lock, flags); ... } If fastrpc_rpmsg_remove() acquires cctx->lock first, could idr_for_each() pass the zero-refcount context to fastrpc_notify_context() before it is removed from the IDR? Since schedule_work() is called without using kref_get_unless_zero(), wouldn't put_work execute fastrpc_context_put() on a dying object, underflowing the refcount and executing fastrpc_context_free() again? [Severity: Critical] Does prematurely dropping the send reference break the lifecycle assumptions of the callback? Looking at fastrpc_rpmsg_callback(): fastrpc_rpmsg_callback() { ... spin_unlock_irqrestore(&cctx->lock, flags); if (!ctx) { ... } ctx->retval = rsp->retval; complete(&ctx->work); ... } The callback drops cctx->lock and relies on the send reference to keep the context alive while it writes to ctx->retval. If fastrpc_rpmsg_remove() runs concurrently and schedules put_work, could the send reference be dropped and the waiter awakened? If the waiter then drops its own reference, the context could be freed before fastrpc_rpmsg_callback() resumes to write to ctx->retval, potentially causing a use-after-free? [Severity: Critical] This is a pre-existing issue, but can a compromised DSP trigger a use-after-free by sending duplicate replies? Looking at fastrpc_rpmsg_callback(), it looks up a context via ctxid and calls schedule_work(&ctx->put_work) without verifying if it was already processed. Since the context is only removed from the IDR when its refcount hits zero, could duplicate replies from the DSP queue the work item again before the final reference is dropped, causing an extra fastrpc_context_put() and underflowing the refcount? > } > - spin_unlock(&user->lock); > + > + ctx->retval = -EPIPE; > + complete(&ctx->work); > + > + return 0; > } > > static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) > { > struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); > struct fastrpc_buf *buf, *b; > - struct fastrpc_user *user; > unsigned long flags; > > /* No invocations past this point */ > spin_lock_irqsave(&cctx->lock, flags); > cctx->rpdev = NULL; [Severity: High] This is a pre-existing issue, but does setting cctx->rpdev to NULL here introduce a TOCTOU race condition that can result in a NULL pointer dereference? In fastrpc_internal_invoke(), there is a lockless check for !fl->cctx->rpdev. If fastrpc_rpmsg_remove() acquires cctx->lock and sets cctx->rpdev = NULL concurrently, could fastrpc_invoke_send() later unconditionally dereference cctx->rpdev->ept without acquiring cctx->lock or re-validating the pointer? fastrpc_invoke_send() { ... ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg)); ... } > - list_for_each_entry(user, &cctx->users, user) > - fastrpc_notify_users(user); > + idr_for_each(&cctx->ctx_idr, fastrpc_notify_context, NULL); > spin_unlock_irqrestore(&cctx->lock, flags); [Severity: High] This is a pre-existing issue, but do timed-out or interrupted invocations permanently leak contexts? Looking at fastrpc_internal_invoke(): 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); } ... } If the invocation is interrupted or times out, it skips calling fastrpc_context_put(), leaving the context on fl->pending with its first reference held. Since the context holds a reference to the fastrpc_user (fl) via fastrpc_user_get(), would the user's refcount never reach zero, preventing fastrpc_user_free() from ever running and permanently leaking the context? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
