Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] accel/qda: unreferenced iommu_dev in qda_gem_obj causes UAF and CB reuse - [High] accel/qda: UAF on CB device unplug in qda_gem_free_object() and import - [High] accel/qda: missing vm_ops in qda_gem_object_funcs leaks GEM ref on mmap - [Medium] accel/qda: 32-bit dma_addr_t truncates SID in qda_gem_prime_import_sg_table() - [Low] accel/qda: unused gem_prime_import_sg_table hook and stale import comments - [Low] accel/qda: qda_gem_prime_import_sg_table() allocates unused shmem file --
commit c21f00e936e2fb1a3bd269969e95dd625e477ff7 Author: Ekansh Gupta <[email protected]> accel/qda: Add DMA-BUF import This commit adds DMA-BUF PRIME import and sg_table export support to the QDA accelerator driver, mapping imported buffers into the importing session's IOMMU context bank and verifying that mappings are physically or IOVA contiguous. > diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c > index 37019ff38ecbc..5e953d1f98bf4 100644 > --- a/drivers/accel/qda/qda_drv.c > +++ b/drivers/accel/qda/qda_drv.c [ ... ] > @@ -69,6 +70,9 @@ static const struct drm_driver qda_drm_driver = { > .postclose = qda_postclose, > .ioctls = qda_ioctls, > .num_ioctls = ARRAY_SIZE(qda_ioctls), > + .gem_prime_import = qda_gem_prime_import, > + .gem_prime_import_sg_table = qda_gem_prime_import_sg_table, > + .prime_fd_to_handle = qda_prime_fd_to_handle, > .name = QDA_DRIVER_NAME, > .desc = "Qualcomm DSP Accelerator Driver", > }; [Severity: Low] This isn't a bug, but is .gem_prime_import_sg_table in qda_drm_driver ever called by the DRM core? In DRM PRIME, dev->driver->gem_prime_import_sg_table is only invoked by drm_gem_prime_import_dev(). However, qda_gem_prime_import() does not call drm_gem_prime_import_dev(); it open-codes dma_buf_attach() and dma_buf_map_attachment_unlocked() and calls qda_gem_prime_import_sg_table() directly. The commit message ("The core PRIME helper takes the device to attach to, so pass the session's context bank") and the kernel-doc comment on qda_gem_prime_import_sg_table() ("Called by the DRM PRIME helpers once they have attached to the exporter...") also describe the drm_gem_prime_import_dev() flow rather than the open-coded path in qda_gem_prime_import(). Could qda_gem_prime_import() either call drm_gem_prime_import_dev(drm_dev, dma_buf, session->assigned_iommu_dev->dev) after handling the same-device check, or drop the unused hook and update the comments? > diff --git a/drivers/accel/qda/qda_gem.c b/drivers/accel/qda/qda_gem.c > index 864bac914e48d..a9ab8ccb78930 100644 > --- a/drivers/accel/qda/qda_gem.c > +++ b/drivers/accel/qda/qda_gem.c [ ... ] > @@ -24,6 +31,10 @@ static int qda_gem_mmap_obj(struct drm_gem_object > *gem_obj, struct vm_area_struc > struct qda_gem_obj *qda_gem_obj = to_qda_gem_obj(gem_obj); > int ret; > > + /* Imported buffers have to be mapped through the exporter */ > + if (drm_gem_is_imported(gem_obj)) > + return -EINVAL; > + > /* The fake offset only identifies the object, it does not index into > it */ > vma->vm_pgoff = 0; > > @@ -36,8 +47,35 @@ static int qda_gem_mmap_obj(struct drm_gem_object > *gem_obj, struct vm_area_struc > return 0; > } > > -static const struct drm_gem_object_funcs qda_gem_object_funcs = { > +/* > + * Lets another context bank map this buffer when a session imports the > + * dma-buf this object was exported as: see qda_gem_prime_import(). > + */ > +static struct sg_table *qda_gem_get_sg_table(struct drm_gem_object *gem_obj) > +{ [ ... ] > + return sgt; > +} > + > +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, > }; [Severity: High] Does mmap() on a QDA GEM object or a PRIME-exported QDA DMA-BUF leak the drm_gem_object reference and its coherent DMA buffer? Both drm_gem_mmap_obj() (for DRM fd mmap) and drm_gem_dmabuf_mmap()->drm_gem_prime_mmap() (for PRIME-exported DMA-BUF mmap, now reachable because .get_sg_table is wired up) take a reference via drm_gem_object_get(obj), set vma->vm_ops = obj->funcs->vm_ops, and call obj->funcs->mmap(obj, vma). Because qda_gem_object_funcs leaves .vm_ops unset and qda_gem_mmap_obj()->qda_dma_mmap()->dma_mmap_coherent() does not set vma->vm_ops either, vma->vm_ops remains NULL (and is replaced with dummy_vm_ops in mmap_region()). When the VMA is later unmapped, drm_gem_vm_close() is never called to drop the reference acquired at mmap time, so qda_gem_free_object() never runs and qda_gem_obj and its DMA memory are leaked. Additionally, in the PRIME DMA-BUF mmap path, drm_gem_prime_mmap() adds drm_vma_node_start(&obj->vma_node) to vma->vm_pgoff before calling obj->funcs->mmap(). Resetting vma->vm_pgoff = 0 in qda_gem_mmap_obj() instead of subtracting drm_vma_node_start(&gem_obj->vma_node) discards any non-zero page offset passed to mmap() on the exported DMA-BUF fd. Should qda_gem_object_funcs set .vm_ops to a vm_operations_struct with .open = drm_gem_vm_open and .close = drm_gem_vm_close, and adjust vma->vm_pgoff -= drm_vma_node_start(&gem_obj->vma_node) in qda_gem_mmap_obj()? > diff --git a/drivers/accel/qda/qda_gem.h b/drivers/accel/qda/qda_gem.h > index af3b9dfc75099..1dab494197aab 100644 > --- a/drivers/accel/qda/qda_gem.h > +++ b/drivers/accel/qda/qda_gem.h [ ... ] > @@ -16,14 +17,20 @@ struct qda_session; > > /** > * struct qda_gem_obj - A buffer the DSP can access > + * > + * Either allocated by this driver or imported from another one. Use > + * drm_gem_is_imported() on @base to tell which; imported buffers have no > + * @virt and own no memory, they only hold a mapping of someone else's. > */ > struct qda_gem_obj { > /** @base: DRM GEM object base; must be first member */ > struct drm_gem_object base; > /** @iommu_dev: Context bank this buffer is mapped into */ > struct qda_iommu_device *iommu_dev; > - /** @virt: Kernel virtual address of the buffer */ > + /** @virt: Kernel virtual address, NULL for imported buffers */ > void *virt; > + /** @sgt: Mapping of an imported buffer, NULL otherwise */ > + struct sg_table *sgt; > /** @dma_addr: Address of the buffer as the DSP sees it */ > dma_addr_t dma_addr; [Severity: Medium] Can qda_gem_obj->dma_addr truncate the stream ID offset on 32-bit CONFIG_COMPILE_TEST builds where dma_addr_t is 32 bits wide? In qda_gem_prime_import_sg_table(): qda_gem_obj->dma_addr = sg_dma_address(sgt->sgl) + qda_soc_sid_offset(qdev->soc_data, iommu_dev->sid); qda_soc_sid_offset() returns a u64 with sid shifted left by 32 or 56 bits, and qda_soc_dma_addr() takes a u64 iova, but struct qda_gem_obj stores dma_addr as dma_addr_t. When !CONFIG_ARCH_DMA_ADDR_T_64BIT, storing the sum into dma_addr_t drops the upper 32 bits containing the SID. Should dma_addr in struct qda_gem_obj be u64? > diff --git a/drivers/accel/qda/qda_prime.c b/drivers/accel/qda/qda_prime.c > new file mode 100644 > index 0000000000000..7a709e3000d8b > --- /dev/null > +++ b/drivers/accel/qda/qda_prime.c > @@ -0,0 +1,184 @@ [ ... ] > +struct drm_gem_object *qda_gem_prime_import_sg_table(struct drm_device > *drm_dev, > + struct dma_buf_attachment > *attach, > + struct sg_table *sgt) > +{ > + struct qda_dev *qdev = qda_dev_from_drm(drm_dev); > + size_t size = PAGE_ALIGN(attach->dmabuf->size); > + struct qda_iommu_device *iommu_dev; > + struct qda_gem_obj *qda_gem_obj; > + int ret; > + > + if (drm_prime_get_contiguous_size(sgt) < size) { > + drm_dbg_driver(drm_dev, "Rejecting non-contiguous buffer of %zu > bytes\n", size); > + return ERR_PTR(-EINVAL); > + } > + > + /* > + * The buffer was mapped into the context bank of the session doing the > + * import, so that is the bank the DSP has to be told about. > + */ > + iommu_dev = dev_get_drvdata(attach->dev); > + if (!iommu_dev) > + return ERR_PTR(-ENODEV); > + > + qda_gem_obj = kzalloc_obj(*qda_gem_obj); > + if (!qda_gem_obj) > + return ERR_PTR(-ENOMEM); > + > + ret = drm_gem_object_init(drm_dev, &qda_gem_obj->base, size); > + if (ret) { > + kfree(qda_gem_obj); > + return ERR_PTR(ret); > + } [Severity: Low] Does an imported DMA-BUF GEM object need shmem backing via drm_gem_object_init()? drm_gem_object_init() calls drm_gem_private_object_init() and then allocates a shmem file of size bytes via shmem_file_setup(). Since imported DMA-BUF objects are backed by the exporter's memory rather than shmem (and also if shmem_file_setup() fails, kfree(qda_gem_obj) is called without drm_gem_private_object_fini() to clean up _resv and gpuva.lock), should qda_gem_prime_import_sg_table() call drm_gem_private_object_init() instead? > + > + qda_gem_obj->base.funcs = &qda_gem_object_funcs; > + qda_gem_obj->size = size; > + qda_gem_obj->sgt = sgt; > + qda_gem_obj->iommu_dev = iommu_dev; > + qda_gem_obj->dma_addr = sg_dma_address(sgt->sgl) + > + qda_soc_sid_offset(qdev->soc_data, > iommu_dev->sid); > + > + return &qda_gem_obj->base; > +} > + [ ... ] > +struct drm_gem_object *qda_gem_prime_import(struct drm_device *drm_dev, > + struct dma_buf *dma_buf) > +{ > + struct qda_dev *qdev = qda_dev_from_drm(drm_dev); > + struct qda_session *session = qdev->importing_session; > + struct dma_buf_attachment *attach; > + struct drm_gem_object *obj; > + struct sg_table *sgt; > + int ret; > + > + if (!session || !session->assigned_iommu_dev) > + return ERR_PTR(-EINVAL); > + > + if (drm_gem_is_prime_exported_dma_buf(drm_dev, dma_buf)) { > + /* > + * A buffer this driver exported. The DRM core would hand back > + * the original object, which stays in the context bank of the > + * session that allocated it, while the DSP of this session > + * translates addresses in its own bank. Share the object only > + * within a bank, and map it again for any other. > + */ > + obj = dma_buf->priv; > + if (to_qda_gem_obj(obj)->iommu_dev == > session->assigned_iommu_dev) { > + drm_gem_object_get(obj); > + return obj; > + } > + } [Severity: High] What happens if a session exports a QDA GEM object as a DMA-BUF (now enabled via qda_gem_get_sg_table()) and then closes its DRM fd while the exported dma_buf fd remains open? When the exporting session closes its DRM fd: qda_postclose() -> qda_session_free() -> qda_memory_manager_release_device(session->assigned_iommu_dev) qda_memory_manager_release_device() clears iommu_dev->assigned_session = NULL and drops the session's kref on iommu_dev, even though the exported qda_gem_obj is still alive and holds an unreferenced pointer in qda_gem_obj->iommu_dev (and remains mapped in that context bank's SMMU domain). This appears to allow two failure modes: 1. If a second session opens the device, qda_memory_manager_assign_device() can assign that same iommu_dev to the new session while the first session's exported buffer is still mapped in that context bank. If the second session then imports the dma_buf, to_qda_gem_obj(obj)->iommu_dev == session->assigned_iommu_dev in qda_gem_prime_import() evaluates to true and returns the original non-imported GEM object (where drm_gem_is_imported(obj) is false), allowing the second session to mmap() the buffer through QDA as well as sharing the SMMU context bank before the old buffer is freed. 2. If the RPMsg device is removed after the exporting session closed its DRM fd while the exported dma_buf is still held open: qda_rpmsg_remove() -> qda_cb_unpopulate() -> qda_cb_teardown_device() -> qda_memory_manager_unregister_device() drops the remaining kref on iommu_dev and frees iommu_dev via kfree(). Any subsequent import/attach calling qda_gem_get_sg_table()->qda_dma_get_sgtable() or final release calling qda_gem_free_object()->qda_memory_manager_free()->qda_dma_free() will then dereference the freed qda_gem_obj->iommu_dev pointer. Should each qda_gem_obj hold a reference on iommu_dev (and keep the context bank from being reassigned until all GEM allocations/mappings in it have been released)? > + > + attach = dma_buf_attach(dma_buf, session->assigned_iommu_dev->dev); > + if (IS_ERR(attach)) > + return ERR_CAST(attach); [ ... ] > +int qda_prime_fd_to_handle(struct drm_device *drm_dev, struct drm_file > *file_priv, > + int prime_fd, u32 *handle) > +{ > + struct qda_dev *qdev = qda_dev_from_drm(drm_dev); > + struct qda_session *session = file_priv->driver_priv; > + int ret; > + > + ret = qda_memory_manager_assign_device(qdev->iommu_mgr, session); > + if (ret) > + return ret; > + > + guard(mutex)(&qdev->import_lock); > + > + qdev->importing_session = session; > + ret = drm_gem_prime_fd_to_handle(drm_dev, file_priv, prime_fd, handle); > + qdev->importing_session = NULL; > + > + return ret; > +} [Severity: High] Can qda_prime_fd_to_handle() and qda_gem_free_object() race with or access freed context bank devices after qda_rpmsg_remove()? In qda_rpmsg_remove(): drm_dev_unplug(&qdev->drm_dev); qda_cb_unpopulate(qdev); qda_deinit_device(qdev); First, drm_ioctl_kernel() only checks drm_dev_is_unplugged() at entry and does not hold a drm_dev_enter()/drm_dev_exit() critical section. Because qda_prime_fd_to_handle() does not guard its body with drm_dev_enter()/drm_dev_exit(), drm_dev_unplug() will not wait for an in-flight import to complete before qda_cb_unpopulate() destroys the context bank platform devices and qda_deinit_device() frees qdev->iommu_mgr->devices and destroys qdev->import_lock. Second, in qda_gem_prime_import(), dma_buf_attach(dma_buf, session->assigned_iommu_dev->dev) attaches the DMA-BUF to the context bank child platform_device (&entry->pdev->dev) without holding a reference that keeps the device registered. If qda_rpmsg_remove() runs while a user process has an open session with imported (or allocated) GEM objects, qda_cb_unpopulate() calls of_platform_device_destroy(&entry->pdev->dev, NULL), which unregisters and frees entry->pdev. When the user process later closes its fd, qda_gem_free_object() calls: drm_prime_gem_destroy(gem_obj, qda_gem_obj->sgt) -> dma_buf_unmap_attachment_unlocked() -> dma_unmap_sgtable(attach->dev, ...) (or qda_dma_free() -> dma_free_coherent(gem_obj->iommu_dev->dev, ...)), dereferencing the unregistered and freed context bank struct device. Should qda_prime_fd_to_handle() use drm_dev_enter()/drm_dev_exit(), and should the context bank device lifetime be tied to active GEM objects/attachments or managed via drmm so it outlives open file descriptors and GEM objects? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
