Re: [PATCH v4 1/3] virtio-gpu: validate host page alignment for MAP_FIXED blobs

["Alex Bennée" <alex.bennee@linaro.org>]

Lucas Amaral <lucaaamaral@gmail.com> writes:

> Commit 4eb0aace ("virtio-gpu: Support mapping hostmem blobs with
> map_fixed") uses mmap(MAP_FIXED) to map blob resources into a
> pre-allocated hostmem region.  Both the offset and size passed to
> mmap must be aligned to the host page size, but the code does not
> validate this.
>
> On hosts where qemu_real_host_page_size() exceeds the guest's page
> size (e.g. ARM64 with 16KB or 64KB pages, macOS ARM64), the guest
> may provide blob offsets aligned to its own page size (4KB) but not
> to the host's.

What was your test case? Is it possible to publish some images for a
functional test?

> This causes mmap(MAP_FIXED) to fail with EINVAL,
> and the subsequent unmap (which also uses mmap MAP_FIXED) fails the
> same way, producing:
>
>   virtio_gpu_virgl_unmap_resource_blob: failed to unmap(fixed)
>   virgl resource: Invalid argument
>
> Add an alignment check before attempting MAP_FIXED.  When the offset
> or blob size is not host-page-aligned, skip the MAP_FIXED path and
> fall through to the existing subregion method, which handles any
> alignment.
>
> Fixes: 4eb0aace ("virtio-gpu: Support mapping hostmem blobs with map_fixed")
> Signed-off-by: Lucas Amaral <lucaaamaral@gmail.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 45 +++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index b7a2d160..f6583b48 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -185,25 +185,34 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g,
>          return -EBUSY;
>      }
>  
> -    ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
> -                                            gl->hostmem_mmap + offset);
> -    switch (ret) {
> -    case 0:
> -        res->map_fixed = gl->hostmem_mmap + offset;
> -        return 0;
> -
> -    case -EOPNOTSUPP:
> -        /*
> -         * MAP_FIXED is unsupported by this resource.
> -         * Mapping falls back to a blob subregion method in that case.
> -         */
> -        break;
> +    /*
> +     * MAP_FIXED requires host-page-aligned offset and size.  Hosts with
> +     * page sizes larger than the guest's (e.g. 16KB on ARM64) may receive
> +     * non-aligned blob offsets.  Fall through to the subregion method when
> +     * alignment requirements are not met.
> +     */
> +    if (QEMU_IS_ALIGNED(offset, qemu_real_host_page_size()) &&
> +        QEMU_IS_ALIGNED(res->base.blob_size, qemu_real_host_page_size())) {
> +        ret = virgl_renderer_resource_map_fixed(res->base.resource_id,
> +                                                gl->hostmem_mmap + offset);
> +        switch (ret) {
> +        case 0:
> +            res->map_fixed = gl->hostmem_mmap + offset;
> +            return 0;
> +
> +        case -EOPNOTSUPP:
> +            /*
> +             * MAP_FIXED is unsupported by this resource.
> +             * Mapping falls back to a blob subregion method in that case.
> +             */
> +            break;
>  
> -    default:
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: failed to map(fixed) virgl resource: %s\n",
> -                      __func__, strerror(-ret));
> -        return ret;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: failed to map(fixed) virgl resource: %s\n",
> +                          __func__, strerror(-ret));
> +            return ret;
> +        }
>      }
>  #endif

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro