Re: [PATCH v6 3/4] drm/virtio: implement userptr support for zero-copy memory access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/virtio: missing pointer clear in virtio_gpu_userptr_get_pages()
- [Critical] drm/virtio: unconditional dma_map_sgtable() causes data corruption
--

commit 6e836cf8f7dd384050e500d6dbf47a69c2767c26
Author: Honglei Huang <Honglei1.Huang@amd.com>
Subject: drm/virtio: implement userptr support for zero-copy memory access

This commit adds userptr blob objects so the guest kernel can pin an existing
userspace mapping and advertise it as CREATE_BLOB backing entries. It pins
pages using pin_user_pages_fast() and maps the scatterlist for the device.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c b/drivers/gpu/drm/virtio/virtgpu_userptr.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c
> @@ -0,0 +1,323 @@
[ ... ]
> +static int
> +virtio_gpu_userptr_get_pages(struct virtio_gpu_object_userptr *userptr)
> +{
[ ... ]
> +		ret = pin_user_pages_fast(userptr->start + pinned * PAGE_SIZE,
> +					  num_pages, flag,
> +					  userptr->pages + pinned);
> +
> +		if (ret < 0) {
> +			if (pinned)
> +				unpin_user_pages(userptr->pages, pinned);
> +			kvfree(userptr->pages);
> +			return ret;
> +		}

[Severity: Critical]
Does this error path leave a dangling pointer? The userptr->pages array is
freed here but not set to NULL.

If pin_user_pages_fast() fails, this bubbles up to virtio_gpu_userptr_create() 
which jumps to err_cleanup and calls virtio_gpu_cleanup_object(). That
function ultimately invokes virtio_gpu_userptr_put_pages():

	if (userptr->pages) {
		unpin_user_pages(userptr->pages, userptr->npages);
		kvfree(userptr->pages);
		userptr->pages = NULL;
	}

Because userptr->pages wasn't cleared, won't this result in a use-after-free
and double-free when the cleanup function executes?

[ ... ]
> +static int
> +virtio_gpu_userptr_get_entries(struct virtio_gpu_device *vgdev,
> +			       struct virtio_gpu_object_userptr *userptr,
> +			       struct virtio_gpu_mem_entry **ents,
> +			       unsigned int *nents)
> +{
> +	bool use_dma_api = virtio_gpu_use_dma_api(vgdev->vdev);
[ ... ]
> +	if (use_dma_api) {
> +		for_each_sgtable_dma_sg(userptr->sgt, sg, si) {
> +			(*ents)[si].addr = cpu_to_le64(sg_dma_address(sg));
> +			(*ents)[si].length = cpu_to_le32(sg_dma_len(sg));
> +			(*ents)[si].padding = 0;
> +		}
> +	} else {
> +		for_each_sgtable_sg(userptr->sgt, sg, si) {
> +			(*ents)[si].addr = cpu_to_le64(sg_phys(sg));
> +			(*ents)[si].length = cpu_to_le32(sg->length);
> +			(*ents)[si].padding = 0;
> +		}
> +	}
[ ... ]
> +int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
> +			      struct drm_file *file,
> +			      struct virtio_gpu_object_params *params,
> +			      struct virtio_gpu_object **bo_ptr)
> +{
[ ... ]
> +	sgt = drm_prime_pages_to_sg(vgdev->ddev, userptr->pages,
> +				    userptr->npages);
> +	if (IS_ERR(sgt)) {
> +		ret = PTR_ERR(sgt);
> +		goto err_cleanup;
> +	}
> +
> +	ret = dma_map_sgtable(drm_dev_dma_dev(vgdev->ddev), sgt,
> +			      DMA_BIDIRECTIONAL, 0);

[Severity: Critical]
Can this unconditional DMA mapping cause silent data corruption?

If the system uses SWIOTLB bounce buffers and the device is configured to
bypass the DMA API, the dma_map_sgtable() call will allocate a bounce buffer.

Later in virtio_gpu_userptr_get_entries(), the code branches based on
virtio_gpu_use_dma_api(), and if false it ignores the DMA address and passes
the raw physical address (sg_phys) to the device.

Since the raw physical address is passed to the device when DMA is bypassed,
the device will write directly to the original physical memory. However,
during cleanup, dma_unmap_sgtable() with DMA_BIDIRECTIONAL will copy the
unmodified bounce buffer back over the original memory. Will this overwrite
and destroy the data written by the device?

> +	if (ret) {
> +		sg_free_table(sgt);
> +		kfree(sgt);
> +		goto err_cleanup;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917102540.1312102-1-honghuan@amd.com?part=3