Skip to contentSkip to conversation

misc: fastrpc: fixes for 7.1

PATCH New [email protected] 1 participant 5 messages May 30

Subsystems (3)
Stable branch Char and misc drivers Qualcomm fastrpc driver
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

cover misc: fastrpc: fixes for 7.1

From: Srinivas Kandagatla <[email protected]>

Hi Greg,

Here are some fixes in ngd-ctrl driver which can go for 7.1 release Could you please queue these as 7.1 material.

Fixes include
- two use-after-free cases fixed by Anandu and Xiao.
- NULL pointer dereference fix
- DMA address corruption due to find_vma misuse

Thanks for all the help,

Srini

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Anandu Krishnan E (1):
misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

Junrui Luo (1):
misc: fastrpc: fix DMA address corruption due to find_vma misuse

Mukesh Ojha (1):
misc: fastrpc: Fix NULL pointer dereference in rpmsg callback

Zhenghang Xiao (1):
misc: fastrpc: fix use-after-free race in fastrpc_map_create

 drivers/misc/fastrpc.c | 107 ++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 39 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH 1/4 misc: fastrpc: fix use-after-free of fastrpc_user in workqueue context

From: Anandu Krishnan E <[email protected]>

There is a race between fastrpc_device_release() and the workqueue that processes DSP responses. When the user closes the file descriptor, fastrpc_device_release() frees the fastrpc_user structure. Concurrently, an in-flight DSP invocation can complete and fastrpc_rpmsg_callback() schedules context cleanup via schedule_work(&ctx->put_work). If the workqueue runs fastrpc_context_free() in parallel with or after fastrpc_device_release() has freed the user structure, it dereferences the freed fastrpc_user. Depending on the state of the context at the time of the race, any one of the following accesses can be hit:

1. fastrpc_buf_free() calls fastrpc_ipa_to_dma_addr(buf->fl->cctx, ...) to strip the SID bits from the stored IOVA before passing the physical address to dma_free_coherent().

2. fastrpc_free_map() reads map->fl->cctx->vmperms[0].vmid to reconstruct the source permission bitmask needed for the qcom_scm_assign_mem() call that returns memory from the DSP VM back to HLOS.

3. fastrpc_free_map() acquires map->fl->lock to safely remove the

    map node from the fl->maps list.

The resulting use-after-free manifests as:

  pc : fastrpc_buf_free+0x38/0x80 [fastrpc]
  lr : fastrpc_context_free+0xa8/0x1b0 [fastrpc]
  fastrpc_context_free+0xa8/0x1b0 [fastrpc]
  fastrpc_context_put_wq+0x78/0xa0 [fastrpc]
  process_one_work+0x180/0x450
  worker_thread+0x26c/0x388

Add kref-based reference counting to fastrpc_user. Have each invoke context take a reference on the user at allocation time and release it when the context is freed. Release the initial reference in fastrpc_device_release() at file close. Move the teardown of the user structure — freeing pending contexts, maps, mmaps, and the channel context reference — into the kref release callback fastrpc_user_free(), so that it runs only when the last reference is dropped, regardless of whether that happens at device close or after the final in-flight context completes.

Fixes 6cffd79504ce ("misc: fastrpc: Add support for dmabuf exporter")
Cc [email protected]
Signed-off-by Anandu Krishnan E <[email protected]>
Signed-off-by Srinivas Kandagatla <[email protected]>
---
 drivers/misc/fastrpc.c | 75 +++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 23 deletions(-)
drivers/misc/fastrpc.c +52 -23
index 1080f9acf70a..48f8262af539 100644
@@ -310,6 +310,8 @@ struct fastrpc_user {
 	spinlock_t lock;
 	/* lock for allocations */
 	struct mutex mutex;
+	/* Reference count */
+	struct kref refcount;
 };
 
 /* Extract SMMU PA from consolidated IOVA */

@@ -497,15 +499,57 @@ static void fastrpc_channel_ctx_put(struct fastrpc_channel_ctx *cctx)
 	kref_put(&cctx->refcount, fastrpc_channel_ctx_free);
 }
 
+static void fastrpc_context_put(struct fastrpc_invoke_ctx *ctx);
+
+static void fastrpc_user_free(struct kref *ref)
+{
+	struct fastrpc_user *fl = container_of(ref, struct fastrpc_user, refcount);
+	struct fastrpc_invoke_ctx *ctx, *n;
+	struct fastrpc_map *map, *m;
+	struct fastrpc_buf *buf, *b;
+
+	if (fl->init_mem)
+		fastrpc_buf_free(fl->init_mem);
+
+	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
+		list_del(&ctx->node);
+		fastrpc_context_put(ctx);
+	}
+
+	list_for_each_entry_safe(map, m, &fl->maps, node)
+		fastrpc_map_put(map);
+
+	list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+		list_del(&buf->node);
+		fastrpc_buf_free(buf);
+	}
+
+	fastrpc_channel_ctx_put(fl->cctx);
+	mutex_destroy(&fl->mutex);
+	kfree(fl);
+}
+
+static void fastrpc_user_get(struct fastrpc_user *fl)
+{
+	kref_get(&fl->refcount);
+}
+
+static void fastrpc_user_put(struct fastrpc_user *fl)
+{
+	kref_put(&fl->refcount, fastrpc_user_free);
+}
+
 static void fastrpc_context_free(struct kref *ref)
 {
 	struct fastrpc_invoke_ctx *ctx;
 	struct fastrpc_channel_ctx *cctx;
+	struct fastrpc_user *fl;
 	unsigned long flags;
 	int i;
 
 	ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount);
 	cctx = ctx->cctx;
+	fl = ctx->fl;
 
 	for (i = 0; i < ctx->nbufs; i++)
 		fastrpc_map_put(ctx->maps[i]);

@@ -521,6 +565,8 @@ static void fastrpc_context_free(struct kref *ref)
 	kfree(ctx->olaps);
 	kfree(ctx);
 
+	/* Release the reference taken in fastrpc_context_alloc() */
+	fastrpc_user_put(fl);
 	fastrpc_channel_ctx_put(cctx);
 }
 

@@ -628,6 +674,8 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 
 	/* Released in fastrpc_context_put() */
 	fastrpc_channel_ctx_get(cctx);
+	/* Take a reference to user, released in fastrpc_context_free() */
+	fastrpc_user_get(user);
 
 	ctx->sc = sc;
 	ctx->retval = -1;

@@ -658,6 +706,7 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	spin_lock(&user->lock);
 	list_del(&ctx->node);
 	spin_unlock(&user->lock);
+	fastrpc_user_put(user);
 	fastrpc_channel_ctx_put(cctx);
 	kfree(ctx->maps);
 	kfree(ctx->olaps);

@@ -1579,9 +1628,6 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 {
 	struct fastrpc_user *fl = (struct fastrpc_user *)file->private_data;
 	struct fastrpc_channel_ctx *cctx = fl->cctx;
-	struct fastrpc_invoke_ctx *ctx, *n;
-	struct fastrpc_map *map, *m;
-	struct fastrpc_buf *buf, *b;
 	unsigned long flags;
 
 	fastrpc_release_current_dsp_process(fl);

@@ -1590,28 +1636,10 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	list_del(&fl->user);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
-	if (fl->init_mem)
-		fastrpc_buf_free(fl->init_mem);
-
-	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
-		list_del(&ctx->node);
-		fastrpc_context_put(ctx);
-	}
-
-	list_for_each_entry_safe(map, m, &fl->maps, node)
-		fastrpc_map_put(map);
-
-	list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
-		list_del(&buf->node);
-		fastrpc_buf_free(buf);
-	}
-
 	fastrpc_session_free(cctx, fl->sctx);
-	fastrpc_channel_ctx_put(cctx);
-
-	mutex_destroy(&fl->mutex);
-	kfree(fl);
 	file->private_data = NULL;
+	/* Release the reference taken in fastrpc_device_open */
+	fastrpc_user_put(fl);
 
 	return 0;
 }

@@ -1655,6 +1683,7 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	spin_lock_irqsave(&cctx->lock, flags);
 	list_add_tail(&fl->user, &cctx->users);
 	spin_unlock_irqrestore(&cctx->lock, flags);
+	kref_init(&fl->refcount);
 
 	return 0;
 }
Signature · 1 line
2.53.0
author maintainer
#2 lore raw

PATCH 2/4 misc: fastrpc: fix DMA address corruption due to find_vma misuse

In reply to [email protected]

From: Junrui Luo <[email protected]>

fastrpc_get_args() uses find_vma() to look up the VMA for a user-provided pointer and compute a DMA address offset. When the address falls in a gap before the returned VMA, (ptr & PAGE_MASK) - vma->vm_start underflows, corrupting the DMA address sent to the DSP.

Replace find_vma() with vma_lookup(), which returns NULL when the address is not contained within any VMA.

Cc [email protected]
Fixes 80f3afd72bd4 ("misc: fastrpc: consider address offset before sending to DSP")
Reported-by Yuhao Jiang <[email protected]>
Signed-off-by Junrui Luo <[email protected]>
Reviewed-by Dmitry Baryshkov <[email protected]>
Signed-off-by Srinivas Kandagatla <[email protected]>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/misc/fastrpc.c +1 -1
index 48f8262af539..cca7489605c5 100644
@@ -1090,7 +1090,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			pages[i].addr = ctx->maps[i]->dma_addr;
 
 			mmap_read_lock(current->mm);
-			vma = find_vma(current->mm, ctx->args[i].ptr);
+			vma = vma_lookup(current->mm, ctx->args[i].ptr);
 			if (vma)
 				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
 						 vma->vm_start;
Signature · 1 line
2.53.0
author maintainer
#3 lore raw

PATCH 3/4 misc: fastrpc: Fix NULL pointer dereference in rpmsg callback

In reply to [email protected]

From: Mukesh Ojha <[email protected]>

A NULL pointer dereference was observed on Hawi at boot when the DSP sends a glink message before fastrpc_rpmsg_probe() has completed initialization:

  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000178
  pc : _raw_spin_lock_irqsave+0x34/0x8c
  lr : fastrpc_rpmsg_callback+0x3c/0xcc [fastrpc]
  ...
  Call trace:
   _raw_spin_lock_irqsave+0x34/0x8c (P)
   fastrpc_rpmsg_callback+0x3c/0xcc [fastrpc]
   qcom_glink_native_rx+0x538/0x6a4
   qcom_glink_smem_intr+0x14/0x24 [qcom_glink_smem]

The faulting address 0x178 corresponds to the lock variable inside struct fastrpc_channel_ctx, confirming that cctx is NULL when fastrpc_rpmsg_callback() attempts to take the spinlock.

There are two issues here. First, dev_set_drvdata() is called before spin_lock_init() and idr_init(), leaving a window where the callback can retrieve a valid cctx pointer but operate on an uninitialized spinlock. Second, the rpmsg channel becomes live as soon as the driver is bound, so fastrpc_rpmsg_callback() can fire before dev_set_drvdata() is called at all, resulting in dev_get_drvdata() returning NULL.

Fix both issues by moving all cctx initialization ahead of dev_set_drvdata() so the structure is fully initialized before it becomes visible to the callback, and add a NULL check in fastrpc_rpmsg_callback() as a guard against any remaining window.

Fixes f6f9279f2bf0 ("misc: fastrpc: Add Qualcomm fastrpc basic driver model")
Cc [email protected]
Signed-off-by Mukesh Ojha <[email protected]>
Reviewed-by Bjorn Andersson <[email protected]>
Signed-off-by Srinivas Kandagatla <[email protected]>
---
 drivers/misc/fastrpc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/misc/fastrpc.c +4 -1
index cca7489605c5..47cf3d21b51d 100644
@@ -2460,7 +2460,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 
 	kref_init(&data->refcount);
 
-	dev_set_drvdata(&rpdev->dev, data);
 	rdev->dma_mask = &data->dma_mask;
 	dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32));
 	INIT_LIST_HEAD(&data->users);

@@ -2469,6 +2468,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	idr_init(&data->ctx_idr);
 	data->domain_id = domain_id;
 	data->rpdev = rpdev;
+	dev_set_drvdata(&rpdev->dev, data);
 
 	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
 	if (err)

@@ -2542,6 +2542,9 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
 	if (len < sizeof(*rsp))
 		return -EINVAL;
 
+	if (!cctx)
+		return -ENODEV;
+
 	ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4);
 
 	spin_lock_irqsave(&cctx->lock, flags);
Signature · 1 line
2.53.0
author maintainer
#4 lore raw

PATCH 4/4 misc: fastrpc: fix use-after-free race in fastrpc_map_create

In reply to [email protected]

From: Zhenghang Xiao <[email protected]>

fastrpc_map_lookup returns a raw pointer after releasing fl->lock. The caller fastrpc_map_create then calls fastrpc_map_get (kref_get_unless_zero) on this unprotected pointer. A concurrent MEM_UNMAP can free the map between the lock release and the kref operation, resulting in a use-after-free on the freed slab object.

Restore the take_ref parameter to fastrpc_map_lookup so the reference is acquired atomically under fl->lock before the pointer is exposed to the caller.

Fixes 10df039834f8 ("misc: fastrpc: Skip reference for DMA handles")
Cc [email protected]
Signed-off-by Zhenghang Xiao <[email protected]>
Signed-off-by Srinivas Kandagatla <[email protected]>
---
 drivers/misc/fastrpc.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)
drivers/misc/fastrpc.c +11 -14
index 47cf3d21b51d..f3a49384586d 100644
@@ -388,7 +388,7 @@ static int fastrpc_map_get(struct fastrpc_map *map)
 
 
 static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
-			    struct fastrpc_map **ppmap)
+			    struct fastrpc_map **ppmap, bool take_ref)
 {
 	struct fastrpc_map *map = NULL;
 	struct dma_buf *buf;

@@ -403,6 +403,12 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
 		if (map->fd != fd || map->buf != buf)
 			continue;
 
+		if (take_ref) {
+			ret = fastrpc_map_get(map);
+			if (ret)
+				break;
+		}
+
 		*ppmap = map;
 		ret = 0;
 		break;

@@ -920,19 +926,10 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, int fd,
 static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 			      u64 len, u32 attr, struct fastrpc_map **ppmap)
 {
-	struct fastrpc_session_ctx *sess = fl->sctx;
-	int err = 0;
-
-	if (!fastrpc_map_lookup(fl, fd, ppmap)) {
-		if (!fastrpc_map_get(*ppmap))
-			return 0;
-		dev_dbg(sess->dev, "%s: Failed to get map fd=%d\n",
-			__func__, fd);
-	}
-
-	err = fastrpc_map_attach(fl, fd, len, attr, ppmap);
+	if (!fastrpc_map_lookup(fl, fd, ppmap, true))
+		return 0;
 
-	return err;
+	return fastrpc_map_attach(fl, fd, len, attr, ppmap);
 }
 
 /*

@@ -1202,7 +1199,7 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx *ctx,
 	for (i = 0; i < FASTRPC_MAX_FDLIST; i++) {
 		if (!fdlist[i])
 			break;
-		if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap))
+		if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false))
 			fastrpc_map_put(mmap);
 	}
Signature · 1 line
2.53.0