Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues: - [Critical] Unconditional dereference of `cctx->remote_heap` causes a kernel panic when `FASTRPC_IOCTL_INIT_CREATE_STATIC` is invoked on non-ADSP domains or concurrently with device removal. - [High] Missing rollback of hypervisor SCM memory assignment in the `fastrpc_rpmsg_probe` error path permanently leaks HLOS memory access permissions. - [High] Mandating the `memory-region` property for the ADSP domain breaks backward compatibility with older Device Trees. - [High] Probe unconditionally calls qcom_scm_assign_mem with a potentially zero vmcount, which may cause probe failures or unrecoverable permission drops. - [High] Concurrent static process creations can leave the DSP process initialized without a remote heap due to a race condition. Pre-existing issues: - [High] Memory access permissions assigned away from HLOS for the SDSP domain are never restored during teardown. -- --- Patch [1]: [PATCH v10] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe --- commit d3151c13cc6203a8d9909ed1502893583ed1fe94 Author: Jianping Li <[email protected]> misc: fastrpc: Allocate entire reserved memory for Audio PD in probe This commit shifts the allocation of the entire Audio PD reserved-memory region to the rpmsg probe phase and ties its lifetime to the rpmsg channel. This aims to prevent memory leaks from unbounded userspace heap requests and ensures proper memory reclamation. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d86e79134c681..03f633028c007 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1391,8 +1391,25 @@ 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; > + /* > + * Audio PD is a static PD and retains the remote heap > + * information across daemon restarts. Therefore only > + * the first attach should provide heap information to > + * DSP. Subsequent attaches reuse the previously > + * initialized memory pool. > + */ > + 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; > + sent_heap = true; [Severity: Critical] Does this unconditional dereference of cctx->remote_heap cause a NULL pointer dereference for non-ADSP domains? Looking at fastrpc_rpmsg_probe(), remote_heap is only allocated for the ADSP_DOMAIN_ID. If a user with access to a non-ADSP fastrpc device node (like /dev/fastrpc-cdsp) invokes the FASTRPC_IOCTL_INIT_CREATE_STATIC ioctl, cctx->remote_heap will be NULL. Additionally, could a concurrent device removal free cctx->remote_heap and set it to NULL while this ioctl is executing, leading to a use-after-free or NULL pointer dereference? [Severity: High] Can concurrent static process creations leave the DSP process initialized without a remote heap due to a race condition? If two threads call this ioctl concurrently, Thread A might set cctx->audio_init_mem to true and proceed with the heap address. Thread B would see the flag as true and proceed with a zeroed address. If Thread A's remote invocation fails and it rolls back cctx->audio_init_mem to false in the err_invoke path below, Thread B's invocation may have already succeeded on the DSP without providing the required heap memory. > + } 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); [ ... ] > @@ -1427,27 +1427,11 @@ static int fastrpc_init_create_static_process(struct > fastrpc_user *fl, > > return 0; > err_invoke: > - if (fl->cctx->vmcount && scm_done) { [ ... ] > + if (sent_heap) { > + spin_lock_irqsave(&cctx->lock, flags); > + cctx->audio_init_mem = false; > + spin_unlock_irqrestore(&cctx->lock, flags); > } > -err_map: > - fastrpc_buf_free(fl->cctx->remote_heap); > - fl->cctx->remote_heap = NULL; > -err_name: > kfree(name); > err: > kfree(args); [ ... ] > @@ -2418,12 +2398,29 @@ 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 && domain_id == ADSP_DOMAIN_ID) { > + dev_err(rdev, "missing mandatory remote heap > memory-region\n"); > + goto err_free_data; > + } > + [Severity: High] Does mandating the memory-region property for the ADSP domain break backward compatibility with older device trees? Previously, if a device tree did not specify a reserved memory region for ADSP, the driver dynamically allocated the remote heap. By explicitly failing the probe when of_reserved_mem_region_to_resource() fails for ADSP_DOMAIN_ID, older device trees might now fail to probe. > if (!err) { > + if (domain_id == ADSP_DOMAIN_ID) { > + data->remote_heap = > + kzalloc_obj(*data->remote_heap); > + 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, > data->vmperms, data->vmcount); [Severity: High] Is it safe to call qcom_scm_assign_mem() unconditionally here without verifying if data->vmcount is greater than zero? If the qcom,vmids property is omitted in the device tree, vmcount will be 0. Calling qcom_scm_assign_mem() with 0 destinations might cause a hypervisor error or silently drop permissions. In the previous implementation, this was guarded by checking if fl->cctx->vmcount was non-zero. > if (err) > goto err_free_data; > } [ ... ] > @@ -2490,6 +2486,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] Does the err_free_data path leak HLOS memory access permissions? If qcom_scm_assign_mem() successfully transfers memory permissions to the DSP during fastrpc_rpmsg_probe(), but a subsequent step fails, the code jumps to err_free_data. The error path frees the allocated memory structures but appears to miss calling qcom_scm_assign_mem() to revert the permissions back to HLOS. [ ... ] > @@ -2529,8 +2527,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; [Severity: High] This is a pre-existing issue, but does the cleanup path skip restoring memory access permissions for the SDSP domain? During fastrpc_rpmsg_probe(), memory access permissions are transferred to the DSP for both SDSP_DOMAIN_ID and ADSP_DOMAIN_ID. However, during teardown here, the permission restoration is explicitly guarded by checking if cctx->remote_heap is populated. Since remote_heap is only allocated for ADSP, it remains NULL for SDSP, skipping the restoration logic entirely and potentially leaking the permissions. > + > + for (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); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
