Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] drm/virtio: double-free and UAF in virtio_gpu_userptr_get_pages error path - [High] drm/virtio: read-only userptr memory mapped as DMA_BIDIRECTIONAL - [Medium] drm/virtio: silent fallback to VRAM if userptr is combined with HOST3D --
commit 357076bf581ca76e53fbe71d8c16da04e304d01c Author: Honglei Huang <[email protected]> drm/virtio: wire blob ioctl creation to userptr objects Integrate userptr into the blob resource creation ioctl. > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c > b/drivers/gpu/drm/virtio/virtgpu_ioctl.c > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c [ ... ] > @@ -458,6 +460,17 @@ static int verify_blob(struct virtio_gpu_device *vgdev, > return -EINVAL; > } > > + if (rc_blob->blob_flags & VIRTGPU_BLOB_FLAG_USE_USERPTR) { > + if (!rc_blob->userptr) > + return -EINVAL; > + } else { > + if (rc_blob->userptr) > + return -EINVAL; > + > + if (rc_blob->blob_flags & VIRTGPU_BLOB_FLAG_USERPTR_RDONLY) > + return -EINVAL; > + } > + [Severity: Medium] Will this silently fall back to VRAM if the user requests a userptr blob but sets the memory type to VIRTGPU_BLOB_MEM_HOST3D? If guest_blob is false and host3d_blob is true, the userptr flag is validated here but it doesn't ensure a guest memory type was requested. This could allow the execution to fall through into: virtio_gpu_resource_create_blob_ioctl() { ... else if (!guest_blob && host3d_blob) ret = virtio_gpu_vram_create(vgdev, ¶ms, &bo); ... } which would silently ignore params.userptr and allocate standard VRAM instead of returning -EINVAL. > switch (rc_blob->blob_mem) { > case VIRTGPU_BLOB_MEM_GUEST: [ ... ] > @@ -538,8 +552,10 @@ static int virtio_gpu_resource_create_blob_ioctl(struct > drm_device *dev, > vfpriv->ctx_id, NULL, NULL); > } > > - if (guest_blob) > + if (guest_blob && !params.userptr) > ret = virtio_gpu_object_create(vgdev, ¶ms, &bo, NULL); > + else if (guest_blob && params.userptr) > + ret = virtio_gpu_userptr_create(vgdev, file, ¶ms, &bo); [Severity: Critical] Does the error path in virtio_gpu_userptr_get_pages() lead to a use-after-free and double-free when this is called? If virtio_gpu_userptr_get_pages() enters the error path, it frees the pages array but leaves a dangling pointer: virtio_gpu_userptr_get_pages() { ... if (pinned) unpin_user_pages(userptr->pages, pinned); kvfree(userptr->pages); return ret; } The error returned to virtio_gpu_userptr_create() triggers a jump to err_cleanup and calls virtio_gpu_cleanup_object(), which eventually calls virtio_gpu_userptr_put_pages(): virtio_gpu_userptr_put_pages() { if (userptr->pages) { unpin_user_pages(userptr->pages, userptr->npages); kvfree(userptr->pages); userptr->pages = NULL; } } Because the pointer is not NULL, this would unpin and free the already freed array, potentially corrupting page refcounts and memory. [Severity: High] Can a read-only userptr be overwritten by the host device? When VIRTGPU_BLOB_FLAG_USERPTR_RDONLY is specified, the userptr memory is pinned without FOLL_WRITE, allowing read-only memory to be pinned. However, the DMA mapping in virtio_gpu_userptr_create() appears to be hardcoded to bidirectional: virtio_gpu_userptr_create() { ... ret = dma_map_sgtable(drm_dev_dma_dev(vgdev->ddev), sgt, DMA_BIDIRECTIONAL, 0); ... } Because it is mapped bidirectionally, the IOMMU allows device writes to these physical pages. Can unprivileged userspace submit command buffers that instruct the host device to write into the blob, overwriting the read-only guest memory? Should this use DMA_TO_DEVICE when USERPTR_RDONLY is set? > + else if (!guest_blob && host3d_blob) > + ret = virtio_gpu_vram_create(vgdev, ¶ms, &bo); > else > return -EINVAL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
