Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] accel/qda: unplug race in qda_shim_ioctl() due to missing drm_dev_enter() - [Critical] accel/qda: UAF and CB reuse when DMA-BUF outlives qda_shim_release() - [High] accel/qda: GEM object leak on DMA-BUF mmap() due to NULL vm_ops --
commit a13f2a6988b9b1d52b35c9e30bbc2ea3fc109ecb Author: Ekansh Gupta <[email protected]> accel/qda: Add legacy FastRPC shim skeleton This commit adds a separate compatibility module that exposes /dev/fastrpc-* misc device nodes per DSP domain on top of the QDA driver and implements the FASTRPC_IOCTL_ALLOC_DMA_BUFF ioctl by exporting buffers as DMA-BUF file descriptors. > diff --git a/drivers/accel/qda/qda_fastrpc_shim.c > b/drivers/accel/qda/qda_fastrpc_shim.c > new file mode 100644 > index 0000000000000..25f5c242efde1 > --- /dev/null > +++ b/drivers/accel/qda/qda_fastrpc_shim.c [ ... ] > +static int qda_shim_release(struct inode *inode, struct file *file) > +{ > + struct qda_shim_file *sf = file->private_data; > + > + qda_legacy_ops_table.session_destroy(sf->session); > + kfree(sf); > + > + return 0; > +} [Severity: Critical] What happens if a DMA-BUF fd returned by qda_shim_alloc_dma_buf() remains open after the /dev/fastrpc-* file descriptor is closed (which also occurs on normal process exit when close_files() closes fds in ascending numerical order)? When qda_shim_alloc_dma_buf() allocates a buffer via legacy_alloc_dmabuf() -> qda_gem_create_object() -> qda_memory_manager_alloc(), a context bank (struct qda_iommu_device) is assigned to sf->session->assigned_iommu_dev and qda_dma_alloc() stores a raw pointer to it in gem_obj->iommu_dev: drivers/accel/qda/qda_memory_dma.c:qda_dma_alloc() { ... gem_obj->virt = virt; gem_obj->dma_addr = dma_addr + qda_soc_sid_offset(iommu_dev->qdev->soc_data, iommu_dev->sid); gem_obj->iommu_dev = iommu_dev; ... } Closing the /dev/fastrpc-* fd calls qda_shim_release() -> legacy_session_destroy() -> qda_session_free() -> qda_memory_manager_release_device(session->assigned_iommu_dev), which clears iommu_dev->assigned_session = NULL and drops the session's kref on iommu_dev->ref while the exported DMA-BUF is still allocated and mapped in that context bank's SMMU domain. Doesn't that allow another session to be assigned the same context bank while the buffer is still live, and also lead to a use-after-free of gem_obj->iommu_dev and gem_obj->iommu_dev->dev in qda_dma_free(), qda_dma_mmap(), or qda_dma_get_sgtable() if qda_rpmsg_remove() tears down the context bank devices before the DMA-BUF fd is closed? > +static long qda_shim_alloc_dma_buf(struct qda_shim_file *sf, void __user > *argp) > +{ > + struct fastrpc_alloc_dma_buf args; > + struct dma_buf *dmabuf; > + int fd; > + > + if (copy_from_user(&args, argp, sizeof(args))) > + return -EFAULT; > + > + /* The size is 64 bits wide on the wire but a size_t further down */ > + if (args.size > SIZE_MAX) > + return -EINVAL; > + > + /* args.flags is not used, as in drivers/misc/fastrpc.c */ > + dmabuf = qda_legacy_ops_table.alloc_dmabuf(sf->session, args.size); > + if (IS_ERR(dmabuf)) > + return PTR_ERR(dmabuf); [Severity: High] Does mapping the DMA-BUF fd returned here leak the underlying drm_gem_object and coherent DMA buffer? qda_shim_alloc_dma_buf() calls legacy_alloc_dmabuf(), which creates a GEM object with qda_gem_object_funcs and exports it via drm_gem_prime_export(): drivers/accel/qda/qda_gem.c: const struct drm_gem_object_funcs qda_gem_object_funcs = { .free = qda_gem_free_object, .get_sg_table = qda_gem_get_sg_table, .mmap = qda_gem_mmap_obj, }; When user space calls mmap() on the returned DMA-BUF fd, drm_gem_dmabuf_mmap() -> drm_gem_prime_mmap() sets vma->vm_ops = obj->funcs->vm_ops, calls drm_gem_object_get(obj), and invokes obj->funcs->mmap(obj, vma). Because qda_gem_object_funcs leaves .vm_ops NULL (instead of setting drm_gem_vm_open and drm_gem_vm_close) and qda_gem_mmap_obj() does not set vma->vm_ops either, nothing drops the GEM object reference when the VMA is unmapped. [ ... ] > +static long qda_shim_ioctl(struct file *file, unsigned int cmd, unsigned > long arg) > +{ > + struct qda_shim_file *sf = file->private_data; > + > + if (drm_dev_is_unplugged(&sf->session->qda_dev->drm_dev)) > + return -ENODEV; > + > + return qda_shim_dispatch(sf, cmd, (void __user *)arg); > +} [Severity: Critical] Can qda_shim_ioctl() race with qda_rpmsg_remove() and access freed memory? drm_dev_is_unplugged() enters the SRCU read-side critical section via drm_dev_enter() and immediately exits it via drm_dev_exit() before returning false. As a result, qda_shim_dispatch() -> qda_shim_alloc_dma_buf() -> legacy_alloc_dmabuf() -> qda_gem_create_object() executes without holding drm_dev_enter(). If qda_rpmsg_remove() runs concurrently: drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_remove() { ... qda_unregister_device(qdev); drm_dev_unplug(&qdev->drm_dev); qdev->rpdev = NULL; qda_cb_unpopulate(qdev); qda_deinit_device(qdev); } misc_deregister() in qda_unregister_device() only serializes against open() and does not wait for in-flight ioctls on already-open /dev/fastrpc-* files, and drm_dev_unplug() returns without waiting for qda_shim_alloc_dma_buf() because the SRCU read lock was already dropped. qda_cb_unpopulate() and qda_deinit_device() then destroy the context bank platform devices, free qdev->iommu_mgr->devices, and destroy qdev->iommu_mgr->assignment_lock, and devres frees qdev->iommu_mgr (which was allocated with devm_kzalloc(qdev->dev, ...) in qda_init_device()) while the concurrent qda_shim_alloc_dma_buf() thread is still accessing qdev->iommu_mgr and allocating from the context bank device. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
