[PATCH 1/2] misc: fastrpc: pin invoke context at glink response lookup

[PATCH 1/2] misc: fastrpc: pin invoke context at glink response lookup

[bo cao]

fastrpc_rpmsg_callback() looks up the invoke context under cctx->lock
but then uses it after dropping the lock, including the ctxid read in
the sequence validation itself, ctx->retval, complete() and
schedule_work().

The final kref_put() happens outside the lock (in the invoking
thread's bail path or in the put_work worker) and
fastrpc_context_free() removes the context from ctx_idr only after
releasing its maps and DMA buffer, so a context found in the IDR may
already have a zero refcount with free in progress:

    callback                          freeing thread
    --------                          --------------
    lock; idr_find() -> ctx; unlock
                                      (refcount already 0; free is
                                       releasing maps and buf)
                                      lock; idr_remove(); unlock
                                      kfree(ctx)
    read ctx->ctxid for the seq
    check -- use after free

Take a reference on the context inside the same critical section as
the idr_find() lookup and run the sequence validation there too;
drop the reference after scheduling the worker (or when the response
is dropped).  kref_get_unless_zero() is required because the IDR may
still hold a context whose refcount has already reached zero while
fastrpc_context_free() is running.

Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
Cc: stable@vger.kernel.org
Signed-off-by: bo cao <treebacker@foxmail.com>
---
 drivers/misc/fastrpc.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index af18ff1992ee..c77048076aee 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2724,6 +2724,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 	struct fastrpc_invoke_ctx *ctx;
 	unsigned long flags;
 	unsigned long ctxid;
+	bool stale = false;
 
 	if (len < sizeof(*rsp))
 		return -EINVAL;
@@ -2743,6 +2744,13 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 
 	spin_lock_irqsave(&cctx->lock, flags);
 	ctx = idr_find(&cctx->ctx_idr, ctxid);
+	if (ctx && !kref_get_unless_zero(&ctx->refcount)) {
+		/* Context is being torn down */
+		ctx = NULL;
+	}
+	if (ctx && FIELD_GET(FASTRPC_CTXID_SEQ_MASK, rsp->ctx) !=
+		   FIELD_GET(FASTRPC_CTXID_SEQ_MASK, ctx->ctxid))
+		stale = true;
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	if (!ctx) {
@@ -2751,19 +2759,17 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 	}
 
 	/*
-	 * 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.
+	 * The IDR slot can be recycled for a new request before a late
+	 * (or duplicate) glink COMPLETE response for the previous request
+	 * arrives, so the sequence number was validated above, under
+	 * cctx->lock.
 	 */
-	if (FIELD_GET(FASTRPC_CTXID_SEQ_MASK, rsp->ctx) !=
-	    FIELD_GET(FASTRPC_CTXID_SEQ_MASK, ctx->ctxid)) {
+	if (stale) {
 		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));
+		fastrpc_context_put(ctx);
 		return 0;
 	}
 
@@ -2778,6 +2784,8 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 	 */
 	schedule_work(&ctx->put_work);
 
+	fastrpc_context_put(ctx);
+
 	return 0;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 1/2] misc: fastrpc: pin invoke context at glink response lookup

[Greg KH]

On Fri, Oct 30, 2026 at 04:19:55PM +0800, bo cao wrote:
> fastrpc_rpmsg_callback() looks up the invoke context under cctx->lock
> but then uses it after dropping the lock, including the ctxid read in
> the sequence validation itself, ctx->retval, complete() and
> schedule_work().
> 
> The final kref_put() happens outside the lock (in the invoking
> thread's bail path or in the put_work worker) and
> fastrpc_context_free() removes the context from ctx_idr only after
> releasing its maps and DMA buffer, so a context found in the IDR may
> already have a zero refcount with free in progress:
> 
>     callback                          freeing thread
>     --------                          --------------
>     lock; idr_find() -> ctx; unlock
>                                       (refcount already 0; free is
>                                        releasing maps and buf)
>                                       lock; idr_remove(); unlock
>                                       kfree(ctx)
>     read ctx->ctxid for the seq
>     check -- use after free
> 
> Take a reference on the context inside the same critical section as
> the idr_find() lookup and run the sequence validation there too;
> drop the reference after scheduling the worker (or when the response
> is dropped).  kref_get_unless_zero() is required because the IDR may
> still hold a context whose refcount has already reached zero while
> fastrpc_context_free() is running.
> 
> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Cc: stable@vger.kernel.org
> Signed-off-by: bo cao <treebacker@foxmail.com>
> ---
>  drivers/misc/fastrpc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)

These two messages are not "threaded" properly, sent as 2 separate
emails.  Is that intentional?  It breaks our tooling :(

Please use something like 'git send-email' or 'b4' for sending patches
out, that's what those tools were written for.

thanks,

greg k-h

Re: fastrpc: use-after-free of fastrpc_invoke_ctx on duplicate glink responses in fastrpc_rpmsg_callback()

[bo cao]

Hi Greg,

> These two messages are not "threaded" properly, sent as 2 separate
> emails.  Is that intentional?  It breaks our tooling :(

Not intentional -- I sent them individually instead of as a series.
Sorry about that.  Resent as a properly threaded [PATCH v2 0/2]
series with git send-email (no code changes):

  0/2: https://lore.kernel.org/all/tencent_AE37F59DA9A6DAB192F6BAFD054E584FAB05@qq.com/
  1/2: https://lore.kernel.org/all/tencent_E9BD7808AB6A2DA5FA326D547F57D65D270A@qq.com/
  2/2: https://lore.kernel.org/all/tencent_C7D71E2F51C2195A0CA91A4C11D51B957708@qq.com/

Thanks,
bo cao