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