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

[sashiko-bot@kernel.org]

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