Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [High] TrustZone memory assignment is permanently leaked on probe error paths. - [High] Concurrent ioctl calls can cause the DSP to receive an uninitialized static process creation request out-of-order. - [Medium] Unconditional TrustZone memory assignment during probe may fail or orphan memory for ADSP devices lacking VMIDs. - [Medium] The `audio_init_mem` flag is incorrectly rolled back on remote invocation errors, allowing the static heap to be sent multiple times. Pre-existing issues: - [High] Permanent TrustZone memory leak for SDSP domains during driver removal. -- --- Patch [1]: [PATCH v12] misc: fastrpc: Allocate entire reserved memory for Audio PD in probe --- commit f78e580b5ae96a440902b976778bd1f19b384768 Author: Jianping Li <[email protected]> misc: fastrpc: Allocate entire reserved memory for Audio PD in probe This commit allocates the entire Audio PD reserved-memory region during rpmsg probe and ties its lifetime to the rpmsg channel. It removes userspace-controlled allocation and freeing, ensuring memory is only reclaimed when the DSP process is torn down. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 90fd669636ec1..3f14a4673698e 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1508,17 +1508,34 @@ 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_addr; > + pages[0].size = cctx->remote_heap_size; > + cctx->audio_init_mem = true; > + inbuf.pageslen = 1; > + sent_heap = true; > + } 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); > args[2].fd = -1; > > sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_CREATE_STATIC, 3, 0); > > err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, > sc, args); [Severity: High] Could this allow the DSP to receive an uninitialized static process creation request out-of-order? Two threads could concurrently execute the ioctl. Thread A acquires the lock, sees audio_init_mem is false, sets it to true, and sets pageslen = 1. Thread B then acquires the lock, sees audio_init_mem is true, and sets pageslen = 0. Because fastrpc_internal_invoke() is called outside the lock, thread execution order is unconstrained once the lock is released. Thread B could be scheduled such that its RPC payload (with pageslen = 0) reaches the DSP before Thread A's initialization payload. > if (err) > goto err_invoke; > > kfree(args); > kfree(name); > > 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: [Severity: Medium] Is it safe to blindly roll back audio_init_mem if fastrpc_internal_invoke() returns an error? Because the DSP operates asynchronously, an interrupted kernel wait (such as -ERESTARTSYS due to a received signal) does not necessarily mean the DSP failed to process the request. Reverting this flag might allow the next process attachment to send the heap configuration pages a second time, which would violate the static process lifecycle. [ ... ] > @@ -2564,25 +2564,20 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) [ ... ] > - if (domain_id == SDSP_DOMAIN_ID) { > + if (fastrpc_domain_has_reserved_heap(domain_id)) { > struct resource res; > u64 src_perms; > > err = of_reserved_mem_region_to_resource(rdev->of_node, 0, > &res); > if (!err) { > + if (fastrpc_domain_uses_static_heap(domain_id)) { > + data->remote_heap_addr = res.start; > + data->remote_heap_size = resource_size(&res); > + } [Severity: High] This is a pre-existing issue, but does this logic permanently leak TrustZone memory for SDSP domains when the driver is removed? For SDSP domains, fastrpc_domain_has_reserved_heap() returns true, so memory is assigned to the DSP VMID below. However, fastrpc_domain_uses_static_heap() returns false for SDSP, so data->remote_heap_size remains uninitialized as 0. > src_perms = BIT(QCOM_SCM_VMID_HLOS); > > err = qcom_scm_assign_mem(res.start, > resource_size(&res), &src_perms, > data->vmperms, data->vmcount); [Severity: Medium] Is it intentional to call qcom_scm_assign_mem() unconditionally here, even if data->vmcount is zero? Previously, this assignment was explicitly guarded by checking vmcount. If a device tree without the qcom,vmids property initializes the ADSP domain, could calling this with a destination count of 0 be rejected by TrustZone and fail the probe? Additionally, if vmcount is 0, the memory is explicitly skipped during cleanup, which would permanently leak it. > if (err) > goto err_free_data; > + } else { > + err = 0; > } > - > } [ ... ] > err_free_data: > kfree(data); > return err; > } [Severity: High] Does this error path permanently leak the TrustZone memory assignment? If any operation fails after the memory is successfully assigned to the DSP VMID (for example, inside fastrpc_device_register() or of_platform_populate()), the error handlers only free local data structures. They do not appear to call qcom_scm_assign_mem() to revert the TrustZone memory ownership back to HLOS. [ ... ] > @@ -2684,24 +2684,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device > *rpdev) [ ... ] > - if (cctx->remote_heap) > - fastrpc_buf_free(cctx->remote_heap); > + if (cctx->remote_heap_size && cctx->vmcount) { > + u64 src_perms = 0; > + struct qcom_scm_vmperm dst_perms; [Severity: High] This is a pre-existing issue, but does this condition permanently bypass memory unassignment for SDSP domains? Because remote_heap_size is intentionally left as 0 for SDSP domains during probe, this cleanup block is skipped. This leaves the reserved memory permanently assigned to the DSP VMID after module removal. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
