RE: [PATCH v6 4/4] drm/virtio: wire blob ioctl creation to userptr objects

Thu, 17 Sep 2026 03:43:42 -0700 

AMD General

Will fix it and waiting for maintainer's suggestions

-----Original Message-----
From: [email protected] <[email protected]>
Sent: Thursday, September 17, 2026 6:38 PM
To: Huang, Honglei1 <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>; [email protected]; 
Eugenio Perez <[email protected]>; [email protected]
Subject: Re: [PATCH v6 4/4] drm/virtio: wire blob ioctl creation to userptr 
objects

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, &params, &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, &params, &bo, NULL);
> +     else if (guest_blob && params.userptr)
> +             ret = virtio_gpu_userptr_create(vgdev, file, &params, &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, &params, &bo);
>       else
>               return -EINVAL;

--
Sashiko AI review ยท 
https://sashiko.dev/#/patchset/[email protected]?part=4

Reply via email to