[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@xxxxxxxxxxxxxxx
Signed-off-by: bo cao <treebacker@xxxxxxxxxxx>
---
 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)