accel/qda: Qualcomm DSP Accelerator driver

drivers/misc/Kconfig config QCOM_FASTRPC · line 272

[Severity: Medium]
As reported by Sashiko on v1 [1], since DRM_ACCEL_QDA in drivers/accel/qda/Kconfig and QCOM_FASTRPC in drivers/misc/Kconfig are both tristate symbols, does "depends on !DRM_ACCEL_QDA" still allow both drivers to be built together as modules?

In Kconfig tristate logic (EXPR_NOT in scripts/kconfig/expr.h), when DRM_ACCEL_QDA=m, !DRM_ACCEL_QDA evaluates to m rather than n. That only prevents QCOM_FASTRPC=y when DRM_ACCEL_QDA=m, while still allowing QCOM_FASTRPC=m and DRM_ACCEL_QDA=m at the same time. Both modules then export the same OF device table entries in qda_rpmsg_of_match and race to bind to the same RPMsg devices.

Should this dependency instead be written as:

depends on DRM_ACCEL_QDA=n

so that QCOM_FASTRPC is disabled whenever DRM_ACCEL_QDA is enabled as either built-in or a module?

[1] https://lore.kernel.org/r/[email protected]

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

drivers/accel/qda/qda_cb.c line 23

[Severity: High]
Can qda_create_cb_device() succeed here even when the context bank device has not been attached to an IOMMU domain or claimed for kernel DMA API use?

Because the iommus property lives on the qcom,fastrpc-compute-cb child nodes rather than the parent fastrpc rpmsg node, fw_devlink only creates a DL_FLAG_SYNC_STATE_ONLY proxy link on rpdev->dev, which device_links_check_suppliers() ignores when qda_rpmsg_probe() runs.

When of_platform_device_create() calls of_device_add() -> device_add(), bus_notify(dev, BUS_NOTIFY_ADD_DEVICE) ignores the return value of iommu_bus_notifier(), and iommu_init_device() ignores the return value of dev->bus->dma_configure(dev):

drivers/iommu/iommu.c:iommu_init_device() {
	if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
		mutex_unlock(&iommu_probe_device_lock);
		dev->bus->dma_configure(dev);
		mutex_lock(&iommu_probe_device_lock);
		/* If another instance finished the job for us, skip it */
		if (!dev->iommu || dev->iommu_group)
			return -ENODEV;
	}
}

If the SMMU supplier has not probed yet, of_iommu_configure() returns -EPROBE_DEFER, causing of_dma_configure_id() to return early before arch_setup_dma_ops() sets dev->dma_coherent:

drivers/of/device.c:of_dma_configure_id() {
	ret = of_iommu_configure(dev, np, id);
	if (ret == -EPROBE_DEFER) {
		/* Don't touch range map if it wasn't set from a valid dma-ranges */
		if (set_map)
			dev->dma_range_map = NULL;
		kfree(map);
		return -EPROBE_DEFER;
	}
}

In the platform bus model, DMA and IOMMU configuration errors (such as -EPROBE_DEFER or SMMU stream mapping failures) are only propagated when a driver binds in really_probe() -> platform_dma_configure(). Since no platform driver binds to pdev, of_platform_device_create() still returns a valid pdev with pdev->dev.iommu_group == NULL, allowing the DRM device to be registered while subsequent session DMA allocations fall back to unisolated dma-direct physical mappings.

Additionally, when platform_dma_configure() is invoked from iommu_init_device() with drv == NULL, it returns before calling iommu_device_use_default_domain():

drivers/base/platform.c:platform_dma_configure() {
	/* @dev->driver may not be valid when we're called from the IOMMU layer */
	if (ret || !drv || to_platform_driver(drv)->driver_managed_dma)
		return ret;

	ret = iommu_device_use_default_domain(dev);
}

This leaves group->owner_cnt at 0, so kernel DMA API ownership of the context bank's IOMMU group is never claimed (unlike drivers/misc/fastrpc.c, which binds a platform driver to qcom,fastrpc-compute-cb devices). Should a component/sub-driver bind to these context bank platform devices, or should qda_create_cb_device() explicitly configure DMA and claim default domain ownership while propagating errors such as -EPROBE_DEFER?

drivers/accel/qda/qda_cb.c line 26

[Severity: Medium]
Should qda_create_cb_device() call dma_set_mask_and_coherent() instead of dma_set_mask() here?

When of_platform_device_create() allocates the device through of_platform_device_create_pdata() -> of_device_alloc() -> platform_device_alloc() -> setup_pdev_dma_masks(), pdev->dev.dma_mask is pointed at pdev->platform_dma_mask while pdev->dev.coherent_dma_mask is initialized separately to DMA_BIT_MASK(32):

drivers/base/platform.c:setup_pdev_dma_masks() {
	if (!pdev->dev.coherent_dma_mask)
		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
	if (!pdev->dev.dma_mask) {
		pdev->platform_dma_mask = DMA_BIT_MASK(32);
		pdev->dev.dma_mask = &pdev->platform_dma_mask;
	}
}

Because dev->dev.dma_mask is already non-NULL when of_platform_device_create_pdata() checks if (!dev->dev.dma_mask), dev->dev.dma_mask and dev->dev.coherent_dma_mask remain separate fields.

Calling dma_set_mask() only updates *pdev->dev.dma_mask (pdev->platform_dma_mask) to 34 bits on Kaanapali CDSP and leaves pdev->dev.coherent_dma_mask at DMA_BIT_MASK(32). When later patches allocate context bank buffers via dma_alloc_coherent() (in commit "accel/qda: Add buffer allocation for the DSP"), the IOMMU DMA allocator passes dev->coherent_dma_mask as the upper IOVA limit to iommu_dma_alloc_iova(), restricting coherent allocations on 34-bit CDSP context banks to 32-bit IOVAs.

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

drivers/accel/qda/qda_cb.c void qda_cb_unpopulate(struct qda_dev *qdev) · line 116

[Severity: High]
Can this leave a dangling pointer in iommu_dev->dev when the rpmsg channel goes away while a session is still open?

In qda_cb_setup_device(), iommu_dev->dev stores the raw cb_dev pointer (&pdev->dev) without taking a reference via get_device(cb_dev), and iommu_device_free() frees struct qda_iommu_device without calling put_device().

When a session owns a context bank via qda_memory_manager_assign_device(), assign_available_device() takes a second reference on iommu_dev->ref so that struct qda_iommu_device stays alive until qda_postclose() -> qda_session_free() calls qda_memory_manager_release_device().

If qda_rpmsg_remove() runs while a DRM file is open:

qda_rpmsg_remove()

    qda_cb_unpopulate()
      qda_cb_teardown_device(&entry->pdev->dev)
        qda_memory_manager_unregister_device()
          kref_put(&iommu_dev->ref, iommu_device_free)  /* ref drops 2 -> 1 */
      of_platform_device_destroy(&entry->pdev->dev, NULL)

of_platform_device_destroy() unregisters and frees entry->pdev (and its embedded struct device), while session->assigned_iommu_dev remains alive with iommu_dev->dev still pointing to the freed struct device for the remainder of the open session's lifetime.

Should qda_cb_setup_device() and iommu_device_free() manage a device reference with get_device() and put_device(), or otherwise invalidate iommu_dev->dev under synchronization when the context bank is torn down?

drivers/accel/qda/qda_drv.c struct qda_dev *qda_alloc_device(struct device *dev) · line 97

[Severity: High]
Does allocating qdev->iommu_mgr with devm_kzalloc(qdev->dev, ...) and tearing it down in qda_rpmsg_remove() -> qda_deinit_device() mismatch the lifetime of struct qda_dev?

qda_alloc_device() allocates struct qda_dev with devm_drm_dev_alloc(), which keeps qdev alive until the last drm_device reference is dropped when all open drm_file descriptors are closed. In contrast, devm_kzalloc(qdev->dev, ...) ties qdev->iommu_mgr to the rpmsg_device driver binding.

When qda_rpmsg_remove() runs while a file is open:

qda_rpmsg_remove()

    drm_dev_unplug(&qdev->drm_dev)
    qda_cb_unpopulate(qdev)
    qda_deinit_device(qdev)
      qda_memory_manager_exit(qdev->iommu_mgr)
        kfree(mem_mgr->devices)
        mutex_destroy(&mem_mgr->assignment_lock)

When qda_rpmsg_remove() returns, devres releases qdev->iommu_mgr, leaving qdev->iommu_mgr as a non-NULL dangling pointer to freed memory and a destroyed mutex while qdev and open qda_session instances are still alive. Moreover, because drm_ioctl() only performs a point-in-time drm_dev_is_unplugged() check at entry before calling the ioctl handler, any in-flight operation that passed that check before drm_dev_unplug() and calls qda_memory_manager_assign_device(qdev->iommu_mgr, session) can race with qda_memory_manager_exit() and devres teardown.

Should struct qda_memory_manager be embedded in struct qda_dev (or managed via drmm) so its lifetime and mutex match qdev->drm_dev?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

drivers/accel/qda/qda_gem.c line 42

[Severity: High]
Does qda_gem_object_funcs leak every mmap()ed qda_gem_obj and its coherent DMA buffer?

When user space maps a buffer on the accel fd, the call chain is:

  drm_gem_mmap()
    drm_gem_mmap_obj()
      drm_gem_object_get(obj);
      vma->vm_ops = obj->funcs->vm_ops;
      obj->funcs->mmap(obj, vma); -> qda_gem_mmap_obj()

drm_gem_mmap_obj() takes a reference on the GEM object for the VMA and relies on vma->vm_ops->close (normally drm_gem_vm_close) to drop that reference when the VMA is unmapped, as well as vma->vm_ops->open (drm_gem_vm_open) to take additional references when the VMA is forked or split.

Because qda_gem_object_funcs leaves .vm_ops unset and neither qda_gem_mmap_obj() nor qda_dma_mmap() initializes vma->vm_ops, drm_gem_vm_close() is never called on munmap() or process exit, so the GEM object reference count never drops to zero and qda_gem_free_object() never runs.

Should qda_gem_object_funcs set .vm_ops to a vm_operations_struct wired to drm_gem_vm_open and drm_gem_vm_close?

drivers/accel/qda/qda_gem.h line 28

[Severity: Medium]
Can storing the DSP-visible address in dma_addr_t truncate the folded stream ID bits when dma_addr_t is 32 bits wide?

In drivers/accel/qda/qda_soc.h, qda_soc_sid_offset() returns a u64 with the stream ID shifted to soc_data->sid_pos (32 on default_soc_data and 56 on kaanapali_soc_data), and qda_soc_dma_addr() takes a u64 iova.

However, struct qda_gem_obj declares dma_addr as dma_addr_t. On 32-bit builds without CONFIG_ARCH_DMA_ADDR_T_64BIT (which are permitted since DRM_ACCEL_QDA depends on ARCH_QCOM || COMPILE_TEST), assigning:

gem_obj->dma_addr = dma_addr +

                      qda_soc_sid_offset(iommu_dev->qdev->soc_data,
                                         iommu_dev->sid);

in qda_dma_alloc() truncates the upper 32 bits and discards the stream ID. Should struct qda_gem_obj::dma_addr be u64 instead of dma_addr_t?

drivers/accel/qda/qda_ioctl.c int qda_ioctl_query(struct drm_device *dev, void *data, struct drm_file *file_pr · line 52

[Severity: High]
Can qda_ioctl_gem_create() race with qda_rpmsg_remove() because it does not guard hardware/manager access with drm_dev_enter() and drm_dev_exit()?

In drivers/accel/qda/qda_rpmsg.c, qda_rpmsg_remove() runs:

  drm_dev_unplug(&qdev->drm_dev);
  qda_cb_unpopulate(qdev);
  qda_deinit_device(qdev);
    qda_memory_manager_exit(qdev->iommu_mgr);

drm_ioctl() only performs a point-in-time drm_dev_is_unplugged() check before invoking the driver ioctl callback; drm_dev_unplug() only waits for readers inside a drm_dev_enter() / drm_dev_exit() section.

If qda_rpmsg_remove() runs right after a thread enters qda_ioctl_gem_create(), drm_dev_unplug() returns immediately and qda_cb_unpopulate() and qda_memory_manager_exit() can destroy the context bank devices and free qdev->iommu_mgr->devices concurrently with qda_gem_create_object() -> qda_memory_manager_alloc() -> qda_dma_alloc().

[Severity: Medium]
Also in qda_ioctl_gem_create(), can passing args->size (__u64) to qda_gem_create_object() (which takes size_t) silently truncate sizes >= 4 GiB on 32-bit builds?

When args->size exceeds SIZE_MAX on a 32-bit kernel (for example 0x100001000ULL), the implicit conversion to size_t narrows the value to its lower 32 bits before PAGE_ALIGN(size) is checked in qda_gem_create_object(), allocating a smaller buffer than requested instead of returning -EINVAL.

drivers/accel/qda/qda_memory_dma.c line 37

[Severity: Medium]
Are coherent allocations in qda_dma_alloc() restricted to a 32-bit DMA mask even on 34-bit CDSP context banks such as kaanapali_soc_data?

In drivers/accel/qda/qda_cb.c, qda_create_cb_device() configures the context bank platform device with:

  dma_bits = qda_soc_dma_addr_bits(qdev);
  ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));

Because of_platform_device_create() initializes pdev->dev.dma_mask to &pdev->platform_dma_mask and pdev->dev.coherent_dma_mask to DMA_BIT_MASK(32), calling dma_set_mask() only updates *pdev->dev.dma_mask and leaves pdev->dev.coherent_dma_mask at 32 bits.

When qda_dma_alloc() calls dma_alloc_coherent(), the IOMMU DMA allocator bounds the IOVA using iommu_dev->dev->coherent_dma_mask rather than *iommu_dev->dev->dma_mask, so 34-bit CDSP context banks on Kaanapali (where dma_addr_bits_cdsp is 34) remain limited to 32-bit IOVAs. Should qda_create_cb_device() call dma_set_mask_and_coherent() instead?

drivers/accel/qda/qda_memory_dma.c line 54

[Severity: Critical]
What happens if a qda_gem_obj outlives the drm_file session that created it?

A GEM object can stay alive after its drm_file is closed, for example if user space exports it to a dma-buf fd via DRM_IOCTL_PRIME_HANDLE_TO_FD (enabled by DRIVER_GEM) or keeps a VMA mapped after closing the accel fd.

When the drm_file closes, the teardown path runs:

  qda_postclose()
    qda_session_free()
      qda_memory_manager_release_device(session->assigned_iommu_dev)
        iommu_dev->assigned_session = NULL;
        kref_put(&iommu_dev->ref, iommu_device_free);

Because qda_dma_alloc() stores gem_obj->iommu_dev without holding a reference on iommu_dev or keeping the context bank reserved while buffers are still allocated from it:

1. Setting iommu_dev->assigned_session = NULL allows qda_memory_manager_assign_device() to hand the same context bank to a new session while the previous session's DMA buffer is still mapped into that context bank's IOMMU domain.

2. If the context bank is also unregistered via qda_cb_teardown_device() -> qda_memory_manager_unregister_device(), iommu_dev->ref drops to zero and iommu_device_free() frees iommu_dev, causing a use-after-free in qda_dma_free() (and qda_dma_mmap()) when it later dereferences gem_obj->iommu_dev->dev and gem_obj->iommu_dev->qdev->soc_data.

[Severity: High]
Similarly, in qda_dma_free(), can gem_obj->iommu_dev->dev be a dangling pointer if the RPMsg device is removed while a user process still holds an open session or GEM buffer?

In drivers/accel/qda/qda_cb.c, qda_cb_unpopulate() runs during qda_rpmsg_remove():

  qda_cb_teardown_device(&entry->pdev->dev);
  of_platform_device_destroy(&entry->pdev->dev, NULL);

While struct qda_iommu_device itself is refcounted via iommu_dev->ref so that it can survive qda_cb_teardown_device(), of_platform_device_destroy() immediately unregisters and frees entry->pdev (which is iommu_dev->dev). When the user process later closes its fd or frees the GEM handle, qda_gem_free_object() -> qda_memory_manager_free() -> qda_dma_free() passes the freed gem_obj->iommu_dev->dev to dma_free_coherent().

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

drivers/accel/qda/qda_drv.c static const struct drm_driver qda_drm_driver = { · line 78

[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?

drivers/accel/qda/qda_gem.c static int qda_gem_mmap_obj(struct drm_gem_object *gem_obj, struct vm_area_struc · line 80

[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()?

drivers/accel/qda/qda_gem.h struct qda_session; · line 35

[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?

drivers/accel/qda/qda_prime.c line 65

[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?

drivers/accel/qda/qda_prime.c line 118

[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)?

drivers/accel/qda/qda_prime.c line 182

[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

drivers/accel/qda/qda_drv.c static int qda_open(struct drm_device *dev, struct drm_file *file) · line 64

[Severity: Low]
This isn't a bug, but why are qda_session_alloc() and qda_session_free() exported with EXPORT_SYMBOL_FOR_MODULES(..., "qda_fastrpc_shim") in drivers/accel/qda/qda_drv.c?

Since qda_legacy_ops.o is linked into the same qda module via qda-y in drivers/accel/qda/Makefile and exposes session creation and destruction through qda_legacy_ops_table (where legacy_session_create() and legacy_session_destroy() also manage the drm_dev reference), does qda_fastrpc_shim ever need to call the raw qda_session_alloc() and qda_session_free() helpers directly?

[ ... ]

drivers/accel/qda/qda_legacy_ops.c line 38

[Severity: High]
What happens if a DMA-BUF allocated by legacy_alloc_dmabuf() (or a GEM object imported by legacy_import_dmabuf()) outlives the session when legacy_session_destroy() is called in drivers/accel/qda/qda_legacy_ops.c? Note that on normal process exit, close_files() closes file descriptors in ascending order, so an earlier /dev/fastrpc-* fd will be closed before DMA-BUF fds allocated later from that session.

When legacy_alloc_dmabuf() creates a buffer and exports it via drm_gem_prime_export(), the resulting dma_buf holds a reference to gem_obj and qdev->drm_dev, and gem_obj->iommu_dev points to session->assigned_iommu_dev. However, neither qda_dma_alloc() nor qda_gem_prime_import_sg_table() takes a reference on iommu_dev->ref or on the session.

When legacy_session_destroy() runs while the exported DMA-BUF is still alive:

legacy_session_destroy()
-> qda_session_free()

       -> qda_memory_manager_release_device(session->assigned_iommu_dev)
            iommu_dev->assigned_session = NULL;
            kref_put(&iommu_dev->ref, iommu_device_free);

First, clearing iommu_dev->assigned_session allows assign_available_device() in drivers/accel/qda/qda_memory_manager.c to hand the same context bank to a new session while the old session's DMA-BUF remains allocated and mapped in that context bank's SMMU domain, and causes qda_gem_prime_import() (which checks to_qda_gem_obj(obj)->iommu_dev == session->assigned_iommu_dev) to treat the previous session's buffer as belonging to the new session.

Second, if qda_rpmsg_remove() subsequently runs and unregisters the context bank via qda_cb_unpopulate() -> qda_cb_teardown_device() -> qda_memory_manager_unregister_device(), iommu_dev->ref drops to zero and iommu_device_free() frees struct qda_iommu_device. When the DMA-BUF fd is later closed, drm_gem_dmabuf_release() -> qda_gem_free_object() -> qda_memory_manager_free() -> qda_dma_free() in drivers/accel/qda/qda_memory_dma.c dereferences the freed gem_obj->iommu_dev:

void qda_dma_free(struct qda_gem_obj *gem_obj)

{
	dma_free_coherent(gem_obj->iommu_dev->dev, gem_obj->size,
			  gem_obj->virt, qda_gem_dma_addr(gem_obj));

Can this lead to a use-after-free of gem_obj->iommu_dev as well as premature context bank reuse?

drivers/accel/qda/qda_legacy_ops.c line 62

[Severity: High]
Does mmap() on a DMA-BUF returned by legacy_alloc_dmabuf() leak gem_obj and its backing DMA allocation?

When user space maps the exported DMA-BUF fd, the call chain is:

dma_buf_mmap()
-> drm_gem_dmabuf_mmap()

       -> drm_gem_prime_mmap()

In drm_gem_prime_mmap():

	vma->vm_ops = obj->funcs->vm_ops;
	drm_gem_object_get(obj);
	ret = obj->funcs->mmap(obj, vma);

However, in drivers/accel/qda/qda_gem.c, qda_gem_object_funcs does not set .vm_ops:

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,
};

And qda_gem_mmap_obj() -> qda_dma_mmap() -> dma_mmap_coherent() does not set vma->vm_ops either. Because vma->vm_ops remains NULL instead of using drm_gem_vm_open and drm_gem_vm_close, unmapping the VMA never calls drm_gem_vm_close() to drop the reference taken by drm_gem_object_get(obj). Won't every mmap() call permanently leak the qda_gem_obj, its coherent DMA buffer, and the associated drm_device reference?

[ ... ]

drivers/accel/qda/qda_rpmsg.c static void qda_rpmsg_remove(struct rpmsg_device *rpdev) · line 58

[Severity: High]
In legacy_session_create(), drm_dev_get(&qdev->drm_dev) is taken so that qdev survives qda_rpmsg_remove() while a legacy session remains open. However, can legacy_alloc_dmabuf(), legacy_import_dmabuf() (via qda_prime_import_dmabuf()), or later buffer cleanup trigger a use-after-free if they run concurrently with or after qda_rpmsg_remove()?

In drivers/accel/qda/qda_rpmsg.c, qda_rpmsg_remove() does:

	qda_unregister_device(qdev);
	drm_dev_unplug(&qdev->drm_dev);
	qdev->rpdev = NULL;
	qda_cb_unpopulate(qdev);
	qda_deinit_device(qdev);

1. In qda_init_device(), qdev->iommu_mgr is allocated with devm_kzalloc(qdev->dev, ...), which is managed by the rpmsg device rather than &qdev->drm_dev. When qda_rpmsg_remove() finishes, devres frees qdev->iommu_mgr even though qdev->drm_dev is still held by the open legacy session.

2. qda_deinit_device() frees iommu_mgr->devices and destroys both iommu_mgr->assignment_lock and qdev->import_lock, while qda_cb_unpopulate() calls of_platform_device_destroy() on each context bank child device (which qda_cb_setup_device() stored in iommu_dev->dev without taking a device reference).

3. Neither legacy_alloc_dmabuf() nor qda_prime_import_dmabuf() enters a drm_dev_enter() / drm_dev_exit() critical section, so drm_dev_unplug() does not synchronize with them. If legacy_alloc_dmabuf() or qda_prime_import_dmabuf() is called on an already-open legacy session during or after qda_rpmsg_remove(), won't it dereference the freed qdev->iommu_mgr, lock the destroyed assignment_lock and import_lock mutexes, and pass the destroyed iommu_dev->dev to dma_alloc_coherent() or dma_buf_attach()? And even if buffers were allocated before qda_rpmsg_remove() and only freed afterwards, won't qda_gem_free_object() pass the destroyed iommu_dev->dev to dma_free_coherent() or dma_buf_unmap_attachment_unlocked()?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

drivers/accel/qda/qda_fastrpc_shim.c line 84

[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?

drivers/accel/qda/qda_fastrpc_shim.c line 102

[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.

[ ... ]

drivers/accel/qda/qda_fastrpc_shim.c line 151

[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

drivers/accel/qda/qda_drv.c int qda_init_device(struct qda_dev *qdev, int num_cbs) · line 129

[Severity: High]
Can calling xa_destroy(&qdev->ctx_xa) in qda_deinit_device() in drivers/accel/qda/qda_drv.c race with outstanding invocation contexts that have not yet dropped their final reference?

In qda_fastrpc_context_alloc() in drivers/accel/qda/qda_fastrpc.c, each struct qda_fastrpc_invoke_ctx is inserted into qdev->ctx_xa with an initial caller reference (refcount = 1) and stores a raw pointer ctx->qdev = qdev without holding a reference on qdev->drm_dev. By design, the entry is only erased from qdev->ctx_xa in qda_fastrpc_context_free() when the last reference is released.

When invocations are wired up in the subsequent commits ("accel/qda: Add RPMsg send and response dispatch" and "accel/qda: Add the invocation ioctl and teardown on unplug"), qda_rpmsg_send_msg() takes a second reference for the response path and the caller sleeps in qda_rpmsg_wait_for_rsp() outside any drm_dev_enter() critical section.

If the rpmsg device is removed while callers are waiting (for example, on a DSP crash or remoteproc restart), qda_rpmsg_remove() runs drm_dev_unplug(), qda_fastrpc_flush_pending(qdev), and then immediately qda_deinit_device(qdev). Because qda_fastrpc_flush_pending() only wakes the waiters and drops the response reference, the callers still hold their reference (and if qda_rpmsg_cb() already ran and queued ctx->put_work, ctx is still in qdev->ctx_xa while put_work is pending, causing qda_fastrpc_flush_pending() to drop a reference that put_work will also drop).

When qda_deinit_device() immediately calls xa_destroy(&qdev->ctx_xa), the internal XArray nodes are freed while live contexts remain in the table. When those woken callers or pending put_work items later run kref_put(&ctx->refcount, qda_fastrpc_context_free), won't qda_fastrpc_context_free() call xa_erase(&ctx->qdev->ctx_xa, ...) on an already-destroyed XArray and dereference ctx->qdev after device teardown?

drivers/accel/qda/qda_fastrpc.c line 60

[Severity: High]
In qda_fastrpc_context_alloc() in drivers/accel/qda/qda_fastrpc.c, does calling xa_alloc(&qdev->ctx_xa, &id, ctx, ...) publish ctx into qdev->ctx_xa and drop xa_lock before ctx->ctxid is initialized?

Because the 8-bit context IDs (1..255) are reused as soon as a prior invocation is freed, a concurrent lookup on qdev->ctx_xa for that recycled id (such as qda_rpmsg_cb() handling a late, duplicate, or malformed DSP response, or qda_fastrpc_flush_pending() during device unplug) can observe ctx while ctx->ctxid is still 0 or is being written without locking:

1. If a late or malformed response carries sequence number 0 in bits [63:16] for slot id, FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, ctx->ctxid) evaluates to 0 and matches the uninitialized ctx->ctxid, completing ctx->work early and scheduling ctx->put_work to drop a reference that qda_rpmsg_send_msg() has not yet taken.

2. If qda_fastrpc_context_free() runs while ctx->ctxid is still 0, xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, ctx->ctxid)) erases index 0 instead of index id and frees ctx, leaving a dangling pointer in qdev->ctx_xa[id].

Should qda_fastrpc_context_alloc() either hold xa_lock(&qdev->ctx_xa) across __xa_alloc() and the ctx->ctxid assignment, or reserve the slot with NULL first and store ctx only after ctx->ctxid is initialized?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10

drivers/accel/qda/qda_rpmsg.c static struct qda_dev *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device *rpdev) · line 118

[Severity: High]
When a protection domain exits on the DSP, the firmware sends a PD exit notification carrying the sentinel value 0xABCDABCD in resp_msg->ctx (which drivers/misc/fastrpc.c filters out via FASTRPC_DSP_PD_NOTIFY_CTX in fastrpc_rpmsg_callback()).

Under the QDA context ID layout, FIELD_GET(QDA_FASTRPC_CTXID_MASK, 0xABCDABCD) evaluates to 0xCD (205) and FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, 0xABCDABCD) evaluates to 0xABCD. If a PD exit notification arrives while context slot 0xCD is occupied by an invocation at sequence number 0xABCD, won't qda_rpmsg_cb() mistakenly match it, overwrite ctx->retval, wake the caller early via complete(&ctx->work) while the DSP is still accessing its DMA buffers, and schedule ctx->put_work?

Additionally, when slot 0xCD is empty, returning -ENOENT here instead of 0 causes SMD-backed RPMsg channels (qcom_smd_channel_recv_single() in drivers/rpmsg/qcom_smd.c) to stop advancing the RX FIFO tail. Should qda_rpmsg_cb() filter out 0xABCDABCD early and return 0 when dropping unmatched responses?

drivers/accel/qda/qda_rpmsg.c static struct qda_dev *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device *rpdev) · line 130

[Severity: High]
Can this unlocked xa_load() race with context teardown and cause a use-after-free in qda_rpmsg_cb()?

When the final reference on ctx drops to zero on another CPU (for example, when qda_rpmsg_send_msg() fails and drops the callback reference followed by the caller dropping its reference, or when an invocation finishes), qda_fastrpc_context_free() removes ctx from qdev->ctx_xa and immediately frees it without an RCU grace period:

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_free() {
	struct qda_fastrpc_invoke_ctx *ctx =
		container_of(ref, struct qda_fastrpc_invoke_ctx, refcount);

	xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, ctx->ctxid));
	kfree(ctx);
}

Because xa_load(&qdev->ctx_xa, ...) only holds rcu_read_lock() internally during the lookup and returns the raw ctx pointer without holding qdev->ctx_xa.xa_lock or pinning ctx->refcount via kref_get_unless_zero(), if a late or duplicate response arrives in qda_rpmsg_cb() just before xa_erase() and kfree(ctx), qda_rpmsg_cb() will dereference freed memory at ctx->ctxid, write ctx->retval, and call complete(&ctx->work) and schedule_work(&ctx->put_work) on the freed object.

Similarly, in qda_fastrpc_context_alloc(), xa_alloc() publishes ctx into qdev->ctx_xa before ctx->ctxid is initialized:

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_alloc() {
	...
	ret = xa_alloc(&qdev->ctx_xa, &id, ctx, XA_LIMIT(1, QDA_FASTRPC_CTXID_MAX),
		       GFP_KERNEL);
	if (ret) {
		kfree(ctx);
		return ERR_PTR(ret);
	}

	ctx->ctxid = FIELD_PREP(QDA_FASTRPC_CTXID_MASK, id) |
		     FIELD_PREP(QDA_FASTRPC_CTXID_SEQ_MASK,
				(u64)atomic64_inc_return(&qdev->ctx_seq));
}

which briefly exposes ctx with ctx->ctxid == 0 to concurrent lookups in qda_rpmsg_cb().

drivers/accel/qda/qda_rpmsg.c static struct qda_dev *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device *rpdev) · line 140

[Severity: High]
What prevents a duplicate or unsolicited DSP response from scheduling ctx->put_work without a corresponding callback reference and underflowing ctx->refcount?

In qda_fastrpc_context_alloc(), ctx is inserted into qdev->ctx_xa with refcount = 1 (owned by the caller), and qda_rpmsg_send_msg() takes a single additional reference via kref_get(&ctx->refcount) for the response handler. When a valid response arrives in qda_rpmsg_cb(), ctx is neither removed from qdev->ctx_xa nor atomically marked as already answered; it stays in qdev->ctx_xa until qda_fastrpc_context_free() runs at refcount == 0.

Once the first put_work begins executing in qda_fastrpc_context_put_wq() and drops refcount from 2 to 1 while the caller still holds its reference, a duplicate response from the DSP carrying the same ctx value (matching both the 8-bit ID and the sequence number) will match ctx in qda_rpmsg_cb() a second time and call schedule_work(&ctx->put_work) again. That second execution of qda_fastrpc_context_put_wq() drops the caller's reference, freeing ctx while the caller is still using it and triggering a refcount underflow when the caller drops its own reference.

Likewise, because ctx is in qdev->ctx_xa before qda_rpmsg_send_msg() is called (and remains in qdev->ctx_xa after rpmsg_send() fails and drops the callback reference), an unsolicited response matching ctx->ctxid before send or after a failed send will also schedule ctx->put_work without a matching callback reference.

drivers/accel/qda/qda_rpmsg.c static struct qda_dev *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device *rpdev) · line 148

[Severity: High]
If the RPMsg device is removed while ctx->put_work is queued or while invocations are in flight, can qda_fastrpc_context_put_wq() access qdev->ctx_xa after qda_deinit_device() has destroyed it?

In qda_rpmsg_cb(), schedule_work(&ctx->put_work) queues qda_fastrpc_context_put_wq() on system_percpu_wq and immediately exits the drm_dev_enter() critical section via drm_dev_exit(idx). When qda_rpmsg_remove() runs:

drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_remove() {
	...
	drm_dev_unplug(&qdev->drm_dev);
	qdev->rpdev = NULL;
	qda_cb_unpopulate(qdev);
	qda_deinit_device(qdev);
}

drm_dev_unplug() only waits for active drm_dev_enter() sections; it does not flush or wait for pending ctx->put_work items before qda_deinit_device(qdev) destroys qdev->ctx_xa via xa_destroy(&qdev->ctx_xa) (and if all DRM files are closed, devm frees qdev when qda_rpmsg_remove() returns). When the queued qda_fastrpc_context_put_wq() worker subsequently runs, qda_fastrpc_context_free() calls xa_erase(&ctx->qdev->ctx_xa, ...) on the destroyed XArray and freed qdev.

In addition, if the RPMsg device is removed in this commit while invocations sent via qda_rpmsg_send_msg() are waiting for a response, qda_rpmsg_remove() does not complete ctx->work to wake waiters in qda_rpmsg_wait_for_rsp() or drop the callback references on entries remaining in qdev->ctx_xa before xa_destroy(). Although a later commit in the series, commit 9e89a77ec2ad ("accel/qda: Add the invocation ioctl and teardown on unplug"), adds qda_fastrpc_flush_pending(qdev) to qda_rpmsg_remove() to wake and put remaining entries in ctx_xa, it still does not flush pending ctx->put_work items that were already scheduled by qda_rpmsg_cb().

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11