Re: [PATCH v4 09/25] accel/qda: Add legacy FastRPC shim skeleton

[sashiko-bot@kernel.org]

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 <ekansh.gupta@oss.qualcomm.com>

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