Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Unprivileged Denial of Service via IDR slot exhaustion and memory leaks in polling mode. - [High] Data race on `ctx->is_work_done` and `ctx->retval` due to missing memory barriers. Pre-existing issues: - [Critical] Duplicate Glink completion responses bypass sequence validation and cause a Use-After-Free. - [High] Accessing `ctx->ctxid` outside the IDR lock leads to a Use-After-Free. -- --- Patch [4]: [PATCH v13 4/4] misc: fastrpc: Add polling mode support for fastRPC driver --- commit d9afeb4ebcb604d7e06c0448d2aa66154de980d6 Author: Ekansh Gupta <[email protected]> misc: fastrpc: Add polling mode support for fastRPC driver This commit adds polling mode support for the fastRPC driver to avoid CPU wakeup and scheduling overhead during glink responses. It polls a shared memory location for completion after sending the invocation. It also embeds a sequence number in the context ID to prevent recycling issues. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > +static int poll_for_remote_response(struct fastrpc_invoke_ctx *ctx) > +{ > + u32 val; > + int ret; > + > + /* > + * Poll until DSP writes FASTRPC_POLL_RESPONSE into *ctx->poll_addr > + * or until another path marks the work done. > + */ > + ret = read_poll_timeout_atomic(fastrpc_read_poll_addr, val, > + (val == FASTRPC_POLL_RESPONSE) || > ctx->is_work_done, 1, [Severity: High] Are there missing memory barriers when updating these fields? When the Glink response arrives, fastrpc_rpmsg_callback() updates ctx->retval and then sets ctx->is_work_done = true. Because there are no write memory barriers, the CPU or compiler can reorder these stores. Concurrently, poll_for_remote_response() spins reading ctx->is_work_done without a read barrier. If the polling thread observes is_work_done == true before the retval update is visible, could it break the loop and read the stale initial value of ctx->retval (-1), causing the invocation to incorrectly fail? [ ... ] > +static int fastrpc_wait_for_completion(struct fastrpc_invoke_ctx *ctx, > + u32 kernel) > +{ > + int err; > + > + if (ctx->is_polled) { > + err = poll_for_remote_response(ctx); > + if (!err) > + return 0; [Severity: Critical] Does returning early on polling success lead to IDR slot exhaustion and memory leaks? If a userspace process invokes a DSP module in polling mode, the DSP updates the poll memory but might maliciously omit the Glink completion response. The CPU thread completes the polling successfully, returns to userspace, and deletes the context from user->pending in fastrpc_internal_invoke(). Because the Glink callback never runs, the context's reference count never reaches zero, and it appears to be permanently abandoned in the IDR. Could an unprivileged process loop this to quickly exhaust the 256 FASTRPC_CTX_MAX IDR slots? [ ... ] > @@ -2559,7 +2719,25 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device > *rpdev, void *data, > return -ENOENT; > } > > + /* > + * Validate the sequence number embedded in the upper bits of the > + * context ID. Under high concurrency the IDR slot can be recycled > + * for a new request before a late (or duplicate) glink COMPLETE > + * response for the previous request arrives. Without this check the > + * driver would call complete() on the wrong context, waking a thread > + * whose buffers are still being accessed by the DSP. > + */ > + if (FIELD_GET(FASTRPC_CTXID_SEQ_MASK, rsp->ctx) != > + FIELD_GET(FASTRPC_CTXID_SEQ_MASK, ctx->ctxid)) { [Severity: High] This isn't a bug introduced by this patch, but does accessing ctx->ctxid outside the IDR lock lead to a use-after-free? Looking at fastrpc_rpmsg_callback(), idr_find() is called to retrieve the context, but the spinlock (cctx->lock) is released immediately after without incrementing the context's reference count. If another thread drops the final reference to this context concurrently (such as a timeout or worker thread), fastrpc_context_free() will remove it from the IDR and free the memory. Can fastrpc_rpmsg_callback() then access freed memory when checking ctx->ctxid? [Severity: Critical] This is a pre-existing issue, but do duplicate Glink completion responses bypass the sequence validation and cause a use-after-free? The sequence number check mitigates responses intended for a recycled IDR slot, but it does not protect against a malicious DSP sending multiple Glink completion responses for the currently active request. If a duplicate response is sent, the sequence number matches, and fastrpc_rpmsg_callback() blindly calls schedule_work(&ctx->put_work) again. If the first scheduled work has already completed, will the duplicate response successfully queue it a second time, erroneously dropping the reference count to zero and freeing the context while the CPU thread is still using it? > + dev_dbg(&rpdev->dev, > + "Stale glink response ctx 0x%llx (expected seq 0x%llx), > dropping\n", > + rsp->ctx, > + FIELD_GET(FASTRPC_CTXID_SEQ_MASK, ctx->ctxid)); > + return 0; > + } > + > ctx->retval = rsp->retval; > + ctx->is_work_done = true; > complete(&ctx->work); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
