Re: [PATCH v9] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe

[sashiko-bot@kernel.org]

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

New issues:
- [High] Race condition and logic flaw in clearing `cctx->audio_init_mem` allows unsafe double-initialization of the DSP heap.
- [High] Concurrent execution of `FASTRPC_IOCTL_INIT_CREATE_STATIC` and `fastrpc_rpmsg_remove` causes Use-After-Free on `cctx->remote_heap`.

Pre-existing issues:
- [High] Permanent SCM memory leak from HLOS on probe error path.
- [High] SDSP domain SCM memory leak on driver unbind due to missing `cctx->remote_heap` allocation.
- [High] Memory assigned via `ADSP_MMAP_REMOTE_HEAP_ADDR` is freed without restoring HLOS ownership.
- [High] Misc device is registered and exposed to userspace before its channel context is fully initialized.
--

--- Patch [1]: [PATCH v9] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe ---
commit a011acdb04cd3c4edb5a740628e2c5ac611ab957
Author: Jianping Li <jianping.li@oss.qualcomm.com>

misc: fastrpc: Allocate entire reserved memory for Audio PD in probe
    
Allocating and freeing Audio PD memory from userspace is unsafe because
the kernel cannot reliably determine when the DSP has finished using the
memory. This allocates the entire Audio PD reserved-memory region during
rpmsg probe and ties its lifetime to the rpmsg channel.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d86e79134c681..1ec37e6c68a28 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1344,15 +1346,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) {

[Severity: High]
Can this code race with fastrpc_rpmsg_remove()?

If the rpmsg channel is removed via driver unbind or DSP crash,
fastrpc_rpmsg_remove() frees cctx->remote_heap. 

Since these pointers are dereferenced here without taking cctx->lock, could
this lead to a use-after-free if FASTRPC_IOCTL_INIT_CREATE_STATIC is called
concurrently?

> +		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);

[ ... ]

>  	return 0;
>  err_invoke:
> +	cctx->audio_init_mem = false;

[Severity: High]
Does this code unconditionally clear the initialization flag on any failure?

If fastrpc_internal_invoke() fails for any caller (like a timeout or bad
argument), this sets cctx->audio_init_mem to false without checking if the
current caller was the one that successfully set it to true.

Could this allow a subsequent caller to re-send the heap coordinates, leading
to an unsafe double-initialization of the DSP heap?

>  	kfree(name);
>  err:

[ ... ]

> @@ -2418,12 +2400,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);

[Severity: High]
This is a pre-existing issue, but does this allocation strictly for ADSP
cause an SDSP memory leak?

Later in fastrpc_rpmsg_remove(), the newly added HLOS unassignment logic is
guarded by:

    if (cctx->remote_heap && cctx->vmcount)

Since data->remote_heap is never allocated for SDSP, it will be NULL and
the SCM unassignment will be silently skipped, permanently leaking the SDSP
reserved memory to the DSP on unbind.

> +				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);

[ ... ]

> @@ -2490,6 +2482,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]
This is a pre-existing issue, but does this error path permanently leak
TrustZone memory?

If a subsequent step in fastrpc_rpmsg_probe() fails (such as
fastrpc_device_register() or of_platform_populate()), the code jumps here
to free the structures.

However, qcom_scm_assign_mem() was already called to transfer ownership to
the DSP VMIDs, and there is no reverse call here to restore memory ownership
back to HLOS.

[Severity: High]
This is a pre-existing issue, but does fastrpc_rpmsg_probe() expose the
misc device before initialization is complete?

Looking at fastrpc_rpmsg_probe():

    err = fastrpc_device_register(rdev, data, secure_dsp, domain);
    [ ... ]
    kref_init(&data->refcount);
    [ ... ]
    INIT_LIST_HEAD(&data->users);
    [ ... ]
    spin_lock_init(&data->lock);
    idr_init(&data->ctx_idr);

fastrpc_device_register() makes the file node instantly visible to userspace.
If a user process opens it immediately, could it access uninitialized
data structures like data->lock or data->users in fastrpc_device_open()?

[Severity: High]
This is a pre-existing issue, but does fastrpc_req_munmap_impl() return memory
to the kernel allocator while it is still owned by TrustZone?

When fastrpc_req_mmap() handles ADSP_MMAP_REMOTE_HEAP_ADDR, it assigns the
memory to the DSP:

    err = qcom_scm_assign_mem(buf->dma_addr, (u64)buf->size,
        &src_perms, fl->cctx->vmperms, fl->cctx->vmcount);

However, the unmap path in fastrpc_req_munmap_impl() calls
fastrpc_buf_free() which runs dma_free_coherent(). It doesn't appear to
invoke qcom_scm_assign_mem() to revert permissions back to HLOS before
freeing.

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