Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] accel/qda: TOCTOU race and UAF between legacy_release() and qda_rpmsg_remove() - [High] accel/qda: missing QDA_FASTRPC_INIT_HANDLE check on user invoke paths - [Medium] accel/qda: qda_shim_init_create() imports args.filefd when filelen or filefd is 0 --
commit cd276e39d68d699c859df87ba50e8e071d334588 Author: Ekansh Gupta <[email protected]> accel/qda: Add DSP process creation and release to the legacy shim This commit adds DSP process creation (FASTRPC_IOCTL_INIT_CREATE) and release upon file close to the legacy /dev/fastrpc-* compatibility shim, enforcing secure/non-secure node rules and importing the ELF image file descriptor into the session's context bank. > diff --git a/drivers/accel/qda/qda_fastrpc_shim.c > b/drivers/accel/qda/qda_fastrpc_shim.c > index 03fce990ccdb9..4a2ba20ee01ac 100644 > --- a/drivers/accel/qda/qda_fastrpc_shim.c > +++ b/drivers/accel/qda/qda_fastrpc_shim.c [ ... ] > @@ -141,6 +154,43 @@ static long qda_shim_alloc_dma_buf(struct qda_shim_file > *sf, void __user *argp) > return 0; > } > > +/* > + * As in drivers/misc/fastrpc.c: on a secure channel, a client that came in > + * through the non-secure node may only create an unsigned PD, and only on a > + * domain that supports unsigned PD offload. > + */ > +static bool qda_shim_session_rejected(struct qda_shim_file *sf, bool > unsigned_pd) > +{ > + if (!sf->secure_node && sf->secure_channel) > + return !sf->unsigned_support || !unsigned_pd; > + > + return false; > +} > + > +static long qda_shim_init_create(struct qda_shim_file *sf, void __user *argp) > +{ > + struct drm_gem_object *elf_obj = NULL; > + struct fastrpc_init_create args; > + int err; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + if (qda_shim_session_rejected(sf, args.attrs & > FASTRPC_MODE_UNSIGNED_MODULE)) > + return -ECONNREFUSED; [Severity: High] Can user space bypass both qda_shim_session_rejected() here and the CAP_SYS_ADMIN check in qda_fastrpc_prepare_args_init_create() by sending a QDA_FASTRPC_INIT_HANDLE (0x1) message through FASTRPC_IOCTL_INVOKE or DRM_IOCTL_QDA_REMOTE_INVOKE? In drivers/misc/fastrpc.c, fastrpc_internal_invoke() rejects user-space invocations targeting FASTRPC_INIT_HANDLE with -EPERM when !kernel. Here, however, neither qda_shim_invoke() -> legacy_invoke() -> qda_fastrpc_invoke_legacy() nor qda_ioctl_invoke() -> qda_fastrpc_prepare_args_invoke() blocks handle == QDA_FASTRPC_INIT_HANDLE. Even more concerning, qda_fastrpc_invoke_pack() checks ctx->handle rather than ctx->type: if (ctx->handle == QDA_FASTRPC_INIT_HANDLE) msg->fastrpc.remote_session_id = 0; else msg->fastrpc.remote_session_id = ctx->session->remote_session_id; As a result, an unprivileged client on a non-secure node can call FASTRPC_IOCTL_INIT_CREATE with FASTRPC_MODE_UNSIGNED_MODULE to transition session->pd to QDA_USER_PD (or leave session->pd at QDA_ROOT_PD) and then issue FASTRPC_IOCTL_INVOKE with handle = QDA_FASTRPC_INIT_HANDLE and sc encoding QDA_FASTRPC_RMID_INIT_CREATE, QDA_FASTRPC_RMID_INIT_CREATE_ATTR, or QDA_FASTRPC_RMID_INIT_RELEASE using user-controlled buffers. Should QDA_FASTRPC_INIT_HANDLE be rejected on the user-space invoke paths? > + > + elf_obj = shim_import_fd(sf->session, args.filefd); > + if (IS_ERR(elf_obj)) > + return PTR_ERR(elf_obj); [Severity: Medium] What happens in qda_shim_init_create() when user space passes a zero-initialized args.filefd (0) for an empty image (args.filelen == 0) or for a bare-pointer image (args.file != 0, args.filelen > 0), or when args.filelen is 0 with a non-negative args.filefd? In drivers/misc/fastrpc.c, fastrpc_init_create_process() only imports init.filefd when both init.filelen and init.filefd are non-zero: if (init.filelen && init.filefd) { ... } And in drivers/accel/qda/qda_legacy_ops.c, legacy_init_create() is written to expect elf_obj == NULL when no descriptor-backed image is used: if (!elf_obj && filelen) return -EOPNOTSUPP; However, shim_import_fd() only treats negative file descriptors as absent: if (fd < 0) return NULL; dmabuf = dma_buf_get(fd); If a caller zero-initializes struct fastrpc_init_create so args.filefd is 0, shim_import_fd(sf->session, 0) calls dma_buf_get(0) on standard input, failing with -EINVAL or -EBADF instead of allowing an empty image or returning -EOPNOTSUPP for a bare pointer (or silently importing fd 0 if stdin happens to be a valid DMA-BUF). Should qda_shim_init_create() only call shim_import_fd() when args.filelen && args.filefd (or should shim_import_fd() treat fd <= 0 as absent)? > diff --git a/drivers/accel/qda/qda_legacy_ops.c > b/drivers/accel/qda/qda_legacy_ops.c > index 74ee4995350e2..2b3b3f4fb246b 100644 > --- a/drivers/accel/qda/qda_legacy_ops.c > +++ b/drivers/accel/qda/qda_legacy_ops.c [ ... ] > @@ -38,6 +39,42 @@ static void legacy_session_destroy(struct qda_session > *session) > drm_dev_put(drm_dev); > } [ ... ] > +static int legacy_release(struct qda_session *session) > +{ > + /* Only a session that created a process has one to release */ > + if (!READ_ONCE(session->init_mem_gem_obj)) > + return 0; > + > + /* Unplug check only; see qda_shim_ioctl() for why not drm_dev_enter() > */ > + if (drm_dev_is_unplugged(&session->qda_dev->drm_dev)) > + return -ENODEV; > + > + return qda_release_dsp_process(session->qda_dev, session); > +} [Severity: High] Does using drm_dev_is_unplugged() instead of holding a critical section across qda_release_dsp_process() expose a race and use-after-free with qda_rpmsg_remove()? In drivers/accel/qda/qda_rpmsg.c, qda_rpmsg_remove() relies on drm_dev_unplug() to wait for active callers to finish before flushing pending contexts and tearing down device resources: drm_dev_unplug(&qdev->drm_dev); qdev->rpdev = NULL; qda_fastrpc_flush_pending(qdev); qda_cb_unpopulate(qdev); qda_deinit_device(qdev); Because drm_dev_is_unplugged() immediately drops its SRCU read lock via drm_dev_exit(), qda_rpmsg_remove() can run concurrently right after drm_dev_is_unplugged() returns false: 1. In qda_release_dsp_process() -> qda_fastrpc_invoke(), qda_fastrpc_context_alloc(qdev) allocates ctx with refcount == 1 and stores it in qdev->ctx_xa. 2. Concurrently, qda_rpmsg_remove() calls drm_dev_unplug(&qdev->drm_dev). When qda_fastrpc_invoke_common() calls qda_rpmsg_send_msg(), drm_dev_enter() now fails with -ENODEV, so the second reference on ctx for the response callback is never taken. 3. Meanwhile, qda_fastrpc_flush_pending(qdev) in qda_rpmsg_remove() iterates qdev->ctx_xa and calls kref_put(&ctx->refcount, qda_fastrpc_context_free), dropping the caller's sole reference and freeing ctx while qda_fastrpc_invoke() is still using it and about to call kref_put(&ctx->refcount, qda_fastrpc_context_free) a second time at out_put. 4. Furthermore, qda_rpmsg_remove() proceeds to qda_cb_unpopulate(qdev) and qda_deinit_device(qdev) (destroying the context-bank platform devices, qdev->iommu_mgr, and qdev->ctx_xa) while qda_release_dsp_process() is still allocating from qdev->ctx_xa and qdev->iommu_mgr. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
