Re: [PATCH v13 4/4] misc: fastrpc: Add polling mode support for fastRPC driver

[sashiko-bot@kernel.org]

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 <ekansh.gupta@oss.qualcomm.com>

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/20260713052236.3033744-1-ekansh.gupta@oss.qualcomm.com?part=4