On 6/9/26 3:59 AM, Jianping Li wrote:
> 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: [email protected]
> Signed-off-by: Jianping Li <[email protected]>
> ---
> 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;
What is the expected behavoiur in this case?
> + }
> + 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;
this can race.
> 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);
>
