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 <[email protected]> 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/[email protected]?part=3
