Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] fastrpc: missing 'sent' flag causes indefinite hang and UAF on retry - [High] fastrpc: resumed context uses stale user-space pointers for DSP output - [High] fastrpc: Use-After-Free in fastrpc_rpmsg_callback due to concurrent teardown - [Medium] fastrpc: memory leak of timed-out contexts on channel teardown --
commit 6840686b144525cb6d359cfc50b159c177c03bae Author: Anandu Krishnan E <[email protected]> misc: fastrpc: fix context leak and hang on signal-interrupted invoke fastrpc invokes work by sending an RPC message to the DSP and blocking in wait_for_completion_interruptible() until the DSP responds. If a signal arrives during this wait, the syscall returns -ERESTARTSYS and the invoke context which holds the in-flight DMA buffers and completion state is left stranded in fl->pending. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 5ac7b3e78ba7e..f51cb772e6831 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1370,9 +1444,21 @@ static int fastrpc_internal_invoke(struct fastrpc_user > *fl, u32 kernel, > return -EPERM; > } > > - ctx = fastrpc_context_alloc(fl, kernel, sc, args); > - if (IS_ERR(ctx)) > - return PTR_ERR(ctx); > + if (!kernel) > + ctx = fastrpc_context_restore_interrupted(fl, handle, sc); > + > + if (ctx) { > + /* ctx->args is already populated from before the interruption. > */ > + resumed = true; > + } else { > + ctx = fastrpc_context_alloc(fl, kernel, sc, args); > + if (IS_ERR(ctx)) > + return PTR_ERR(ctx); > + ctx->handle = handle; > + } > + [Severity: High] Does this code ignore the new args structure provided by the retrying user-space caller? The code appears to retain the stale pointers captured before the interruption, failing to update ctx->args with the retry's arguments. When the DSP responds, fastrpc_put_args() might copy the output to the original, potentially freed or repurposed user-space addresses. > + if (resumed) > + goto wait; [Severity: High] Can skipping the message sending cause a thread to get permanently hung? Because the missing 'sent' flag check fails to detect that the message wasn't delivered initially, the resumed context may incorrectly skip sending the message here. > > err = fastrpc_get_args(kernel, ctx); > if (err) [ ... ] > @@ -1409,21 +1496,13 @@ static int fastrpc_internal_invoke(struct > fastrpc_user *fl, u32 kernel, > goto bail; > > bail: > - 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 (err == -ERESTARTSYS) { > + fastrpc_context_save_interrupted(ctx); > + } else { [Severity: High] Is the context unconditionally parked as resumable without verifying if the message was actually sent? The commit message mentions this is handled via a 'sent' flag, but the flag appears to be missing in the patch, which could park unsent contexts as resumable. > spin_lock(&fl->lock); > - list_for_each_entry_safe(buf, b, &fl->mmaps, node) { > - list_del(&buf->node); > - list_add_tail(&buf->node, > &fl->cctx->invoke_interrupted_mmaps); > - } > + list_del_init(&ctx->node); > spin_unlock(&fl->lock); > + fastrpc_context_put(ctx); > } [Severity: Medium] When a context times out (err == -ETIMEDOUT), does this code remove it from the tracking lists? This would make it unreachable for the fastrpc_notify_users() cleanup sweep, potentially leaking the context and its DSP reference if the DSP never responds and the channel is torn down. [ ... ] > @@ -2669,13 +2762,22 @@ static void fastrpc_notify_users(struct fastrpc_user > *user) > ctx->retval = -EPIPE; > complete(&ctx->work); > } > + > + /* Drops only the worker ref; the invoker ref is released by > fastrpc_user_free(). */ > + list_for_each_entry(ctx, &user->interrupted, node) { [Severity: Medium] Because the timed-out contexts are removed from fl->pending and fl->interrupted, does this teardown routine miss cleaning them up when the DSP reference is still held? > + ctx->retval = -EPIPE; > + complete(&ctx->work); > + if (!ctx->put_work_scheduled) { > + ctx->put_work_scheduled = true; > + schedule_work(&ctx->put_work); > + } > + } > spin_unlock(&user->lock); > } [ ... ] > @@ -2778,9 +2877,11 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device > *rpdev, void *data, > /* > * The DMA buffer associated with the context cannot be freed in > * interrupt context so schedule it through a worker thread to > - * avoid a kernel BUG. > + * avoid a kernel BUG. fastrpc_context_schedule_put_work() guards > + * against a concurrent fastrpc_notify_users() also scheduling this > + * same work item for an interrupted context. > */ > - schedule_work(&ctx->put_work); > + fastrpc_context_schedule_put_work(ctx); > > return 0; > } [Severity: High] Can a concurrent channel teardown or file descriptor closure cause a use-after-free here? Earlier in this function, the context is found in the IDR and the cctx->lock is dropped without elevating the refcount. Concurrent teardown paths can drop both the invoker and DSP references, freeing the context before the callback reads ctx->ctxid or accesses it here. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261002-anandu-fastrpc-interrupted-invoke-v3-v3-1-31e90439d...@oss.qualcomm.com?part=1
