Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] accel/qda: race between qda_session_attach() and qda_session_init_create() - [High] accel/qda: use-after-free in qda_session_attach() during rpmsg removal - [Medium] accel/qda: interrupted wait in qda_attach_audio_pd() resends remote heap --
commit ae4f1060dc81ff84d55ddf1c5fd82d79333077f8 Author: Ekansh Gupta <[email protected]> accel/qda: Add remote session attach to the legacy shim This commit wires FASTRPC_IOCTL_INIT_ATTACH, FASTRPC_IOCTL_INIT_ATTACH_SNS, and FASTRPC_IOCTL_INIT_CREATE_STATIC into the legacy FastRPC compatibility shim by extracting qda_session_attach() and exposing it via qda_legacy_ops_table. > diff --git a/drivers/accel/qda/qda_fastrpc_shim.c > b/drivers/accel/qda/qda_fastrpc_shim.c > index c46d2de4f5818..c1464b1adbc2c 100644 > --- a/drivers/accel/qda/qda_fastrpc_shim.c > +++ b/drivers/accel/qda/qda_fastrpc_shim.c [ ... ] > @@ -295,6 +296,23 @@ static long qda_shim_init_create(struct qda_shim_file > *sf, void __user *argp) > return err; > } > > +static long qda_shim_attach(struct qda_shim_file *sf, u32 pd_type) > +{ > + return qda_legacy_ops_table.attach(sf->session, pd_type, NULL, 0); > +} > + > +/* memlen is ignored: the Audio PD heap size comes from the device tree */ > +static long qda_shim_init_create_static(struct qda_shim_file *sf, void > __user *argp) > +{ > + struct fastrpc_init_create_static args; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + return qda_legacy_ops_table.attach(sf->session, QDA_ATTACH_AUDIO_PD, > + u64_to_user_ptr(args.name), > args.namelen); > +} [Severity: High] Can qda_shim_attach() and qda_shim_init_create_static() (as well as qda_ioctl_attach()) trigger a use-after-free if the underlying rpmsg device is removed while an attach ioctl is in flight? Both qda_shim_ioctl() and drm_ioctl() only perform a point-in-time drm_dev_is_unplugged() check on entry without entering a drm_dev_enter() critical section: drivers/accel/qda/qda_fastrpc_shim.c:qda_shim_ioctl() { if (drm_dev_is_unplugged(&sf->session->qda_dev->drm_dev)) return -ENODEV; return qda_shim_dispatch(sf, cmd, (void __user *)arg); } If qda_rpmsg_remove() runs after that check passes (for example while qda_shim_init_create_static() or qda_attach_audio_pd() sleeps in copy_from_user() or memdup_user()), drm_dev_unplug() does not wait for the in-flight qda_session_attach() call before tearing down the device: drivers/accel/qda/qda_rpmsg.c: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); } qda_deinit_device() destroys qdev->iommu_mgr->assignment_lock and qdev->ctx_xa, and devres teardown on &rpdev->dev frees qdev->iommu_mgr (allocated with devm_kzalloc(qdev->dev, ...) in qda_init_device()). When the in-flight qda_session_attach() proceeds into: qda_fastrpc_invoke() -> qda_fastrpc_context_alloc() -> qda_fastrpc_prepare_args() -> alloc_kernel_gem() -> qda_gem_create_object() -> qda_memory_manager_alloc() -> qda_memory_manager_assign_device() wouldn't it access the destroyed qdev->ctx_xa and dereference the freed qdev->iommu_mgr (or have its newly allocated ctx freed early if qda_fastrpc_flush_pending() runs between qda_fastrpc_context_alloc() and qda_rpmsg_send_msg())? > diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c > index 41234fc576a9a..aaf3e5e6b5b18 100644 > --- a/drivers/accel/qda/qda_ioctl.c > +++ b/drivers/accel/qda/qda_ioctl.c > @@ -183,7 +183,7 @@ int qda_release_dsp_process(struct qda_dev *qdev, struct > qda_session *session) > * 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) > +static int qda_attach_audio_pd(struct qda_session *session, const char > __user *uname, u32 namelen) > { > struct qda_dev *qdev = session->qda_dev; > struct qda_static_pd_args sargs = {}; > @@ -193,20 +193,20 @@ static int qda_attach_audio_pd(struct qda_session > *session, struct drm_qda_remot > if (strcmp(qdev->dsp_name, "adsp") || !qdev->remote_heap_size) > return -ENODEV; > > - if (!args->namelen || args->namelen > QDA_FASTRPC_INIT_NAMELEN_MAX) > + if (!namelen || namelen > QDA_FASTRPC_INIT_NAMELEN_MAX) > return -EINVAL; > > - name = memdup_user(u64_to_user_ptr(args->name), args->namelen); > + name = memdup_user(uname, namelen); > if (IS_ERR(name)) > return PTR_ERR(name); > > - if (name[args->namelen - 1]) { > + if (name[namelen - 1]) { > err = -EINVAL; > goto out_free; > } > > sargs.name = name; > - sargs.namelen = args->namelen; > + sargs.namelen = namelen; > > scoped_guard(mutex, &qdev->remote_heap_lock) { > if (!qdev->audio_init_mem) { [Severity: Medium] What happens in qda_attach_audio_pd() if qda_fastrpc_invoke() is interrupted by a signal after sending the request to the DSP? Inside the qdev->remote_heap_lock guard: 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; } qda_fastrpc_invoke_common() calls qda_rpmsg_send_msg() and then blocks in qda_rpmsg_wait_for_rsp() via wait_for_completion_interruptible(&ctx->work). If a signal interrupts the wait before the DSP replies, qda_fastrpc_invoke() returns -ERESTARTSYS while the first QDA_FASTRPC_RMID_INIT_CREATE_STATIC request is still in flight on the DSP. Because err is non-zero, qdev->audio_init_mem remains false when qdev->remote_heap_lock is released. Could a concurrent or restarted FASTRPC_IOCTL_INIT_CREATE_STATIC / DRM_IOCTL_QDA_REMOTE_SESSION_ATTACH call then observe !qdev->audio_init_mem and hand the remote heap to the DSP a second time? > @@ -226,38 +226,60 @@ static int qda_attach_audio_pd(struct qda_session > *session, struct drm_qda_remot > } > > /** > - * qda_ioctl_attach() - Attach to a DSP-resident protection domain > - * @dev: DRM device structure > - * @data: User-space data (struct drm_qda_remote_attach) > - * @file_priv: DRM file private data > + * qda_session_attach() - Attach a session to a DSP-resident protection > domain > + * @session: Session to attach > + * @pd_type: QDA_ATTACH_ROOT_PD, QDA_ATTACH_SENSORS_PD or QDA_ATTACH_AUDIO_PD > + * @name: User pointer to the static PD name (QDA_ATTACH_AUDIO_PD only) > + * @namelen: Length of @name including the NUL (QDA_ATTACH_AUDIO_PD only) > + * > + * Shared by the accel UAPI and the legacy shim, so the privilege check lives > + * here rather than on the ioctl. > * > * Return: 0 on success, negative error code on failure > */ > -int qda_ioctl_attach(struct drm_device *dev, void *data, struct drm_file > *file_priv) > +int qda_session_attach(struct qda_session *session, u32 pd_type, const char > __user *name, > + u32 namelen) > { > - struct drm_qda_remote_attach *args = data; > - struct qda_session *session = file_priv->driver_priv; > int pd; > > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > if (READ_ONCE(session->init_mem_gem_obj)) > return -EBUSY; > > - switch (args->pd_type) { > + switch (pd_type) { > case QDA_ATTACH_ROOT_PD: > case QDA_ATTACH_SENSORS_PD: > - if (args->name || args->namelen) > + if (name || namelen) > return -EINVAL; > > - pd = args->pd_type == QDA_ATTACH_ROOT_PD ? QDA_ROOT_PD : > QDA_SENSORS_PD; > + pd = pd_type == QDA_ATTACH_ROOT_PD ? QDA_ROOT_PD : > QDA_SENSORS_PD; > > return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_ATTACH, &pd, > session); > case QDA_ATTACH_AUDIO_PD: > - return qda_attach_audio_pd(session, args); > + return qda_attach_audio_pd(session, name, namelen); > default: > return -EINVAL; > } > } [Severity: High] Does qda_session_attach() race with concurrent qda_session_init_create() calls on the same session? Both the accel UAPI and the legacy shim allow concurrent ioctls on the same open file without holding a per-session lock. In qda_session_attach(), the check: if (READ_ONCE(session->init_mem_gem_obj)) return -EBUSY; runs unlocked before qda_fastrpc_invoke() sleeps waiting for the DSP response, and qda_session_init_create() does not check whether the session has already attached or is attaching. State updates only happen after the DSP responds in qda_fastrpc_invoke(): if (type == QDA_FASTRPC_RMID_INIT_CREATE) { qda_session_set_init_mem(session, ctx->init_mem_gem_obj); WRITE_ONCE(session->pd, QDA_USER_PD); } else if (type == QDA_FASTRPC_RMID_INIT_ATTACH || type == QDA_FASTRPC_RMID_INIT_CREATE_STATIC) { WRITE_ONCE(session->pd, ctx->pd); } If one thread calls qda_session_attach() and another concurrently calls qda_session_init_create() on the same file descriptor, both can pass validation and send RPCs to the DSP. If QDA_FASTRPC_RMID_INIT_CREATE finishes first (setting session->init_mem_gem_obj and session->pd = QDA_USER_PD) and QDA_FASTRPC_RMID_INIT_ATTACH finishes second, session->pd is overwritten with QDA_ROOT_PD or QDA_SENSORS_PD while session->init_mem_gem_obj remains non-NULL. When the file is later closed, legacy_release() or qda_postclose() sees session->init_mem_gem_obj is non-NULL and calls qda_release_dsp_process(), which sends QDA_FASTRPC_RMID_INIT_RELEASE directed at READ_ONCE(session->pd) (now the attached PD instead of QDA_USER_PD). Wouldn't that leave the User PD process running on the DSP while qda_session_release() frees session->init_mem_gem_obj underneath it? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
