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 <[email protected]> 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/[email protected]?part=3
