Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] accel/qda: missing GEM object reference for active DSP mappings - [High] accel/qda: refcount underflow and UAF in qda_fastrpc_flush_pending() - [Low] accel/qda: duplicate address fields and unchecked fields in qda_ioctl_munmap() --
commit 52a28fd51f1253e9ef94a6c86442ba00197bb35c Author: Ekansh Gupta <[email protected]> accel/qda: Add remote memory unmapping from the DSP address space This commit adds the DRM_IOCTL_QDA_REMOTE_MUNMAP ioctl and FastRPC prepare handlers to allow user space to unmap plain and attribute-based buffer mappings from the DSP address space. > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index 744d7317cbe11..dbd14cd07cb20 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c > @@ -1072,6 +1072,73 @@ static int qda_fastrpc_prepare_args_mem_map(struct > qda_fastrpc_invoke_ctx *ctx, > return err; > } > > +/* > + * INIT_MUNMAP packed layout: > + * [0] req sizeof(qda_fastrpc_munmap_req_msg) IN > + */ > +static int qda_fastrpc_prepare_args_munmap(struct qda_fastrpc_invoke_ctx > *ctx, void *argp) > +{ > + struct drm_qda_mem_unmap *uargs = argp; > + struct qda_fastrpc_munmap_req_msg *req; > + struct qda_gem_obj *kernel_gem; > + int err; > + > + ctx->sc = QDA_FASTRPC_SCALARS(QDA_FASTRPC_RMID_INIT_MUNMAP, 1, 0); > + ctx->handle = QDA_FASTRPC_INIT_HANDLE; > + > + err = alloc_kernel_args(ctx, 1); > + if (err) > + return err; > + > + kernel_gem = alloc_kernel_gem(ctx, sizeof(*req)); > + if (IS_ERR(kernel_gem)) > + return PTR_ERR(kernel_gem); > + > + req = kernel_gem->virt; > + req->remote_session_id = ctx->session->remote_session_id; > + req->vaddr = uargs->vaddrout; > + req->size = uargs->size; > + > + assign_kernel_gem_arg(ctx, 0, kernel_gem, req, sizeof(*req)); > + drm_gem_object_put(&kernel_gem->base); > + > + return 0; > +} > + > +/* > + * INIT_MEM_UNMAP packed layout: > + * [0] req sizeof(qda_fastrpc_mem_unmap_req_msg) IN > + */ > +static int qda_fastrpc_prepare_args_mem_unmap(struct qda_fastrpc_invoke_ctx > *ctx, void *argp) > +{ > + struct drm_qda_mem_unmap *uargs = argp; > + struct qda_fastrpc_mem_unmap_req_msg *req; > + struct qda_gem_obj *kernel_gem; > + int err; > + > + ctx->sc = QDA_FASTRPC_SCALARS(QDA_FASTRPC_RMID_INIT_MEM_UNMAP, 1, 0); > + ctx->handle = QDA_FASTRPC_INIT_HANDLE; > + > + err = alloc_kernel_args(ctx, 1); > + if (err) > + return err; > + > + kernel_gem = alloc_kernel_gem(ctx, sizeof(*req)); > + if (IS_ERR(kernel_gem)) > + return PTR_ERR(kernel_gem); > + > + req = kernel_gem->virt; > + req->remote_session_id = ctx->session->remote_session_id; > + req->handle = uargs->dsp_handle; > + req->vaddrin = uargs->vaddr; > + req->len = uargs->size; > + > + assign_kernel_gem_arg(ctx, 0, kernel_gem, req, sizeof(*req)); > + drm_gem_object_put(&kernel_gem->base); > + > + return 0; > +} [Severity: High] Can leaving the GEM object lifetime entirely to user space while a DSP mapping is active lead to DMA use-after-free? During qda_ioctl_mmap(), setup_mmap_pages() acquires ctx->map_obj only for the duration of the MMAP RPC, and qda_fastrpc_context_free() drops it as soon as the ioctl completes: drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_free() { ... if (ctx->map_obj) drm_gem_object_put(ctx->map_obj); ... } While the mapping remains live on the DSP between DRM_IOCTL_QDA_REMOTE_MMAP and DRM_IOCTL_QDA_REMOTE_MUNMAP, the kernel does not track the mapping on struct qda_session or hold a drm_gem_object reference. If user space closes the GEM handle via DRM_IOCTL_GEM_CLOSE before calling DRM_IOCTL_QDA_REMOTE_MUNMAP (or if qda_rpmsg_wait_for_rsp() is interrupted by a signal after the map request was sent so user space never receives vaddrout), qda_gem_free_object() immediately frees the DMA buffer and its IOVA in session->assigned_iommu_dev while the DSP mapping is still active. Because session->assigned_iommu_dev is also used by alloc_kernel_gem() and qda_fastrpc_invoke_pack() to allocate internal FastRPC request and payload buffers, can the freed IOVA be reused for kernel RPC messages while an unprivileged DSP user process still has vaddrout mapped to it? Furthermore, even if user space keeps the handle open, when a process exits or is killed while a mapping is active, drm_file_free() runs drm_gem_release() before dev->driver->postclose() (qda_postclose()): drivers/gpu/drm/drm_file.c:drm_file_free() -> drm_gem_release() -> qda_postclose() -> qda_release_dsp_process() Doesn't this free and unmap all GEM buffers while the DSP process is still running and before qda_release_dsp_process() tears it down? Also, because active mappings are not tracked by the kernel, qda_fastrpc_prepare_args_munmap() and qda_fastrpc_prepare_args_mem_unmap() forward arbitrary user-supplied vaddr/vaddrout, size, and dsp_handle values to QDA_FASTRPC_INIT_HANDLE without verifying that they match an existing mapping. > diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c > index cd3cea72f50e8..17d1b488b4a9b 100644 > --- a/drivers/accel/qda/qda_ioctl.c > +++ b/drivers/accel/qda/qda_ioctl.c > @@ -197,6 +197,30 @@ int qda_ioctl_mmap(struct drm_device *dev, void *data, > struct drm_file *file_pri > } > } > > +/** > + * qda_ioctl_munmap() - Remove a buffer's mapping from the DSP's address > space > + * @dev: DRM device structure > + * @data: User-space data (struct drm_qda_mem_unmap) > + * @file_priv: DRM file private data > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_ioctl_munmap(struct drm_device *dev, void *data, struct drm_file > *file_priv) > +{ > + struct drm_qda_mem_unmap *unmap_req = data; > + > + switch (unmap_req->request) { > + case QDA_REQUEST_MUNMAP: > + return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_MUNMAP, data, > + file_priv->driver_priv); > + case QDA_REQUEST_MEM_UNMAP: > + return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_MEM_UNMAP, data, > + file_priv->driver_priv); > + default: > + return -EINVAL; > + } > +} [Severity: High] Can calling qda_fastrpc_invoke() from qda_ioctl_munmap() race with qda_fastrpc_flush_pending() during rpmsg teardown and cause a refcount underflow and use-after-free on struct qda_fastrpc_invoke_ctx? In qda_fastrpc_invoke(), qda_fastrpc_context_alloc() initializes ctx->refcount to 1 and inserts ctx into qdev->ctx_xa before the message is prepared or sent: drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_alloc() { ... kref_init(&ctx->refcount); ... ret = xa_alloc(&qdev->ctx_xa, &id, ctx, XA_LIMIT(1, QDA_FASTRPC_CTXID_MAX), GFP_KERNEL); ... } The second reference on ctx->refcount for the response handler is only taken later in qda_rpmsg_send_msg(), and is dropped immediately if rpmsg_send() fails or asynchronously via ctx->put_work after qda_rpmsg_cb() runs. However, ctx is only erased from qdev->ctx_xa inside qda_fastrpc_context_free() when the final reference is dropped. If qda_rpmsg_remove() runs concurrently and calls qda_fastrpc_flush_pending(): drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_flush_pending() { ... xa_for_each(&qdev->ctx_xa, idx, ctx) { ctx->retval = -EPIPE; complete(&ctx->work); kref_put(&ctx->refcount, qda_fastrpc_context_free); } } while an invocation in qda_fastrpc_invoke() is still in qda_fastrpc_prepare_args() or qda_fastrpc_invoke_pack(), or after qda_rpmsg_send_msg() failed (for example with -ENODEV at drm_dev_enter()), or after qda_rpmsg_cb() already queued ctx->put_work, won't qda_fastrpc_flush_pending() drop a reference that was never held by the pending response path, freeing ctx before the caller or worker drops its own reference? > diff --git a/include/uapi/drm/qda_accel.h b/include/uapi/drm/qda_accel.h > index bd2d0f4308b10..43598ddef461d 100644 > --- a/include/uapi/drm/qda_accel.h > +++ b/include/uapi/drm/qda_accel.h [ ... ] > @@ -173,6 +180,29 @@ struct drm_qda_mem_map { > __u64 vaddrout; > }; > > +/** > + * struct drm_qda_mem_unmap - Parameters for removing a DSP mapping > + * @request: QDA_REQUEST_MUNMAP or QDA_REQUEST_MEM_UNMAP > + * @dsp_handle: Identifier the DSP tracks the buffer by > (QDA_REQUEST_MEM_UNMAP) > + * @vaddr: Address the mapping was made at (QDA_REQUEST_MEM_UNMAP) > + * @vaddrout: Address the mapping was made at (QDA_REQUEST_MUNMAP) > + * @size: Length of the mapping in bytes > + * > + * Removes a mapping made with DRM_IOCTL_QDA_REMOTE_MMAP, named by the > address > + * that call returned in drm_qda_mem_map.vaddrout. The request must match the > + * one the mapping was made with: QDA_REQUEST_MUNMAP uses @vaddrout and > @size, > + * QDA_REQUEST_MEM_UNMAP uses @dsp_handle, @vaddr and @size. The buffer is > not > + * affected, so its handle must stay open for as long as the DSP uses the > + * mapping. > + */ > +struct drm_qda_mem_unmap { > + __u32 request; > + __s32 dsp_handle; > + __u64 vaddr; > + __u64 vaddrout; > + __u64 size; > +}; [Severity: Low] As reported by Sashiko on v1 [1], struct drm_qda_mem_unmap defines two separate address fields (vaddr and vaddrout) for the same DSP virtual address returned in drm_qda_mem_map.vaddrout, and qda_ioctl_munmap() does not verify that the fields unused by the selected request type (dsp_handle and vaddr for QDA_REQUEST_MUNMAP, or vaddrout for QDA_REQUEST_MEM_UNMAP) are zero. Could a single address field be used here, with unused fields checked for zero in qda_ioctl_munmap()? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
