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

[sashiko-bot@kernel.org]

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 <Honglei1.Huang@amd.com>

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/20260917102540.1312102-1-honghuan@amd.com?part=4