Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] mm: silent data loss due to missing dirty tracking on unpin - [Critical] virtgpu: privilege escalation via read-write PRIME export of read-only userptrs - [High] virtgpu: missing DMA sync for userptr objects causes stale reads --
commit 419406d5baa93528ec030b7b95cc7d5ada190751 Author: Honglei Huang <[email protected]> drm/virtio: implement userptr support for zero-copy memory access This commit implements userptr support for virtio-gpu by adding a new virtio_gpu_object_userptr type. It pins user pages via pin_user_pages_fast() and maps them via the DMA API when required. > diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c > b/drivers/gpu/drm/virtio/virtgpu_userptr.c > new file mode 100644 > index 0000000000000..009755489774c > --- /dev/null > +++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c > @@ -0,0 +1,335 @@ [ ... ] > +static const struct drm_gem_object_funcs virtio_gpu_userptr_funcs = { > + .open = virtio_gpu_gem_object_open, > + .close = virtio_gpu_gem_object_close, > + .free = virtio_gpu_userptr_free, > + .export = virtgpu_gem_prime_export, > + .get_sg_table = virtio_gpu_userptr_get_sg_table, > +}; [Severity: Critical] Is it safe to expose these objects to the PRIME export subsystem without verifying their read-only status? If a userptr object is created with VIRTGPU_BLOB_FLAG_USERPTR_RDONLY to pin read-only memory, userspace could potentially export it via PRIME with O_RDWR permissions and import it into another device that performs DMA writes. Could this allow hardware to bypass CPU page table protections and overwrite read-only system memory or files? [ ... ] > +static void > +virtio_gpu_userptr_put_pages(struct virtio_gpu_object_userptr *userptr) > +{ [ ... ] > + if (userptr->pages) { > + unpin_user_pages(userptr->pages, userptr->npages); > + kvfree(userptr->pages); > + userptr->pages = NULL; > + } [Severity: Critical] Are we missing dirty tracking when releasing these pages back to the OS? If a userptr object is created without the read-only flag, pinned with FOLL_WRITE, written to by the device, and then destroyed, we call unpin_user_pages() here without explicitly marking the pages as dirty. If the pages are file-backed or swapped, could the kernel memory management subsystem silently discard the data written by the host or device? [ ... ] > +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) > +{ [ ... ] > + /* > + * Match shmem blobs: only DMA-map when the virtio DMA API is in > + * use. Mapping unconditionally can create SWIOTLB bounce buffers > + * that get copied back over guest pages on unmap even though the > + * host was given sg_phys() addresses. > + */ > + if (virtio_gpu_use_dma_api(vgdev->vdev)) { > + enum dma_data_direction dir = > + (userptr->flags & VIRTGPU_BLOB_FLAG_USERPTR_RDONLY) ? > + DMA_TO_DEVICE : DMA_BIDIRECTIONAL; > + > + ret = dma_map_sgtable(drm_dev_dma_dev(vgdev->ddev), sgt, > + dir, 0); [Severity: High] How does this DMA-mapped memory get synchronized to avoid stale reads? When mapping with DMA_BIDIRECTIONAL and the guest modifies the userptr memory, the host might read stale data from bounce buffers unless a DMA sync occurs. However, in virtio_gpu_cmd_transfer_to_host_3d() and 2d transfers, the sync check looks like this: if (virtio_gpu_is_shmem(bo) && use_dma_api) dma_sync_sgtable_for_device(vgdev->vdev->dev.parent, bo->base.sgt, DMA_TO_DEVICE); Since virtio_gpu_is_shmem() evaluates to false for userptr objects, they appear to be excluded from this synchronization. Will this skip cause the host to read stale data during transfers? > + if (ret) > + goto err_cleanup; > + > + userptr->dma_dir = dir; > + userptr->dma_mapped = true; > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
