Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] accel/qda: race between qda_attach_audio_pd() and qda_rpmsg_remove() - [High] accel/qda: unallocated CMA memory hyp-assigned in qda_init_remote_heap() - [Medium] accel/qda: signal in qda_attach_audio_pd() desyncs audio_init_mem state - [Medium] accel/qda: 32-bit BIT() overflow and unvalidated VMIDs in remote heap setup - [Low] accel/qda: qda_attach_audio_pd() mixes goto cleanup with scoped_guard() - [Low] accel/qda: missing mutex_destroy() for remote_heap_lock on device teardown --
commit cd6c4ef6e7fc95f52a56e49a2e18e2d259cf294c Author: Ekansh Gupta <[email protected]> accel/qda: Add Audio protection domain attach and its remote heap This commit adds support for attaching to the static Audio protection domain by name via QDA_ATTACH_AUDIO_PD and hands the DSP its reserved remote heap memory region on the first attach. At probe time, it parses the qcom,vmids property and reserved-memory region and uses qcom_scm_assign_mem() to share the heap with the DSP VMs until the rpmsg device is removed. > diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c > index 9b8cccfea1e3a..41234fc576a9a 100644 > --- a/drivers/accel/qda/qda_ioctl.c > +++ b/drivers/accel/qda/qda_ioctl.c [ ... ] > @@ -176,6 +177,54 @@ int qda_release_dsp_process(struct qda_dev *qdev, struct > qda_session *session) > return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_RELEASE, NULL, session); > } > > +/* > + * The Audio PD keeps the remote heap across daemon restarts, so only the > first > + * attach on a device hands it to the DSP. The lock is held across the attach > + * so that a concurrent one cannot skip the heap while the first is still in > + * flight, and the flag is set only once the DSP has accepted it. > + */ > +static int qda_attach_audio_pd(struct qda_session *session, struct > drm_qda_remote_attach *args) > +{ > + struct qda_dev *qdev = session->qda_dev; > + struct qda_static_pd_args sargs = {}; > + char *name; > + int err; > + > + if (strcmp(qdev->dsp_name, "adsp") || !qdev->remote_heap_size) > + return -ENODEV; > + > + if (!args->namelen || args->namelen > QDA_FASTRPC_INIT_NAMELEN_MAX) > + return -EINVAL; > + > + name = memdup_user(u64_to_user_ptr(args->name), args->namelen); > + if (IS_ERR(name)) > + return PTR_ERR(name); > + > + if (name[args->namelen - 1]) { > + err = -EINVAL; > + goto out_free; > + } > + > + sargs.name = name; > + sargs.namelen = args->namelen; > + > + scoped_guard(mutex, &qdev->remote_heap_lock) { > + if (!qdev->audio_init_mem) { > + sargs.heap.addr = qdev->remote_heap_addr; > + sargs.heap.size = qdev->remote_heap_size; > + } > + > + err = qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_CREATE_STATIC, > &sargs, session); > + if (!err && sargs.heap.size) > + qdev->audio_init_mem = true; > + } [Severity: Medium] What happens if the calling task receives a signal while waiting for the DSP response in qda_attach_audio_pd()? The call chain: qda_attach_audio_pd() qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_CREATE_STATIC, &sargs, session) qda_fastrpc_invoke_common() qda_rpmsg_send_msg() qda_rpmsg_wait_for_rsp() wait_for_completion_interruptible(&ctx->work) waits interruptibly after the QDA_FASTRPC_RMID_INIT_CREATE_STATIC message carrying sargs.heap has already been transmitted to the DSP. If a signal arrives before the DSP replies, qda_fastrpc_invoke() returns -ERESTARTSYS while the DSP continues processing the remote heap initialization. Because err is non-zero, qdev->audio_init_mem remains false and qdev->remote_heap_lock is released, allowing a restarted syscall or a concurrent caller to observe !qdev->audio_init_mem and hand the remote heap to the DSP a second time while the first attach is still in flight or after the DSP has already accepted it. > + > +out_free: > + kfree(name); > + > + return err; > +} [Severity: Low] This isn't a bug, but qda_attach_audio_pd() mixes goto out_free error handling for name with scoped_guard(mutex, &qdev->remote_heap_lock) in the same function. Could name be declared with __free(kfree) so that the NUL-termination check can return -EINVAL directly without mixing goto cleanup and scope-based cleanup helpers? > diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c > index 7e58df3eb2e17..b93e906b57930 100644 > --- a/drivers/accel/qda/qda_rpmsg.c > +++ b/drivers/accel/qda/qda_rpmsg.c [ ... ] > @@ -145,6 +147,81 @@ static int qda_rpmsg_cb(struct rpmsg_device *rpdev, void > *data, int len, > return ret; > } > > +static int qda_init_vmperms(struct qda_dev *qdev) > +{ > + u32 vmids[QDA_MAX_VMIDS]; > + int vmcount, i; > + > + vmcount = of_property_read_variable_u32_array(qdev->dev->of_node, > "qcom,vmids", > + vmids, 0, QDA_MAX_VMIDS); > + if (vmcount == -EINVAL || vmcount == -ENODATA) > + return 0; > + if (vmcount < 0) > + return vmcount; > + > + if (!qcom_scm_is_available()) > + return -EPROBE_DEFER; > + > + for (i = 0; i < vmcount; i++) { > + qdev->vmperms[i].vmid = vmids[i]; > + qdev->vmperms[i].perm = QCOM_SCM_PERM_RWX; > + } > + qdev->vmcount = vmcount; > + > + return 0; > +} > + > +/* > + * The region is optional: without it an Audio PD attach is refused. It is > + * recorded only once it has been assigned, so teardown hands back exactly > + * what was handed over. > + */ > +static int qda_init_remote_heap(struct qda_dev *qdev) > +{ > + u64 src_perms = BIT(QCOM_SCM_VMID_HLOS); > + struct resource res; > + int ret; > + > + if (strcmp(qdev->dsp_name, "adsp") && strcmp(qdev->dsp_name, "sdsp")) > + return 0; > + > + if (of_reserved_mem_region_to_resource(qdev->dev->of_node, 0, &res)) > + return 0; > + > + if (qdev->vmcount) { > + ret = qcom_scm_assign_mem(res.start, resource_size(&res), > &src_perms, > + qdev->vmperms, qdev->vmcount); > + if (ret) > + return ret; > + } > + > + qdev->remote_heap_addr = res.start; > + qdev->remote_heap_size = resource_size(&res); > + > + return 0; > +} [Severity: High] Can qda_init_remote_heap() hyp-assign memory away from HLOS without first allocating it from CMA? In the Qualcomm FastRPC device tree bindings and upstream SoC device trees, the ADSP remote heap memory-region (adsp_rpc_remote_heap_mem) is defined as a reusable shared-dma-pool without no-map. For a reusable CMA region, those pages remain in the Linux page allocator for movable HLOS allocations until allocated via CMA or DMA APIs. Taking the raw physical range from of_reserved_mem_region_to_resource() in qda_init_remote_heap() and revoking QCOM_SCM_VMID_HLOS access via qcom_scm_assign_mem() without allocating the pages first leaves active or future movable HLOS pages inside that range, which will trigger stage-2 permission faults when accessed by the CPU and corrupt memory when used by the ADSP. > + > +static void qda_deinit_remote_heap(struct qda_dev *qdev) > +{ > + struct qcom_scm_vmperm dst_perms = { > + .vmid = QCOM_SCM_VMID_HLOS, > + .perm = QCOM_SCM_PERM_RWX, > + }; > + u64 src_perms = 0; > + int ret, i; > + > + if (!qdev->remote_heap_size || !qdev->vmcount) > + return; > + > + for (i = 0; i < qdev->vmcount; i++) > + src_perms |= BIT(qdev->vmperms[i].vmid); [Severity: Medium] Could BIT(qdev->vmperms[i].vmid) overflow a 32-bit unsigned long or shift past 63? In qda_deinit_remote_heap() (and in qda_init_remote_heap() when initializing src_perms), BIT() uses 1UL << nr, whereas src_perms is a u64 and ADSP remote heap VMIDs such as QCOM_SCM_VMID_ADSP_HEAP (0x25) or QCOM_SCM_VMID_CP_ADSP_SHARED (0x33) exceed 31. On 32-bit builds (allowed via ARCH_QCOM || COMPILE_TEST), shifting 1UL by 37 or 51 is undefined behavior and fails to populate the upper 32 bits of src_perms. In addition, qda_init_vmperms() copies u32 values from the qcom,vmids property without checking that each vmid is less than BITS_PER_TYPE(u64). Should qda_init_remote_heap() and qda_deinit_remote_heap() use BIT_ULL(), and should qda_init_vmperms() reject VMIDs >= BITS_PER_TYPE(u64)? > + > + ret = qcom_scm_assign_mem(qdev->remote_heap_addr, > qdev->remote_heap_size, > + &src_perms, &dst_perms, 1); > + if (ret) > + dev_err(qdev->dev, "Failed to assign remote heap back to HLOS: > %d\n", ret); > +} > + > static void qda_rpmsg_remove(struct rpmsg_device *rpdev) > { > struct qda_dev *qdev = dev_get_drvdata(&rpdev->dev); > @@ -163,6 +240,7 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev) > qda_fastrpc_flush_pending(qdev); > qda_cb_unpopulate(qdev); > qda_deinit_device(qdev); > + qda_deinit_remote_heap(qdev); > } [Severity: High] Can qda_attach_audio_pd() race with qda_rpmsg_remove() and access destroyed device state after teardown? If thread A is inside qda_attach_audio_pd() holding qdev->remote_heap_lock while waiting for the DSP response, a concurrent thread B calling qda_ioctl_attach() -> qda_attach_audio_pd() can already be blocked waiting to acquire qdev->remote_heap_lock before allocating an invoke context in qdev->ctx_xa. When qda_rpmsg_remove() runs: qda_rpmsg_remove() drm_dev_unplug(&qdev->drm_dev) qdev->rpdev = NULL qda_fastrpc_flush_pending(qdev) qda_cb_unpopulate(qdev) qda_deinit_device(qdev) qda_deinit_remote_heap(qdev) drm_dev_unplug() does not wait for threads already inside drm_ioctl(), and qda_fastrpc_flush_pending() only wakes thread A. While thread A releases qdev->remote_heap_lock and thread B acquires it, qda_rpmsg_remove() proceeds to destroy the context bank platform devices in qda_cb_unpopulate() and free qdev->iommu_mgr and destroy qdev->ctx_xa in qda_deinit_device(). Thread B then calls qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_CREATE_STATIC, ...) without checking whether the device was unplugged, executing qda_fastrpc_context_alloc() on the destroyed qdev->ctx_xa and qda_fastrpc_prepare_args_create_static() -> alloc_kernel_gem() -> qda_gem_create_object() -> qda_memory_manager_alloc() on the destroyed iommu_mgr and freed context bank device before reaching the drm_dev_enter() check in qda_rpmsg_send_msg(). > @@ -195,10 +273,20 @@ static int qda_rpmsg_probe(struct rpmsg_device *rpdev) > return ret; > } > > - ret = qda_init_device(qdev, qda_count_cb_nodes(rpdev->dev.of_node)); > + mutex_init(&qdev->remote_heap_lock); [Severity: Low] Unlike qdev->import_lock, which is initialized in qda_alloc_device() and destroyed in qda_deinit_device(), qdev->remote_heap_lock is initialized here in qda_rpmsg_probe() and never passed to mutex_destroy() on probe failure or in qda_deinit_device(). Should remote_heap_lock be initialized and destroyed alongside import_lock? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=21
