Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] accel/qda: missing VMA and size checks in process_msg_buffer() - [High] accel/qda: broken fd and handle semantics in legacy invoke path - [High] accel/qda: premature unmap of legacy DMA handle args in qda_fastrpc_context_free() - [High] accel/qda: premature ctx_xa publication in qda_fastrpc_invoke_legacy() - [High] accel/qda: race with unplug teardown in qda_shim_invoke() --
commit db6b9ba28a99a54082c1f760f47bf540c628d123 Author: Ekansh Gupta <[email protected]> accel/qda: Add invocation through the legacy FastRPC shim This commit adds FASTRPC_IOCTL_INVOKE support to the legacy FastRPC compatibility shim by importing each fd-backed argument into the session's context bank and passing the resolved GEM objects into qda_fastrpc_invoke_legacy(). Buffer offsets for legacy arguments are derived from the caller's VMA mapping in process_msg_buffer(). > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index 3a1959e2c53ee..693dd6f035d2a 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c > @@ -55,6 +55,22 @@ static void setup_pages_from_gem_obj(struct qda_gem_obj > *qda_gem_obj, > pages->size = qda_gem_obj->size; > } > > +static int calculate_vma_offset(u64 user_ptr, u64 *offset) > +{ > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(current->mm); > + vma = find_vma(current->mm, user_ptr); > + if (vma && vma->vm_start <= user_ptr) { > + *offset = (user_ptr & PAGE_MASK) - vma->vm_start; > + ret = 0; > + } > + mmap_read_unlock(current->mm); > + > + return ret; > +} > + > static u64 calculate_page_aligned_size(u64 addr, u64 len) > { > u64 pg_start = (addr & PAGE_MASK) >> PAGE_SHIFT; > @@ -286,23 +302,45 @@ struct qda_fastrpc_invoke_ctx > *qda_fastrpc_context_alloc(struct qda_dev *qdev) > } > > /* > - * Resolve the address of an in/out buffer argument. The buffer is named by a > - * GEM handle, looked up in the calling session's own handle table, so a > - * session cannot name another's buffer. The offset and length come from user > - * space and are checked against the buffer's real size: a region described > as > - * larger than its buffer would let the DSP read and write past the end of > it, > - * into whatever else is mapped in the same context bank. > + * Resolve the address of an in/out buffer argument. The buffer is either a > + * GEM object the legacy shim already resolved into ctx->gem_objs[i], or a > GEM > + * handle looked up in the calling session's own handle table. For a handle, > + * the offset and length come from user space and are checked against the > + * buffer's real size: a region described as larger than its buffer would let > + * the DSP read and write past the end of it, into whatever else is mapped in > + * the same context bank. > */ > static int process_msg_buffer(struct qda_fastrpc_invoke_ctx *ctx, int i, > union qda_remote_arg *rpra, > struct qda_phy_page *pages) > { > u64 len = ctx->args[i].length; > - struct qda_gem_obj *qda_gem_obj; > struct drm_gem_object *gem_obj; > + struct qda_gem_obj *qda_gem_obj; > u64 unaligned_addr; > + u64 vma_offset = 0; > int err; > > + if (ctx->gem_objs[i]) { > + /* > + * The legacy ABI predates GEM handles, so the offset field > + * holds an address in the caller's own mapping of the buffer, > + * or 0 if it has none, meaning the start of the buffer. > + */ > + qda_gem_obj = to_qda_gem_obj(ctx->gem_objs[i]); > + if (ctx->args[i].offset) { > + err = calculate_vma_offset(ctx->args[i].offset, > &vma_offset); > + if (err) > + return err; > + } > + > + rpra[i].buf.addr = ctx->args[i].offset; > + pages[i].addr = qda_gem_obj->dma_addr + vma_offset; > + pages[i].size = > calculate_page_aligned_size(ctx->args[i].offset, len); > + > + return 0; > + } [Severity: Critical] Could this legacy buffer path in process_msg_buffer() in drivers/accel/qda/qda_fastrpc.c allow out-of-bounds DSP DMA access or Stream ID spoofing across context banks? When qda_shim_invoke() -> qda_fastrpc_invoke_legacy() -> qda_fastrpc_invoke_common() -> qda_fastrpc_invoke_pack() -> qda_fastrpc_get_args() -> process_msg_buffer() resolves a buffer with ctx->gem_objs[i] set, calculate_vma_offset() looks up any VMA in current->mm containing user_ptr and computes: *offset = (user_ptr & PAGE_MASK) - vma->vm_start; without verifying that the VMA backs the imported DMA-BUF for ctx->gem_objs[i] or accounting for vma->vm_pgoff. Unlike the GEM handle path below it, which checks: if (ctx->args[i].offset > qda_gem_obj->size || len > qda_gem_obj->size - ctx->args[i].offset) and unlike fastrpc_map_attach() in drivers/misc/fastrpc.c, which checks len > map->size, the legacy branch in process_msg_buffer() does not check vma_offset, the in-page offset (ctx->args[i].offset & ~PAGE_MASK), or len against qda_gem_obj->size. As a result, passing a length or VMA offset larger than qda_gem_obj->size sets pages[i].addr and pages[i].size to a range extending past the end of qda_gem_obj into adjacent mappings in the session's IOMMU context bank, such as ctx->msg_gem_obj. In addition, because qda_gem_obj->dma_addr encodes the session's Stream ID at soc_data->sid_pos (bit 32 on default_soc_data), passing a user_ptr inside a large anonymous VMA so that vma_offset >= (1ULL << 32) causes qda_gem_obj->dma_addr + vma_offset to carry into the Stream ID bits and target another session's context bank. Finally, because ctx->args[i].offset and len are unvalidated 64-bit values on this path, addr + len - 1 and (pg_end - pg_start + 1) * PAGE_SIZE in calculate_page_aligned_size() can overflow 64-bit arithmetic and wrap pages[i].size to a small value or zero. Should calculate_vma_offset() and process_msg_buffer() verify that the offset and length stay within qda_gem_obj->size without overflowing? [ ... ] > @@ -551,6 +589,105 @@ int qda_fastrpc_invoke_common(struct qda_dev *qdev, > struct qda_fastrpc_invoke_ct > return 0; > } > > +/** > + * qda_fastrpc_invoke_legacy() - Perform a dynamic invocation on behalf of > the legacy shim > + * @qdev: Device to invoke on > + * @session: Legacy session (no backing drm_file) > + * @handle: Remote handle to invoke on the DSP > + * @sc: FastRPC scalars value encoding the number of in/out buffers > + * @legacy_args: Kernel-side argument array, one entry per > QDA_REMOTE_SCALARS_LENGTH(sc) > + * > + * Unlike the accel UAPI path, the argument descriptors and any backing GEM > + * objects already live in kernel memory: the shim has already resolved > + * every fd-backed argument via its own import path, since a legacy session > + * has no GEM handle table to resolve a handle-based argument against. > + * Ownership of each @legacy_args[i].obj reference transfers to this > function, > + * on failure as well as on success. > + * > + * Return: 0 on success, negative error code on failure > + */ > +int qda_fastrpc_invoke_legacy(struct qda_dev *qdev, struct qda_session > *session, > + u32 handle, u32 sc, struct qda_legacy_invoke_arg > *legacy_args) > +{ > + struct qda_fastrpc_invoke_ctx *ctx; > + struct drm_qda_fastrpc_invoke_args *args; > + u32 nscalars; > + int err; > + int i; > + > + nscalars = QDA_REMOTE_SCALARS_LENGTH(sc); > + > + /* > + * An fd naming a buffer another session exported resolves to the > + * original object, still mapped in that session's context bank, so > + * the same check as for a handle applies. > + */ > + for (i = 0; i < nscalars; ++i) { > + if (legacy_args[i].obj && > + to_qda_gem_obj(legacy_args[i].obj)->iommu_dev != > session->assigned_iommu_dev) { > + err = -EACCES; > + goto err_put_legacy_objs; > + } > + } > + > + ctx = qda_fastrpc_context_alloc(qdev); > + if (IS_ERR(ctx)) { > + err = PTR_ERR(ctx); > + goto err_put_legacy_objs; > + } > + > + /* Dropped by qda_fastrpc_context_free(), as for the accel UAPI path */ > + qda_session_get(session); > + ctx->session = session; > + ctx->handle = handle; > + ctx->sc = sc; > + > + ctx->nbufs = QDA_REMOTE_SCALARS_INBUFS(sc) + > QDA_REMOTE_SCALARS_OUTBUFS(sc); > + if (nscalars) { > + args = kzalloc_objs(*args, nscalars); > + if (!args) { > + err = -ENOMEM; > + goto err_put_legacy_objs_ctx; > + } > + > + ctx->gem_objs = kzalloc_objs(*ctx->gem_objs, nscalars); > + if (!ctx->gem_objs) { > + kfree(args); > + err = -ENOMEM; > + goto err_put_legacy_objs_ctx; > + } > + > + for (i = 0; i < nscalars; ++i) { > + args[i].offset = legacy_args[i].ptr; > + args[i].length = legacy_args[i].length; > + /* process_dma_handle() skips handle args whose handle > is 0 */ > + args[i].handle = (i >= ctx->nbufs && > legacy_args[i].obj) ? > + (u32)legacy_args[i].fd : 0; > + ctx->gem_objs[i] = legacy_args[i].obj; > + } [Severity: High] Won't moving the sole reference to legacy_args[i].obj into ctx->gem_objs[i] for DMA handle arguments (i >= ctx->nbufs) in qda_fastrpc_invoke_legacy() cause them to be unmapped from the SMMU context bank as soon as the invocation finishes? In qda_shim_invoke(), shim_import_fd() imports each fd via qda_prime_import_dmabuf(), returning a drm_gem_object with a single reference for any external DMA-BUF (or once user space closes its fd). When the invocation finishes, qda_fastrpc_context_free() drops every entry in ctx->gem_objs: if (ctx->gem_objs) { for (i = 0; i < ctx->nscalars; ++i) { if (ctx->gem_objs[i]) drm_gem_object_put(ctx->gem_objs[i]); } kfree(ctx->gem_objs); } Unlike drivers/misc/fastrpc.c (where fastrpc_create_maps() retains handle mappings on fl->maps across invocations until the DSP returns their fd in fdlist) and unlike the accel UAPI path (where ctx->session->file holds a GEM handle reference until qda_fastrpc_cleanup_handlelist() deletes it), qda_fastrpc_context_free() drops the last reference on DMA handle objects immediately on return, unmapping them while the DSP still holds their IOVA. Moreover, qda_fastrpc_cleanup_handlelist() checks if (ctx->session->file), which is always NULL for legacy sessions. How are DMA handle mappings meant to stay alive across invocations and be released via handlelist for legacy sessions? > + > + ctx->args = args; > + } > + > + ctx->nscalars = nscalars; > + > + err = qda_fastrpc_invoke_common(qdev, ctx); > + > + kref_put(&ctx->refcount, qda_fastrpc_context_free); [Severity: High] Can a concurrent call to qda_rpmsg_remove() -> qda_fastrpc_flush_pending() drop the only reference on ctx and cause a use-after-free and double put in qda_fastrpc_invoke_legacy()? In qda_fastrpc_invoke_legacy(), qda_fastrpc_context_alloc() initializes ctx->refcount to 1 and immediately inserts ctx into qdev->ctx_xa via xa_alloc(), before ctx->session, ctx->args, and ctx->gem_objs are populated and before qda_rpmsg_send_msg() takes the second reference for the response path. Because qda_shim_ioctl() only performs a lockless drm_dev_is_unplugged() check rather than holding a drm_dev_enter() section across the ioctl, qda_rpmsg_remove() can run concurrently and call 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); } If qda_fastrpc_flush_pending() runs before qda_rpmsg_send_msg() acquires the second reference (or after qda_rpmsg_send_msg() fails with -ENODEV), it drops ctx->refcount from 1 to 0 and frees ctx while qda_fastrpc_invoke_legacy() is still using it, followed by a second kref_put() on the freed ctx when qda_fastrpc_invoke_legacy() returns. [ ... ] > diff --git a/drivers/accel/qda/qda_fastrpc_shim.c > b/drivers/accel/qda/qda_fastrpc_shim.c > index 25f5c242efde1..03fce990ccdb9 100644 > --- a/drivers/accel/qda/qda_fastrpc_shim.c > +++ b/drivers/accel/qda/qda_fastrpc_shim.c [ ... ] > @@ -50,6 +52,25 @@ struct qda_shim_dev { > > static const struct file_operations qda_shim_fops; > > +/* A negative @fd means the argument has no buffer, which is not an error */ > +static struct drm_gem_object *shim_import_fd(struct qda_session *session, > s32 fd) > +{ > + struct drm_gem_object *obj; > + struct dma_buf *dmabuf; > + > + if (fd < 0) > + return NULL; > + > + dmabuf = dma_buf_get(fd); > + if (IS_ERR(dmabuf)) > + return ERR_CAST(dmabuf); > + > + obj = qda_legacy_ops_table.import_dmabuf(session, dmabuf); > + dma_buf_put(dmabuf); > + > + return obj; > +} > + > static int qda_shim_open(struct inode *inode, struct file *file) > { > struct qda_shim_node *snode = container_of(file->private_data, struct > qda_shim_node, > @@ -120,11 +141,77 @@ static long qda_shim_alloc_dma_buf(struct qda_shim_file > *sf, void __user *argp) > return 0; > } > > +static long qda_shim_invoke(struct qda_shim_file *sf, void __user *argp) > +{ > + struct qda_legacy_invoke_arg *largs; > + struct fastrpc_invoke_args *uargs; > + struct fastrpc_invoke args; > + u32 nargs; > + int err; > + int i; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + nargs = QDA_REMOTE_SCALARS_LENGTH(args.sc); > + if (!nargs) > + return qda_legacy_ops_table.invoke(sf->session, args.handle, > args.sc, NULL); > + > + uargs = kcalloc(nargs, sizeof(*uargs), GFP_KERNEL); > + if (!uargs) > + return -ENOMEM; > + > + if (copy_from_user(uargs, u64_to_user_ptr(args.args), nargs * > sizeof(*uargs))) { > + kfree(uargs); > + return -EFAULT; > + } > + > + largs = kcalloc(nargs, sizeof(*largs), GFP_KERNEL); > + if (!largs) { > + kfree(uargs); > + return -ENOMEM; > + } > + > + for (i = 0; i < nargs; ++i) { > + struct drm_gem_object *obj; > + > + largs[i].ptr = uargs[i].ptr; > + largs[i].length = uargs[i].length; > + largs[i].fd = uargs[i].fd; > + > + obj = shim_import_fd(sf->session, uargs[i].fd); > + if (IS_ERR(obj)) { > + err = PTR_ERR(obj); > + goto err_put_objs; > + } > + largs[i].obj = obj; > + } [Severity: High] Does the fd handling across shim_import_fd(), qda_fastrpc_invoke_legacy(), process_msg_buffer(), and process_dma_handle() break several valid legacy FASTRPC_IOCTL_INVOKE argument patterns? 1. In drivers/misc/fastrpc.c, fastrpc_create_maps() skips importing arguments when fd == 0, fd == -1, or length == 0. Here, qda_shim_invoke() calls shim_import_fd() even when uargs[i].length == 0, and shim_import_fd() only returns NULL for fd < 0. If user space passes a zero-initialized descriptor (fd == 0, length == 0) or fd == 0 for an unbacked argument, shim_import_fd() calls dma_buf_get(0) (typically stdin) and fails the ioctl with -EINVAL or -EBADF. 2. Conversely, if a DMA handle argument (i >= ctx->nbufs) genuinely uses fd == 0 and shim_import_fd() imports it into legacy_args[i].obj, qda_fastrpc_invoke_legacy() sets: args[i].handle = (i >= ctx->nbufs && legacy_args[i].obj) ? (u32)legacy_args[i].fd : 0; which evaluates to 0. In process_dma_handle(), the check for !ctx->args[i].handle runs before checking ctx->gem_objs[i], so fd 0 is treated as having no buffer and setup_pages_from_gem_obj() is skipped. 3. For a buffer argument (i < ctx->nbufs) with fd < 0 and length > 0, shim_import_fd() returns NULL ("not an error"), leaving ctx->gem_objs[i] == NULL and ctx->args[i].handle == 0. When qda_fastrpc_get_args() calls process_msg_buffer(), both ctx->gem_objs[i] and ctx->args[i].handle are 0, so process_msg_buffer() immediately returns -EINVAL. > + > + kfree(uargs); > + > + /* Takes ownership of every largs[i].obj, whether or not it succeeds */ > + err = qda_legacy_ops_table.invoke(sf->session, args.handle, args.sc, > largs); [Severity: High] What prevents qda_rpmsg_remove() from tearing down the device while qda_shim_invoke() is importing buffers and building the invocation in drivers/accel/qda/qda_fastrpc_shim.c? In qda_shim_ioctl(), the unplug check is a lockless call to drm_dev_is_unplugged(&sf->session->qda_dev->drm_dev), and drm_dev_enter() is not acquired until qda_rpmsg_send_msg(). If qda_rpmsg_remove() runs right after qda_shim_ioctl() passes drm_dev_is_unplugged(), drm_dev_unplug() completes immediately without waiting for in-flight shim ioctls, and qda_rpmsg_remove() proceeds to run: qda_fastrpc_flush_pending(qdev); qda_cb_unpopulate(qdev); qda_deinit_device(qdev); This destroys the context bank platform devices (of_platform_device_destroy), frees qdev->iommu_mgr->devices, and destroys assignment_lock, qdev->ctx_xa, and qdev->import_lock while qda_shim_invoke() -> shim_import_fd() -> qda_prime_import_dmabuf() and qda_fastrpc_invoke_legacy() are concurrently accessing them. Should qda_shim_ioctl() guard the dispatch call with drm_dev_enter() and drm_dev_exit() so that drm_dev_unplug() waits for in-flight legacy ioctls to finish before tearing down the device? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
