[PATCH v8 0/4] misc: fastrpc: Add missing bug fixes

[PATCH v8 0/4] misc: fastrpc: Add missing bug fixes

[Jianping Li]

Add missing bug fixes in memory areas. This patch series fixes multiple memory
handling issues in the FastRPC driver, primarily around the Audio PD remote heap.

The Audio PD uses a reserved memory-region that is shared between HLOS
and the DSP. Allocating and freeing this memory from userspace is unsafe,
as the kernel cannot reliably determine when the DSP has finished using
the buffers.

To address this, the entire reserved memory-region for the Audio PD is
now fully assigned to the DSP during remoteproc boot-up, and its lifetime
is tied to the rpmsg channel.

Patch [v7]: https://lore.kernel.org/all/20260602071750.526-1-jianping.li@oss.qualcomm.com/

Change in v8:
  - Squashed "Fail Audio PD init when reserved memory is missing" into
    "Allocate entire reserved memory for Audio PD in probe" as the
    validation check depends on the probe allocation
  - Fixed error path in probe: use goto err_free_data instead of bare
    return when kzalloc_obj fails
  - Added kfree(data->remote_heap) in err_free_data path
  - Made kfree(cctx->remote_heap) unconditional in rpmsg_remove,
    not tied to vmcount or qcom_scm_assign_mem result
  - Used local cctx variable consistently instead of fl->cctx

Change in v7:
  - Removed duplicate remote heap validation check; keep it only at
    the beginning of fastrpc_init_create_static_process()

Change in v6:
  - Separate the handling of err_copy
  - Place the check for remote_heap at the beginning of the function

Change in v5:
  - Split reserved-memory handling into separate patches

Change in v4:
  - Fail Audio PD static process creation when no reserved memory-region
    is present, instead of silently proceeding

Change in v3:
  - Adjusted the order of the series, placing NULL check changes that are not bug fixes at the end
  - Modified the commit message to describe the bug background in detail
  - Switch buf->list_lock back to fl->lock
  - Add locking to the operation of audio_init_mem

Changes in v2:
  - Remove the if check outside fastrpc_buf_free
  - Store the spinlock pointer in the struct fastrpc_buf instead
  - Allocate entire reserved memory to audio PD through remote heap

Ekansh Gupta (3):
  misc: fastrpc: Fix initial memory allocation for Audio PD memory pool
  misc: fastrpc: Remove buffer from list prior to unmap operation
  misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

Jianping Li (1):
  misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

 drivers/misc/fastrpc.c | 137 ++++++++++++++++++++++-------------------
 1 file changed, 75 insertions(+), 62 deletions(-)

-- 
2.43.0

[PATCH v8 1/4] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

The initial buffer allocated for the Audio PD memory pool is never added
to the pool because pageslen is set to 0. As a result, the buffer is not
registered with Audio PD and is never used, causing a memory leak. Audio
PD immediately falls back to allocating memory from the remote heap since
the pool starts out empty.

Fix this by setting pageslen to 1 so that the initially allocated buffer
is correctly registered and becomes part of the Audio PD memory pool.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a9b2ae44c06f..96961217b856 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1370,7 +1370,9 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		err = PTR_ERR(name);
 		goto err;
 	}
-
+	inbuf.client_id = fl->client_id;
+	inbuf.namelen = init.namelen;
+	inbuf.pageslen = 0;
 	if (!fl->cctx->remote_heap) {
 		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
 						&fl->cctx->remote_heap);
@@ -1393,12 +1395,10 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 				goto err_map;
 			}
 			scm_done = true;
+			inbuf.pageslen = 1;
 		}
 	}
 
-	inbuf.client_id = fl->client_id;
-	inbuf.namelen = init.namelen;
-	inbuf.pageslen = 0;
 	fl->pd = USER_PD;
 
 	args[0].ptr = (u64)(uintptr_t)&inbuf;
-- 
2.43.0

Re: [PATCH v8 1/4] misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `fastrpc_map_attach()` doubly detaches and frees `map->buf` when error checks fail after mapping.
- [High] Pending invocation contexts and associated DMA mappings are permanently leaked if the remote DSP crashes.
--

commit c836369bdd9f243155871c03517e477035418fb9
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Fix initial memory allocation for Audio PD memory pool

This commit fixes an issue where the initial buffer allocated for the
Audio PD memory pool was not registered because pageslen was 0. It updates
pageslen to 1 to ensure the buffer is correctly added to the pool.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a9b2ae44c06f6..96961217b8569 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does fastrpc_map_attach() doubly detach
and free map->buf when error checks fail after mapping?

If an error occurs after map->table is successfully assigned, the error
handling block runs:

fastrpc_map_attach() {
        ...
map_err:
        dma_buf_detach(map->buf, map->attach);
attach_err:
        dma_buf_put(map->buf);
get_err:
        fastrpc_map_put(map);
}

Since fastrpc_map_put() invokes fastrpc_free_map(), which also calls
dma_buf_detach() and dma_buf_put() if map->table is set, does this cause a
double free and reference count underflow?

[Severity: High]
This is a pre-existing issue, but does fastrpc_rpmsg_remove() permanently
leak pending invocation contexts and associated DMA mappings if the remote
DSP crashes?

When fastrpc_invoke_send() sends a message to the DSP, it takes a reference
on the context via fastrpc_context_get(ctx) to represent DSP ownership. If
the DSP crashes, fastrpc_rpmsg_remove() runs:

fastrpc_rpmsg_remove() {
        ...
        list_for_each_entry(user, &cctx->users, user)
                fastrpc_notify_users(user);
        ...
}

Since the rpmsg channel is removed, the callback will never fire. Does
fastrpc_notify_users() wake up waiting threads without dropping the DSP's
reference count, preventing the ctx reference count from ever reaching zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=1

[PATCH v8 2/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

fastrpc_req_munmap_impl() is called to unmap any buffer. The buffer is
getting removed from the list after it is unmapped from DSP. This can
create potential race conditions if multiple threads invoke unmap
concurrently, where one thread may remove the entry from the list while
another thread's unmap operation is still ongoing.

Fix this by removing the buffer entry from the list before calling the
unmap operation. If the unmap fails, the entry is re-added to the list
so that userspace can retry the unmap, or alternatively, the buffer
will be cleaned up during device release when the DSP process is torn
down and all DSP-side mappings are freed along with remaining buffers
in the list.

Fixes: 2419e55e532de ("misc: fastrpc: add mmap/unmap support")
Cc: stable@kernel.org
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 96961217b856..517884000331 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1889,9 +1889,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 				      &args[0]);
 	if (!err) {
 		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
-		spin_lock(&fl->lock);
-		list_del(&buf->node);
-		spin_unlock(&fl->lock);
 		fastrpc_buf_free(buf);
 	} else {
 		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);
@@ -1905,6 +1902,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	struct fastrpc_buf *buf = NULL, *iter, *b;
 	struct fastrpc_req_munmap req;
 	struct device *dev = fl->sctx->dev;
+	int err;
 
 	if (copy_from_user(&req, argp, sizeof(req)))
 		return -EFAULT;
@@ -1912,6 +1910,7 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 	spin_lock(&fl->lock);
 	list_for_each_entry_safe(iter, b, &fl->mmaps, node) {
 		if ((iter->raddr == req.vaddrout) && (iter->size == req.size)) {
+			list_del(&iter->node);
 			buf = iter;
 			break;
 		}
@@ -1924,7 +1923,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
 		return -EINVAL;
 	}
 
-	return fastrpc_req_munmap_impl(fl, buf);
+	err = fastrpc_req_munmap_impl(fl, buf);
+	if (err) {
+		spin_lock(&fl->lock);
+		list_add_tail(&buf->node, &fl->mmaps);
+		spin_unlock(&fl->lock);
+	}
+
+	return err;
 }
 
 static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
-- 
2.43.0

Re: [PATCH v8 2/4] misc: fastrpc: Remove buffer from list prior to unmap operation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] The patch introduces a critical use-after-free and list corruption vulnerability in the fastrpc_req_mmap() error path.

Pre-existing issues:
- [High] Concurrent accesses to fl->mmaps without holding fl->lock when handling -ERESTARTSYS.
--

commit a53abf279a47f6742d5cb44fb5a332592c4787b8
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Remove buffer from list prior to unmap operation

This commit removes the buffer from the fastrpc user's mapped list before
calling the unmap operation to prevent concurrent unmap races. If the
unmap operation fails, the buffer is re-added to the list.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 96961217b8569..517884000331b 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1889,9 +1889,6 @@ static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
>  				      &args[0]);
>  	if (!err) {
>  		dev_dbg(dev, "unmmap\tpt 0x%09lx OK\n", buf->raddr);
> -		spin_lock(&fl->lock);
> -		list_del(&buf->node);
> -		spin_unlock(&fl->lock);

[Severity: Critical]
Does removing this list_del() introduce a use-after-free in the error
path of fastrpc_req_mmap()?

When fastrpc_req_mmap() encounters a copy_to_user() failure after successfully
adding the buffer to the fl->mmaps list, it jumps to the err_assign label
and calls fastrpc_req_munmap_impl() without first unlinking the buffer:

drivers/misc/fastrpc.c:fastrpc_req_mmap() {
        ...
        spin_lock(&fl->lock);
        list_add_tail(&buf->node, &fl->mmaps);
        spin_unlock(&fl->lock);

        if (copy_to_user((void __user *)argp, &req, sizeof(req))) {
                err = -EFAULT;
                goto err_assign;
        }
        ...
err_assign:
        fastrpc_req_munmap_impl(fl, buf);

        return err;
}

Since fastrpc_req_munmap_impl() no longer removes the buffer from fl->mmaps
before calling fastrpc_buf_free(), the freed buffer will remain in the
list. Subsequent traverses of fl->mmaps will trigger a use-after-free.

>  		fastrpc_buf_free(buf);
>  	} else {
>  		dev_err(dev, "unmmap\tpt 0x%09lx ERROR\n", buf->raddr);

[ ... ]

> @@ -1924,7 +1923,14 @@ static int fastrpc_req_munmap(struct fastrpc_user *fl, char __user *argp)
>  		return -EINVAL;
>  	}
>  
> -	return fastrpc_req_munmap_impl(fl, buf);
> +	err = fastrpc_req_munmap_impl(fl, buf);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the mmap
list handling here, I noticed concurrent modifications to fl->mmaps happen
locklessly in fastrpc_internal_invoke() when handling -ERESTARTSYS:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
        ...
        if (err == -ERESTARTSYS) {
                list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
                        list_del(&buf->node);
                        list_add_tail(&buf->node, 
                                      &fl->cctx->invoke_interrupted_mmaps);
                }
        }
        ...
}

Can this lead to list corruption if another thread concurrently executes
fastrpc_req_mmap() or fastrpc_req_munmap() and modifies fl->mmaps with
fl->lock held?

> +	if (err) {
> +		spin_lock(&fl->lock);
> +		list_add_tail(&buf->node, &fl->mmaps);
> +		spin_unlock(&fl->lock);
> +	}
> +
> +	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=2

[PATCH v8 3/4] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[Jianping Li]

Allocating and freeing Audio PD memory from userspace is unsafe because
the kernel cannot reliably determine when the DSP has finished using the
memory. Userspace may free buffers while they are still in use by the DSP,
and remote free requests cannot be safely trusted.

Additionally, the current implementation allows userspace to repeatedly
grow the Audio PD heap, but does not support shrinking it. This can lead
to unbounded memory usage over time, effectively causing a memory leak.

Fix this by allocating the entire Audio PD reserved-memory region during
rpmsg probe and tying its lifetime to the rpmsg channel. This removes
userspace-controlled alloc/free and ensures that memory is reclaimed only
when the DSP process is torn down.

Add explicit validation for remote_heap presence and size before sending
the memory to DSP, and fail early if the reserved-memory region is
missing or incomplete.

Fixes: 0871561055e66 ("misc: fastrpc: Add support for audiopd")
Cc: stable@kernel.org
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 112 ++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 53 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 517884000331..1942e74535e5 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -276,6 +276,8 @@ struct fastrpc_channel_ctx {
 	struct kref refcount;
 	/* Flag if dsp attributes are cached */
 	bool valid_attributes;
+	/* Flag if audio PD init mem was allocated */
+	bool audio_init_mem;
 	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
 	struct fastrpc_device *secure_fdevice;
 	struct fastrpc_device *fdevice;
@@ -1341,15 +1343,24 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	struct fastrpc_init_create_static init;
 	struct fastrpc_invoke_args *args;
 	struct fastrpc_phy_page pages[1];
+	struct fastrpc_channel_ctx *cctx = fl->cctx;
 	char *name;
 	int err;
-	bool scm_done = false;
 	struct {
 		int client_id;
 		u32 namelen;
 		u32 pageslen;
 	} inbuf;
 	u32 sc;
+	unsigned long flags;
+
+	if (!cctx->remote_heap || !cctx->remote_heap->dma_addr ||
+	    !cctx->remote_heap->size) {
+		err = -ENOMEM;
+		dev_err(fl->sctx->dev,
+			"remote heap memory region is not added\n");
+		return err;
+	}
 
 	args = kzalloc_objs(*args, FASTRPC_CREATE_STATIC_PROCESS_NARGS);
 	if (!args)
@@ -1373,31 +1384,6 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	inbuf.client_id = fl->client_id;
 	inbuf.namelen = init.namelen;
 	inbuf.pageslen = 0;
-	if (!fl->cctx->remote_heap) {
-		err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
-						&fl->cctx->remote_heap);
-		if (err)
-			goto err_name;
-
-		/* Map if we have any heap VMIDs associated with this ADSP Static Process. */
-		if (fl->cctx->vmcount) {
-			u64 src_perms = BIT(QCOM_SCM_VMID_HLOS);
-
-			err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
-							(u64)fl->cctx->remote_heap->size,
-							&src_perms,
-							fl->cctx->vmperms, fl->cctx->vmcount);
-			if (err) {
-				dev_err(fl->sctx->dev,
-					"Failed to assign memory with dma_addr %pad size 0x%llx err %d\n",
-					&fl->cctx->remote_heap->dma_addr,
-					fl->cctx->remote_heap->size, err);
-				goto err_map;
-			}
-			scm_done = true;
-			inbuf.pageslen = 1;
-		}
-	}
 
 	fl->pd = USER_PD;
 
@@ -1409,8 +1395,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 	args[1].length = inbuf.namelen;
 	args[1].fd = -1;
 
-	pages[0].addr = fl->cctx->remote_heap->dma_addr;
-	pages[0].size = fl->cctx->remote_heap->size;
+	spin_lock_irqsave(&cctx->lock, flags);
+	if (!cctx->audio_init_mem) {
+		pages[0].addr = cctx->remote_heap->dma_addr;
+		pages[0].size = cctx->remote_heap->size;
+		cctx->audio_init_mem = true;
+		inbuf.pageslen = 1;
+	} else {
+		pages[0].addr = 0;
+		pages[0].size = 0;
+	}
+	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	args[2].ptr = (u64)(uintptr_t) pages;
 	args[2].length = sizeof(*pages);
@@ -1428,27 +1423,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 
 	return 0;
 err_invoke:
-	if (fl->cctx->vmcount && scm_done) {
-		u64 src_perms = 0;
-		struct qcom_scm_vmperm dst_perms;
-		u32 i;
-
-		for (i = 0; i < fl->cctx->vmcount; i++)
-			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
-
-		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
-		dst_perms.perm = QCOM_SCM_PERM_RWX;
-		err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
-						(u64)fl->cctx->remote_heap->size,
-						&src_perms, &dst_perms, 1);
-		if (err)
-			dev_err(fl->sctx->dev, "Failed to assign memory dma_addr %pad size 0x%llx err %d\n",
-				&fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
-	}
-err_map:
-	fastrpc_buf_free(fl->cctx->remote_heap);
-	fl->cctx->remote_heap = NULL;
-err_name:
+	cctx->audio_init_mem = false;
 	kfree(name);
 err:
 	kfree(args);
@@ -2415,12 +2390,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		}
 	}
 
-	if (domain_id == SDSP_DOMAIN_ID) {
+	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
 		struct resource res;
 		u64 src_perms;
 
 		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
 		if (!err) {
+			if (domain_id == ADSP_DOMAIN_ID) {
+				data->remote_heap =
+					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
+				if (!data->remote_heap) {
+					err = -ENOMEM;
+					goto err_free_data;
+				}
+
+				data->remote_heap->dma_addr = res.start;
+				data->remote_heap->size = resource_size(&res);
+			}
 			src_perms = BIT(QCOM_SCM_VMID_HLOS);
 
 			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
@@ -2428,7 +2414,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 			if (err)
 				goto err_free_data;
 		}
-
 	}
 
 	secure_dsp = !(of_property_read_bool(rdev->of_node, "qcom,non-secure-domain"));
@@ -2487,6 +2472,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		misc_deregister(&data->secure_fdevice->miscdev);
 
 err_free_data:
+	kfree(data->remote_heap);
 	kfree(data);
 	return err;
 }
@@ -2509,6 +2495,7 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	struct fastrpc_buf *buf, *b;
 	struct fastrpc_user *user;
 	unsigned long flags;
+	int err;
 
 	/* No invocations past this point */
 	spin_lock_irqsave(&cctx->lock, flags);
@@ -2526,8 +2513,27 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
 		list_del(&buf->node);
 
-	if (cctx->remote_heap)
-		fastrpc_buf_free(cctx->remote_heap);
+	if (cctx->remote_heap && cctx->vmcount) {
+		u64 src_perms = 0;
+		struct qcom_scm_vmperm dst_perms;
+
+		for (u32 i = 0; i < cctx->vmcount; i++)
+			src_perms |= BIT(cctx->vmperms[i].vmid);
+
+		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+		dst_perms.perm = QCOM_SCM_PERM_RWX;
+
+		err = qcom_scm_assign_mem(cctx->remote_heap->dma_addr,
+					  cctx->remote_heap->size, &src_perms,
+					  &dst_perms, 1);
+		if (err)
+			dev_err(&rpdev->dev,
+				"Failed to assign memory back to HLOS: dma_addr %pad size %#llx err %d\n",
+				&cctx->remote_heap->dma_addr, cctx->remote_heap->size, err);
+	}
+
+	kfree(cctx->remote_heap);
+	cctx->remote_heap = NULL;
 
 	of_platform_depopulate(&rpdev->dev);
 
-- 
2.43.0

Re: [PATCH v8 3/4] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unconditional call to `qcom_scm_assign_mem` during probe can result in an invalid TrustZone call or memory lockout if `data->vmcount` is 0.
- [High] Unconditional and lockless clearance of `cctx->audio_init_mem` on the static process error path corrupts initialization synchronization.
- [High] Removal of the dynamic heap fallback breaks Audio PD on older Device Trees.
- [High] Missing rollback of `qcom_scm_assign_mem` on probe failure permanently locks out SCM memory.

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition leading to a NULL pointer dereference of `cctx->remote_heap`.
--

commit 780b6a562518105022f75b2b13f38c9dbad812e3
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

This patch changes the Audio PD memory allocation to use the entire
reserved-memory region during rpmsg probe, tying its lifetime to the rpmsg
channel. It also adds explicit validation for the remote heap before sending
memory to the DSP.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 517884000331b..1942e74535e52 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -1341,15 +1343,24 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	struct fastrpc_init_create_static init;
>  	struct fastrpc_invoke_args *args;
>  	struct fastrpc_phy_page pages[1];
> +	struct fastrpc_channel_ctx *cctx = fl->cctx;
>  	char *name;
>  	int err;
>  	struct {
>  		int client_id;
>  		u32 namelen;
>  		u32 pageslen;
>  	} inbuf;
>  	u32 sc;
> +	unsigned long flags;
> +
> +	if (!cctx->remote_heap || !cctx->remote_heap->dma_addr ||
> +	    !cctx->remote_heap->size) {
> +		err = -ENOMEM;
> +		dev_err(fl->sctx->dev,
> +			"remote heap memory region is not added\n");
> +		return err;
> +	}

[Severity: High]
Does the removal of the dynamic heap fallback break backwards compatibility?

In the original code, if the device tree lacked a memory-region, the heap was
dynamically allocated via fastrpc_remote_heap_alloc(). Now, platforms with
older device trees will see FASTRPC_IOCTL_INIT_CREATE_STATIC fail with
-ENOMEM.

[ ... ]

> @@ -1409,8 +1395,17 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  	args[1].length = inbuf.namelen;
>  	args[1].fd = -1;
>  
> -	pages[0].addr = fl->cctx->remote_heap->dma_addr;
> -	pages[0].size = fl->cctx->remote_heap->size;
> +	spin_lock_irqsave(&cctx->lock, flags);
> +	if (!cctx->audio_init_mem) {
> +		pages[0].addr = cctx->remote_heap->dma_addr;
> +		pages[0].size = cctx->remote_heap->size;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference?

In fastrpc_init_create_static_process(), cctx->remote_heap is checked
locklessly at the start of the function. If a concurrent
fastrpc_rpmsg_remove() executes during a sleeping operation like
copy_from_user():

CPU1
fastrpc_rpmsg_remove()
    ...
    kfree(cctx->remote_heap);
    cctx->remote_heap = NULL;

Will CPU2's unconditional dereference of cctx->remote_heap->dma_addr inside
cctx->lock cause a kernel panic?

> +		cctx->audio_init_mem = true;
> +		inbuf.pageslen = 1;
> +	} else {
> +		pages[0].addr = 0;
> +		pages[0].size = 0;
> +	}
> +	spin_unlock_irqrestore(&cctx->lock, flags);

[ ... ]

> @@ -1428,27 +1423,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
>  
>  	return 0;
>  err_invoke:
> -	if (fl->cctx->vmcount && scm_done) {
> -		u64 src_perms = 0;
> -		struct qcom_scm_vmperm dst_perms;
> -		u32 i;
> -
> -		for (i = 0; i < fl->cctx->vmcount; i++)
> -			src_perms |= BIT(fl->cctx->vmperms[i].vmid);
> -
> -		dst_perms.vmid = QCOM_SCM_VMID_HLOS;
> -		dst_perms.perm = QCOM_SCM_PERM_RWX;
> -		err = qcom_scm_assign_mem(fl->cctx->remote_heap->dma_addr,
> -						(u64)fl->cctx->remote_heap->size,
> -						&src_perms, &dst_perms, 1);
> -		if (err)
> -			dev_err(fl->sctx->dev, "Failed to assign memory dma_addr %pad size 0x%llx err %d\n",
> -				&fl->cctx->remote_heap->dma_addr, fl->cctx->remote_heap->size, err);
> -	}
> -err_map:
> -	fastrpc_buf_free(fl->cctx->remote_heap);
> -	fl->cctx->remote_heap = NULL;
> -err_name:
> +	cctx->audio_init_mem = false;

[Severity: High]
Can this lockless assignment cause a data race that corrupts initialization
synchronization?

If Thread A successfully sets cctx->audio_init_mem = true inside cctx->lock
and proceeds, and Thread B concurrently executes the ioctl, skips
initialization because the flag is true, but then fails its invoke operation,
Thread B will unconditionally clear the flag for the entire channel. Does
this break the single-initialization guarantee?

>  	kfree(name);
>  err:
>  	kfree(args);

[ ... ]

> @@ -2415,12 +2390,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		}
>  	}
>  
> -	if (domain_id == SDSP_DOMAIN_ID) {
> +	if (domain_id == SDSP_DOMAIN_ID || domain_id == ADSP_DOMAIN_ID) {
>  		struct resource res;
>  		u64 src_perms;
>  
>  		err = of_reserved_mem_region_to_resource(rdev->of_node, 0, &res);
>  		if (!err) {
> +			if (domain_id == ADSP_DOMAIN_ID) {
> +				data->remote_heap =
> +					kzalloc_obj(*data->remote_heap, GFP_KERNEL);
> +				if (!data->remote_heap) {
> +					err = -ENOMEM;
> +					goto err_free_data;
> +				}
> +
> +				data->remote_heap->dma_addr = res.start;
> +				data->remote_heap->size = resource_size(&res);
> +			}
>  			src_perms = BIT(QCOM_SCM_VMID_HLOS);
>  
>  			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
> @@ -2428,7 +2414,6 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  			if (err)
>  				goto err_free_data;

[Severity: High]
Is it safe to call qcom_scm_assign_mem() here without verifying
data->vmcount > 0?

If the device tree lacks the qcom,vmids property, data->vmcount is 0. Passing
a dest_cnt of 0 to TrustZone could cause the SMC call to fail or permanently
lock out the memory by stripping HLOS permissions, especially since
fastrpc_rpmsg_remove() skips restoring memory if cctx->vmcount == 0.

>  		}
>  	}

[ ... ]

> @@ -2487,6 +2472,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		misc_deregister(&data->secure_fdevice->miscdev);
>  
>  err_free_data:
> +	kfree(data->remote_heap);
>  	kfree(data);
>  	return err;
>  }

[Severity: High]
Is there a missing rollback for qcom_scm_assign_mem() on this error path?

If the initial assignment succeeds, but a subsequent probe step like
fastrpc_device_register() fails, the driver memory is freed but the
hypervisor mapping is not restored to QCOM_SCM_VMID_HLOS. Can this
permanently lock out the SCM memory and cause subsequent probes to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=3

[PATCH v8 4/4] misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

[Jianping Li]

From: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

Make fastrpc_buf_free() a no-op when passed a NULL pointer, allowing
callers to avoid open-coded NULL checks.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
Signed-off-by: Jianping Li <jianping.li@oss.qualcomm.com>
---
 drivers/misc/fastrpc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 1942e74535e5..b4912b443b98 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -424,6 +424,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
 
 static void fastrpc_buf_free(struct fastrpc_buf *buf)
 {
+	if (!buf)
+		return;
+
 	dma_free_coherent(buf->dev, buf->size, buf->virt,
 			  fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr));
 	kfree(buf);
@@ -516,8 +519,7 @@ static void fastrpc_user_free(struct kref *ref)
 	struct fastrpc_map *map, *m;
 	struct fastrpc_buf *buf, *b;
 
-	if (fl->init_mem)
-		fastrpc_buf_free(fl->init_mem);
+	fastrpc_buf_free(fl->init_mem);
 
 	list_for_each_entry_safe(ctx, n, &fl->pending, node) {
 		list_del(&ctx->node);
@@ -562,8 +564,7 @@ static void fastrpc_context_free(struct kref *ref)
 	for (i = 0; i < ctx->nbufs; i++)
 		fastrpc_map_put(ctx->maps[i]);
 
-	if (ctx->buf)
-		fastrpc_buf_free(ctx->buf);
+	fastrpc_buf_free(ctx->buf);
 
 	spin_lock_irqsave(&cctx->lock, flags);
 	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);
-- 
2.43.0

Re: [PATCH v8 4/4] misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Exported DMA buffers outlive their fastrpc_user context, leading to a Use-After-Free on dmabuf operations.
- [Critical] In `fastrpc_req_mmap()`, if `copy_to_user()` fails, the allocated buffer is freed via `fastrpc_req_munmap_impl()`, but its node is never removed from `fl->mmaps`.
- [Critical] Timeouts in kernel RPCs cause DMA buffers to be freed while the DSP is still actively using them.
- [High] Interrupted mmaps are unlinked but their memory is never freed during `fastrpc_rpmsg_remove()`.
- [High] Invocation contexts leak references to attached maps for handles, allowing unbounded resource accumulation.
- [High] Failure to restore hypervisor memory permissions in `fastrpc_req_munmap_impl` for remote heap allocations.
--

commit c345df439255945e9c8c222e251d798521775ff6
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

Make fastrpc_buf_free() a no-op when passed a NULL pointer, allowing
callers to avoid open-coded NULL checks.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 1942e74535e52..b4912b443b982 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -424,6 +424,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>  
>  static void fastrpc_buf_free(struct fastrpc_buf *buf)
>  {
> +	if (!buf)
> +		return;
> +
>  	dma_free_coherent(buf->dev, buf->size, buf->virt,
>  			  fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr));

[Severity: Critical]
This is a pre-existing issue, but does fastrpc_buf_free() have a
use-after-free risk when called from the dmabuf release path?

If a user closes the fastrpc device descriptor but keeps the DMA-BUF file
descriptor open, the fastrpc_user (fl) context is freed. When the DMA-BUF
is later released, it calls fastrpc_buf_free(), which dereferences
buf->fl->cctx here to calculate the DMA address.

Since fastrpc_dmabuf_alloc() doesn't take a reference on the fastrpc_user
when exporting the DMA-BUF, can this lead to memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but does fastrpc_buf_free()
safely handle buffers mapped with ADSP_MMAP_REMOTE_HEAP_ADDR?

When fastrpc_req_mmap() maps a remote heap buffer, it uses
qcom_scm_assign_mem() to grant access to the DSP VM. During unmapping,
fastrpc_req_munmap_impl() calls fastrpc_buf_free() directly.

Will handing this restricted memory back to the kernel's allocator without
first calling qcom_scm_assign_mem() to restore ownership to the HLOS lead
to a hypervisor fault when the kernel tries to use the memory?

[Severity: Critical]
This is a pre-existing issue, but does calling fastrpc_buf_free() on
timeout lead to memory corruption by the DSP?

When fastrpc_internal_invoke() times out, callers like fastrpc_req_mmap()
and fastrpc_init_create_process() treat it as a failure and immediately
call fastrpc_buf_free(buf) to release the kernel memory.

Since there is no mechanism to cancel the pending request on the DSP, won't
the DSP continue processing and access the freed DMA memory, potentially
corrupting memory that has been reallocated by the system?

>  	kfree(buf);
>  }
> @@ -516,8 +519,7 @@ static void fastrpc_user_free(struct kref *ref)
>  	struct fastrpc_map *map, *m;
>  	struct fastrpc_buf *buf, *b;
>  
> -	if (fl->init_mem)
> -		fastrpc_buf_free(fl->init_mem);
> +	fastrpc_buf_free(fl->init_mem);
>  
>  	list_for_each_entry_safe(ctx, n, &fl->pending, node) {

[Severity: Critical]
This isn't a bug introduced by this patch, but does fastrpc_user_free()
risk a use-after-free and double free when cleaning up fl->mmaps?

In fastrpc_req_mmap(), if copy_to_user() fails, the code jumps to
err_assign and calls fastrpc_req_munmap_impl(). This unmaps and frees the
buffer using fastrpc_buf_free(), but leaves its node in the fl->mmaps
list.

When fastrpc_user_free() later loops over fl->mmaps, will it attempt to
access and free this already-freed buffer?

[Severity: High]
This is a pre-existing issue, but does fastrpc_rpmsg_remove() leak
interrupted mmaps when tearing down the channel?

When an invocation returns -ERESTARTSYS, mapped buffers are moved from
fl->mmaps to cctx->invoke_interrupted_mmaps. During fastrpc_rpmsg_remove(),
the elements are unlinked:

    list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
        list_del(&buf->node);

Does skipping the fastrpc_buf_free(buf) call here cause a permanent leak
of the fastrpc_buf and its associated DMA/SMMU memory allocations?

>  		list_del(&ctx->node);
> @@ -562,8 +564,7 @@ static void fastrpc_context_free(struct kref *ref)
>  	for (i = 0; i < ctx->nbufs; i++)
>  		fastrpc_map_put(ctx->maps[i]);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does fastrpc_context_free()
leak references to attached maps for handles?

In fastrpc_create_maps(), the ctx->maps array is populated for all
nscalars. For indices >= nbufs (handles), fastrpc_map_attach() creates
a new map and adds it to fl->maps.

Since this cleanup loop only iterates up to ctx->nbufs, does it fail to
drop the references for the remaining handle maps in ctx->maps, causing
unbounded accumulation in fl->maps?

> -	if (ctx->buf)
> -		fastrpc_buf_free(ctx->buf);
> +	fastrpc_buf_free(ctx->buf);
>  
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=4