Re: [PATCH v4 08/25] accel/qda: Add legacy ops base and auxiliary device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] accel/qda: use-after-free of iommu_dev when DMA-BUF outlives session
- [High] accel/qda: use-after-free in legacy ops after qda_rpmsg_remove()
- [High] accel/qda: GEM object leak on mmap() due to missing vm_ops
- [Low] accel/qda: unnecessary export of qda_session_alloc() and qda_session_free()
--

commit c10272995846f83dffe56b2be6fa2691d606459a
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

accel/qda: Add legacy ops base and auxiliary device

This commit creates an auxiliary device per DSP domain for the legacy FastRPC
compatibility module to bind to and exports a table of operations
(qda_legacy_ops_table) for session lifecycle and DMA-BUF allocation and
import. Sessions created by the legacy module hold a drm_device reference and
operate on DMA-BUFs and GEM objects directly without a drm_file.

> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c
> index 5e953d1f98bf4..21113deceaadb 100644
> --- a/drivers/accel/qda/qda_drv.c
> +++ b/drivers/accel/qda/qda_drv.c
> @@ -14,7 +15,13 @@
>  #include "qda_ioctl.h"
>  #include "qda_prime.h"
>  
> -static struct qda_session *qda_session_alloc(struct qda_dev *qdev)
> +/**
> + * qda_session_alloc() - Create a session on a device
> + * @qdev: Device to create the session on
> + *
> + * Return: The new session, or ERR_PTR on failure
> + */
> +struct qda_session *qda_session_alloc(struct qda_dev *qdev)
>  {
>  	struct qda_session *session;
>  
> @@ -26,6 +33,7 @@ static struct qda_session *qda_session_alloc(struct qda_dev *qdev)
>  
>  	return session;
>  }
> +EXPORT_SYMBOL_FOR_MODULES(qda_session_alloc, "qda_fastrpc_shim");
>  
>  static int qda_open(struct drm_device *dev, struct drm_file *file)
>  {
> @@ -42,13 +50,18 @@ static int qda_open(struct drm_device *dev, struct drm_file *file)
>  	return 0;
>  }
>  
> -static void qda_session_free(struct qda_session *session)
> +/**
> + * qda_session_free() - Release a session and whatever it still holds
> + * @session: Session to release
> + */
> +void qda_session_free(struct qda_session *session)
>  {
>  	if (session->assigned_iommu_dev)
>  		qda_memory_manager_release_device(session->assigned_iommu_dev);
>  
>  	kfree(session);
>  }
> +EXPORT_SYMBOL_FOR_MODULES(qda_session_free, "qda_fastrpc_shim");

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

[ ... ]

> diff --git a/drivers/accel/qda/qda_legacy_ops.c b/drivers/accel/qda/qda_legacy_ops.c
> new file mode 100644
> index 0000000000000..3566a10c64880
> --- /dev/null
> +++ b/drivers/accel/qda/qda_legacy_ops.c
> @@ -0,0 +1,80 @@

[ ... ]

> +static struct qda_session *legacy_session_create(struct qda_dev *qdev)
> +{
> +	struct qda_session *session;
> +
> +	session = qda_session_alloc(qdev);
> +	if (!IS_ERR(session))
> +		drm_dev_get(&qdev->drm_dev);
> +
> +	return session;
> +}
> +
> +static void legacy_session_destroy(struct qda_session *session)
> +{
> +	struct drm_device *drm_dev = &session->qda_dev->drm_dev;
> +
> +	qda_session_free(session);
> +	drm_dev_put(drm_dev);
> +}

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

> +
> +/*
> + * As the GEM_CREATE ioctl, but exporting a DMA-BUF rather than installing a
> + * handle: a session the shim created has no DRM file to hold handles, and the
> + * legacy interface hands buffers to user space as file descriptors anyway.
> + */
> +static struct dma_buf *legacy_alloc_dmabuf(struct qda_session *session, u64 size)
> +{
> +	struct qda_dev *qdev = session->qda_dev;
> +	struct drm_gem_object *gem_obj;
> +	struct dma_buf *dmabuf;
> +
> +	gem_obj = qda_gem_create_object(&qdev->drm_dev, qdev->iommu_mgr, size, session);
> +	if (IS_ERR(gem_obj))
> +		return ERR_CAST(gem_obj);
> +
> +	/* O_RDWR so the fd can be mapped writable */
> +	dmabuf = drm_gem_prime_export(gem_obj, O_RDWR);
> +
> +	/* The DMA-BUF took its own reference if the export succeeded */
> +	drm_gem_object_put(gem_obj);
> +
> +	return dmabuf;
> +}

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

[ ... ]

> diff --git a/drivers/accel/qda/qda_prime.c b/drivers/accel/qda/qda_prime.c
> index 7a709e3000d8b..128d4ab2cf64f 100644
> --- a/drivers/accel/qda/qda_prime.c
> +++ b/drivers/accel/qda/qda_prime.c
> @@ -181,4 +181,37 @@ int qda_prime_fd_to_handle(struct drm_device *drm_dev, struct drm_file *file_pri
>  	return ret;
>  }
>  
> +/**
> + * qda_prime_import_dmabuf() - Import a DMA-BUF without installing a handle
> + * @drm_dev: DRM device
> + * @session: Session to import into
> + * @dma_buf: Buffer to import
> + *
> + * As qda_prime_fd_to_handle(), but stopping at the GEM object: a session the
> + * legacy shim created has no DRM file to install a handle in. The caller owns
> + * the returned reference.
> + *
> + * Return: The new GEM object on success, ERR_PTR on failure
> + */
> +struct drm_gem_object *qda_prime_import_dmabuf(struct drm_device *drm_dev,
> +					       struct qda_session *session,
> +					       struct dma_buf *dma_buf)
> +{
> +	struct qda_dev *qdev = qda_dev_from_drm(drm_dev);
> +	struct drm_gem_object *gem_obj;
> +	int ret;
> +
> +	ret = qda_memory_manager_assign_device(qdev->iommu_mgr, session);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	guard(mutex)(&qdev->import_lock);
> +
> +	qdev->importing_session = session;
> +	gem_obj = qda_gem_prime_import(drm_dev, dma_buf);
> +	qdev->importing_session = NULL;
> +
> +	return gem_obj;
> +}
> +
>  MODULE_IMPORT_NS("DMA_BUF");
> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c
> index 2c9d79937b106..14034e250555d 100644
> --- a/drivers/accel/qda/qda_rpmsg.c
> +++ b/drivers/accel/qda/qda_rpmsg.c
> @@ -50,6 +50,9 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev)
>  {
>  	struct qda_dev *qdev = dev_get_drvdata(&rpdev->dev);
>  
> +	/* Take the legacy nodes away first, so nothing new opens during teardown */
> +	qda_unregister_device(qdev);
> +
>  	/*
>  	 * Unplug before clearing @rpdev: once there are ioctls that reach the
>  	 * DSP, this is what stops a new one from starting and waits for the

[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/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=8