Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] accel/qda: use-after-free race on placeholder in qda_shim_mem_map() - [High] accel/qda: unmap size mismatch in qda_shim_mmap() when size is 0 - [High] accel/qda: missing unplug check in shim_mappings_release() on close - [High] accel/qda: premature buffer release when legacy unmap fails or is interrupted - [Medium] accel/qda: race in shim_mapping_detach() and shim_mapping_attach() - [Medium] accel/qda: missing offset bounds check in setup_mmap_pages() --
commit b1739b465f1ac98dac259c9a90724960699924f1 Author: Ekansh Gupta <[email protected]> accel/qda: Add remote memory map and unmap to the legacy shim This commit implements the four legacy FastRPC memory map and unmap ioctls in the QDA compatibility shim by importing DMA-BUF file descriptors and tracking live mappings per open file so buffers remain referenced while mapped on the DSP. > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index dbd14cd07cb20..b35604fce630c 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c [ ... ] > @@ -935,19 +935,33 @@ static int qda_fastrpc_prepare_args_init_create(struct > qda_fastrpc_invoke_ctx *c > * buffer is held by the context until the request is done, so that closing > its > * handle meanwhile cannot free memory the DSP is being told to map. The > * region must fit in the buffer, as the DSP would otherwise map past its > end. > + * > + * A caller with no handle table supplies the buffer already imported. It is > + * checked as a buffer named by handle would be: an fd can name a buffer that > + * another session exported, which resolves to the original object, still > + * mapped in that session's context bank. > */ > -static int setup_mmap_pages(struct qda_fastrpc_invoke_ctx *ctx, struct > drm_qda_mem_map *margs, > +static int setup_mmap_pages(struct qda_fastrpc_invoke_ctx *ctx, struct > qda_mem_map_args *args, > struct qda_phy_page *pages) > { > + struct drm_qda_mem_map *margs = &args->map; > struct qda_gem_obj *qda_gem_obj; > int err; > > - if (!margs->handle) > - return -EINVAL; > + if (args->obj) { > + if (to_qda_gem_obj(args->obj)->iommu_dev != > ctx->session->assigned_iommu_dev) > + return -EACCES; > > - err = get_gem_obj_from_handle(ctx, margs->handle, &ctx->map_obj); > - if (err) > - return err; > + drm_gem_object_get(args->obj); > + ctx->map_obj = args->obj; > + } else { > + if (!margs->handle) > + return -EINVAL; > + > + err = get_gem_obj_from_handle(ctx, margs->handle, > &ctx->map_obj); > + if (err) > + return err; > + } > > qda_gem_obj = to_qda_gem_obj(ctx->map_obj); > if (margs->size > qda_gem_obj->size) > return -EINVAL; > > pages->addr = qda_gem_obj->dma_addr; > pages->size = margs->size ? margs->size : qda_gem_obj->size; [Severity: Medium] Can a caller of FASTRPC_IOCTL_MEM_MAP or DRM_IOCTL_QDA_REMOTE_MMAP (with QDA_REQUEST_MEM_MAP) map past the end of qda_gem_obj by supplying a non-zero margs->offset? In qda_shim_mem_map(), only args.offset < 0 is rejected, and qda_fastrpc_prepare_args_mem_map() only checks margs->offset > INT_MAX before setting req->offset = margs->offset. Here in setup_mmap_pages(), only margs->size > qda_gem_obj->size is checked. If a caller passes 0 < margs->offset <= INT_MAX and margs->size <= qda_gem_obj->size (including margs->size == 0, which sets pages->size to qda_gem_obj->size) such that margs->offset + pages->size exceeds qda_gem_obj->size, shouldn't setup_mmap_pages() also check that margs->offset <= qda_gem_obj->size and pages->size <= qda_gem_obj->size - margs->offset? > diff --git a/drivers/accel/qda/qda_fastrpc_shim.c > b/drivers/accel/qda/qda_fastrpc_shim.c > index 4a2ba20ee01ac..c46d2de4f5818 100644 > --- a/drivers/accel/qda/qda_fastrpc_shim.c > +++ b/drivers/accel/qda/qda_fastrpc_shim.c [ ... ] > @@ -79,6 +101,83 @@ static struct drm_gem_object *shim_import_fd(struct > qda_session *session, s32 fd > return obj; > } > > +/* Takes ownership of @obj on success; on failure the caller still owns it */ > +static struct qda_shim_mapping *shim_mapping_add(struct qda_shim_file *sf, > + struct list_head *list, u64 > key, u64 vaddr, > + u64 size, struct > drm_gem_object *obj) > +{ > + struct qda_shim_mapping *m, *new; > + > + new = kzalloc_obj(*new); > + if (!new) > + return ERR_PTR(-ENOMEM); > + > + new->key = key; > + new->vaddr = vaddr; > + new->size = size; > + new->obj = obj; > + > + guard(mutex)(&sf->maps_lock); > + > + list_for_each_entry(m, list, node) { > + if (m->key == key) { > + kfree(new); > + return ERR_PTR(-EEXIST); > + } > + } > + list_add_tail(&new->node, list); > + > + return new; > +} > + > +static struct qda_shim_mapping *shim_mapping_detach(struct qda_shim_file *sf, > + struct list_head *list, u64 > key, u64 vaddr) > +{ > + struct qda_shim_mapping *m; > + > + guard(mutex)(&sf->maps_lock); > + > + list_for_each_entry(m, list, node) { > + if (m->key == key && m->vaddr == vaddr) { > + list_del(&m->node); > + return m; > + } > + } > + > + return NULL; > +} > + > +static void shim_mapping_attach(struct qda_shim_file *sf, struct list_head > *list, > + struct qda_shim_mapping *m) > +{ > + guard(mutex)(&sf->maps_lock); > + list_add_tail(&m->node, list); > +} [Severity: Medium] Can detaching a mapping in shim_mapping_detach() and dropping sf->maps_lock before validating size or calling the blocking unmap RPC race with concurrent map and unmap calls on the same open file? First, in qda_shim_mem_unmap(), m is removed from sf->mem_map_list while qda_legacy_ops_table.mem_unmap() runs without sf->maps_lock held. While m is detached, its key (args.fd) is absent from sf->mem_map_list, so a concurrent qda_shim_mem_map() with the same fd can pass the duplicate-key check in shim_mapping_add() and insert a second entry with the same fd. If the in-flight mem_unmap() then fails (for example, interrupted by a signal with -ERESTARTSYS in wait_for_completion_interruptible()), shim_mapping_attach() unconditionally appends m back onto sf->mem_map_list with list_add_tail(), leaving duplicate fd keys in sf->mem_map_list. Second, in both qda_shim_mmap() and qda_shim_mem_map(), m is visible on the list before copy_to_user() runs. If a concurrent qda_shim_munmap() (for example, with a mismatched args.size since m->size != args.size is checked after shim_mapping_detach()) or a failing qda_shim_mem_unmap() temporarily detaches m while copy_to_user() faults, the error rollback call to shim_mapping_detach() in the map path will return NULL and skip unmapping and freeing m, after which the failing unmap path calls shim_mapping_attach() and re-attaches m even though the map ioctl returned -EFAULT. > + > +static void shim_mapping_free(struct qda_shim_mapping *m) > +{ > + drm_gem_object_put(m->obj); > + kfree(m); > +} > + > +/* The DSP unmap comes first: the buffer must outlive the DSP's use of it */ > +static void shim_mappings_release(struct qda_shim_file *sf) > +{ > + struct qda_shim_mapping *m, *tmp; > + > + list_for_each_entry_safe(m, tmp, &sf->mmap_list, node) { > + list_del(&m->node); > + qda_legacy_ops_table.munmap(sf->session, m->vaddr, m->size); > + shim_mapping_free(m); > + } > + > + list_for_each_entry_safe(m, tmp, &sf->mem_map_list, node) { > + list_del(&m->node); > + qda_legacy_ops_table.mem_unmap(sf->session, m->key, m->vaddr, > m->size); > + shim_mapping_free(m); > + } > +} [Severity: High] What happens in qda_shim_release() -> shim_mappings_release() if the file is closed after the underlying rpmsg device has been removed? When the rpmsg device is removed while a /dev/fastrpc-* file is still open, qda_rpmsg_remove() calls drm_dev_unplug(), qda_cb_unpopulate() (destroying each context-bank platform device), and qda_deinit_device() (destroying qdev->iommu_mgr->assignment_lock and qdev->ctx_xa), and devres frees qdev->iommu_mgr. In drivers/accel/qda/qda_legacy_ops.c, legacy_release() checks drm_dev_is_unplugged(&session->qda_dev->drm_dev) before calling qda_release_dsp_process(). However, qda_shim_release() calls shim_mappings_release() before legacy_release(), and neither shim_mappings_release() nor legacy_munmap() and legacy_mem_unmap() checks drm_dev_is_unplugged(). Won't closing a file with active mappings after unplug call into qda_fastrpc_invoke() -> xa_alloc(&qdev->ctx_xa, ...) and alloc_kernel_gem() -> qda_memory_manager_alloc(), dereferencing the destroyed ctx_xa, devm-freed qdev->iommu_mgr, and destroyed context-bank platform device? [ ... ] > @@ -191,6 +295,156 @@ static long qda_shim_init_create(struct qda_shim_file > *sf, void __user *argp) > return err; > } > > +static long qda_shim_mmap(struct qda_shim_file *sf, void __user *argp) > +{ > + struct qda_shim_mapping *m; > + struct fastrpc_req_mmap args; > + struct drm_gem_object *obj; > + int err; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + obj = shim_import_fd(sf->session, args.fd); > + if (IS_ERR(obj)) > + return PTR_ERR(obj); > + if (!obj) > + return -EINVAL; > + > + err = qda_legacy_ops_table.mmap(sf->session, obj, args.vaddrin, > args.size, args.flags, > + &args.vaddrout); > + if (err) { > + drm_gem_object_put(obj); > + return err; > + } > + > + m = shim_mapping_add(sf, &sf->mmap_list, args.vaddrout, args.vaddrout, > args.size, obj); [Severity: High] If user space passes args.size == 0 to FASTRPC_IOCTL_MMAP (or args.length == 0 to FASTRPC_IOCTL_MEM_MAP in qda_shim_mem_map()), doesn't this record m->size = 0 while the DSP actually mapped the full GEM object size? In setup_mmap_pages(): pages->addr = qda_gem_obj->dma_addr; pages->size = margs->size ? margs->size : qda_gem_obj->size; When args.size is 0, the DSP maps qda_gem_obj->size bytes, but qda_shim_mmap() and qda_shim_mem_map() record 0 in m->size. When the mapping is later unmapped via qda_shim_munmap(), qda_shim_mem_unmap(), copy_to_user() failure rollback, or shim_mappings_release(), the DSP is asked to unmap 0 bytes before shim_mapping_free() drops the reference on obj, and FASTRPC_IOCTL_MUNMAP calls specifying the actual buffer size will be rejected by m->size != args.size. Should m->size record the effective mapped size (or should a zero size/length be rejected)? > + if (IS_ERR(m)) { > + err = PTR_ERR(m); > + goto err_unmap; > + } > + > + if (copy_to_user(argp, &args, sizeof(args))) { > + m = shim_mapping_detach(sf, &sf->mmap_list, args.vaddrout, > args.vaddrout); > + if (m) { > + qda_legacy_ops_table.munmap(sf->session, m->vaddr, > m->size); > + shim_mapping_free(m); > + } > + return -EFAULT; > + } > + > + return 0; > + > +err_unmap: > + qda_legacy_ops_table.munmap(sf->session, args.vaddrout, args.size); > + drm_gem_object_put(obj); > + > + return err; > +} [Severity: High] Can ignoring the return value of munmap() and mem_unmap() in these rollback paths (and in shim_mappings_release()) release m->obj while the DSP still has the buffer mapped? Here in qda_shim_mmap() (on copy_to_user() failure and at err_unmap), in qda_shim_mem_map() (on copy_to_user() failure), and in shim_mappings_release() on file close, qda_legacy_ops_table.munmap() or mem_unmap() waits in qda_rpmsg_wait_for_rsp() via wait_for_completion_interruptible(&ctx->work). If a signal is pending or arrives while waiting (returning -ERESTARTSYS), or if allocating the unmap invocation context fails, the DSP has not unmapped the buffer, yet shim_mapping_free(m) or drm_gem_object_put(obj) immediately drops the reference on the GEM object (which for an imported DMA-BUF in the shim is the sole reference, tearing down its IOMMU mapping or freeing its DMA memory in qda_gem_free_object()). [ ... ] > +static long qda_shim_mem_map(struct qda_shim_file *sf, void __user *argp) > +{ > + struct qda_shim_mapping *m; > + struct fastrpc_mem_map args; > + struct drm_gem_object *obj; > + int err; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + if (args.offset < 0) > + return -EINVAL; > + > + obj = shim_import_fd(sf->session, args.fd); > + if (IS_ERR(obj)) > + return PTR_ERR(obj); > + if (!obj) > + return -EINVAL; > + > + /* Reserved before the DSP is asked, so the same fd cannot be mapped > twice */ > + m = shim_mapping_add(sf, &sf->mem_map_list, args.fd, 0, args.length, > obj); > + if (IS_ERR(m)) { > + drm_gem_object_put(obj); > + return PTR_ERR(m); > + } > + > + err = qda_legacy_ops_table.mem_map(sf->session, obj, args.flags, > args.attrs, args.offset, > + args.vaddrin, args.length, args.fd, > &args.vaddrout); > + if (err) { > + m = shim_mapping_detach(sf, &sf->mem_map_list, args.fd, 0); > + if (m) > + shim_mapping_free(m); > + return err; > + } > + > + scoped_guard(mutex, &sf->maps_lock) > + m->vaddr = args.vaddrout; [Severity: Critical] Can a concurrent FASTRPC_IOCTL_MEM_UNMAP with args.vaddr == 0 detach and free the placeholder m while qda_legacy_ops_table.mem_map() is still running? In qda_shim_mem_map(), shim_mapping_add() inserts m into sf->mem_map_list with m->key = args.fd and m->vaddr = 0, and transfers the caller's only reference on obj into m->obj before dropping sf->maps_lock: m = shim_mapping_add(sf, &sf->mem_map_list, args.fd, 0, args.length, obj); ... err = qda_legacy_ops_table.mem_map(sf->session, obj, ...); While qda_legacy_ops_table.mem_map() is executing without sf->maps_lock, a concurrent qda_shim_mem_unmap() on the same file with the same args.fd and args.vaddr == 0 will match the placeholder in shim_mapping_detach(): list_for_each_entry(m, list, node) { if (m->key == key && m->vaddr == vaddr) { list_del(&m->node); return m; } } If qda_legacy_ops_table.mem_unmap() then succeeds, qda_shim_mem_unmap() calls shim_mapping_free(m), which puts m->obj (potentially before setup_mmap_pages() has called drm_gem_object_get(args->obj)) and frees m. When qda_legacy_ops_table.mem_map() finishes and returns 0, qda_shim_mem_map() writes m->vaddr = args.vaddrout to the freed m and leaves the DSP mapping active with m->obj already released. Conversely, if qda_legacy_ops_table.mem_map() fails while the concurrent qda_shim_mem_unmap() has detached m, qda_shim_mem_map()'s cleanup shim_mapping_detach() returns NULL, and if mem_unmap() then fails and calls shim_mapping_attach(), the failed placeholder with vaddr == 0 is re-attached to sf->mem_map_list, leaking m->obj until file close and blocking future FASTRPC_IOCTL_MEM_MAP calls on that fd with -EEXIST. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
