Re: [PATCH v2] virtio-gpu: use g_try_malloc to avoid guest-triggered abort

Hi Akihiko

On Tue, Jul 28, 2026 at 11:17 PM Akihiko Odaki
<[email protected]> wrote:
>
> On 2026/07/28 23:33, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Jul 28, 2026 at 6:28 PM Akihiko Odaki
> > <[email protected]> wrote:
> >>
> >> On 2026/07/28 19:15, [email protected] wrote:
> >>> From: Marc-André Lureau <[email protected]>
> >>>
> >>> Use g_try_malloc/g_try_new0 for guest-controlled allocation, so failure
> >>> returns an error to the guest rather than crashing the host (glib
> >>> behaviour).
> >>>
> >>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3898
> >>>
> >>> Signed-off-by: Marc-André Lureau <[email protected]>
> >>> ---
> >>> Based-on: <[email protected]> ("[PATCH] vhost-user-gpu: fix integer overflow in buffer allocation")
> >>> v2:
> >>>    - accept NULL 0-size allocations
> >>> ---
> >>>    contrib/vhost-user-gpu/vhost-user-gpu.c | 25 +++++++++++++++++--------
> >>>    contrib/vhost-user-gpu/virgl.c          |  6 +++++-
> >>>    contrib/vhost-user-gpu/vugbm.c          |  5 ++++-
> >>>    hw/display/virtio-gpu-rutabaga.c        | 14 ++++++++++++--
> >>>    hw/display/virtio-gpu-virgl.c           |  6 +++++-
> >>>    hw/display/virtio-gpu.c                 |  5 ++++-
> >>>    6 files changed, 47 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
> >>> index ee9858c397ce..0fd115300425 100644
> >>> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> >>> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> >>> @@ -487,7 +487,7 @@ vg_create_mapping_iov(VuGpu *g,
> >>>                          struct virtio_gpu_ctrl_command *cmd,
> >>>                          struct iovec **iov)
> >>>    {
> >>> -    struct virtio_gpu_mem_entry *ents;
> >>> +    g_autofree struct virtio_gpu_mem_entry *ents = NULL;
> >>>        size_t esize, s;
> >>>        int i;
> >>>
> >>> @@ -498,17 +498,22 @@ vg_create_mapping_iov(VuGpu *g,
> >>>        }
> >>>
> >>>        esize = sizeof(*ents) * ab->nr_entries;
> >>> -    ents = g_malloc(esize);
> >>> +    ents = g_try_malloc(esize);
> >>> +    if (!ents && esize) {
> >>> +        return -1;
> >>> +    }
> >>>        s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num,
> >>>                       sizeof(*ab), ents, esize);
> >>>        if (s != esize) {
> >>>            g_critical("%s: command data size incorrect %zu vs %zu",
> >>>                       __func__, s, esize);
> >>> -        g_free(ents);
> >>>            return -1;
> >>>        }
> >>>
> >>> -    *iov = g_new0(struct iovec, ab->nr_entries);
> >>> +    *iov = g_try_new0(struct iovec, ab->nr_entries);
> >>> +    if (!*iov && ab->nr_entries) {
> >>> +        return -1;
> >>> +    }
> >>>        for (i = 0; i < ab->nr_entries; i++) {
> >>>            uint64_t len = ents[i].length;
> >>>            (*iov)[i].iov_len = ents[i].length;
> >>> @@ -517,12 +522,10 @@ vg_create_mapping_iov(VuGpu *g,
> >>>                g_critical("%s: resource %d element %d",
> >>>                           __func__, ab->resource_id, i);
> >>>                g_free(*iov);
> >>> -            g_free(ents);
> >>>                *iov = NULL;
> >>>                return -1;
> >>>            }
> >>>        }
> >>> -    g_free(ents);
> >>>        return 0;
> >>>    }
> >>>
> >>> @@ -828,8 +831,14 @@ vg_resource_flush(VuGpu *g,
> >>>                    PIXMAN_FORMAT_BPP(pixman_image_get_format(res->image)) / 8;
> >>>                size_t size = width * height * bpp;
> >>>
> >>> -            void *p = g_malloc(VHOST_USER_GPU_HDR_SIZE +
> >>> -                               sizeof(VhostUserGpuUpdate) + size);
> >>> +            void *p = g_try_malloc(VHOST_USER_GPU_HDR_SIZE +
> >>> +                                   sizeof(VhostUserGpuUpdate) + size);
> >>> +            if (!p) {
> >>> +                pixman_region_fini(&region);
> >>> +                pixman_region_fini(&finalregion);
> >>> +                cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> >>> +                break;
> >>> +            }
> >>>                VhostUserGpuMsg *msg = p;
> >>>                msg->request = VHOST_USER_GPU_UPDATE;
> >>>                msg->size = sizeof(VhostUserGpuUpdate) + size;
> >>> diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
> >>> index 550fd03bf5c4..20bae57d0fe4 100644
> >>> --- a/contrib/vhost-user-gpu/virgl.c
> >>> +++ b/contrib/vhost-user-gpu/virgl.c
> >>> @@ -209,7 +209,11 @@ virgl_cmd_submit_3d(VuGpu *g,
> >>>            return;
> >>>        }
> >>>
> >>> -    buf = g_malloc(cs.size);
> >>> +    buf = g_try_malloc(cs.size);
> >>> +    if (!buf && cs.size) {
> >>> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> >>> +        return;
> >>> +    }
> >>>        s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num,
> >>>                       sizeof(cs), buf, cs.size);
> >>>        if (s != cs.size) {
> >>> diff --git a/contrib/vhost-user-gpu/vugbm.c b/contrib/vhost-user-gpu/vugbm.c
> >>> index 710d54529779..e2d8385fd857 100644
> >>> --- a/contrib/vhost-user-gpu/vugbm.c
> >>> +++ b/contrib/vhost-user-gpu/vugbm.c
> >>> @@ -13,7 +13,10 @@
> >>>    static bool
> >>>    mem_alloc_bo(struct vugbm_buffer *buf)
> >>>    {
> >>> -    buf->mmap = g_malloc((uint64_t)buf->width * buf->height * 4);
> >>> +    buf->mmap = g_try_malloc((uint64_t)buf->width * buf->height * 4);
> >>> +    if (!buf->mmap && buf->width && buf->height) {
> >>> +        return false;
> >>> +    }
> >>>        buf->stride = buf->width * 4;
> >>>        return true;
> >>>    }
> >>> diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-gpu-rutabaga.c
> >>> index e28aad94eead..60f64d9a8fa7 100644
> >>> --- a/hw/display/virtio-gpu-rutabaga.c
> >>> +++ b/hw/display/virtio-gpu-rutabaga.c
> >>> @@ -360,10 +360,20 @@ rutabaga_cmd_submit_3d(VirtIOGPU *g,
> >>>            return;
> >>>        }
> >>>
> >>> -    buf = g_new0(uint8_t, cs.size);
> >>> +    buf = g_try_new0(uint8_t, cs.size);
> >>> +    if (!buf && cs.size) {
> >>> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> >>> +        return;
> >>> +    }
> >>>        s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num,
> >>>                       sizeof(cs), buf, cs.size);
> >>> -    CHECK(s == cs.size, cmd);
> >>> +    if (s != cs.size) {
> >>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>> +                      "%s: size mismatch (%zu/%u)\n",
> >>> +                      __func__, s, cs.size);
> >>> +        cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
> >>> +        return;
> >>> +    }
> >>>
> >>>        rutabaga_cmd.ctx_id = cs.hdr.ctx_id;
> >>>        rutabaga_cmd.cmd = buf;
> >>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> >>> index d9e5b0110497..028ab776df6a 100644
> >>> --- a/hw/display/virtio-gpu-virgl.c
> >>> +++ b/hw/display/virtio-gpu-virgl.c
> >>> @@ -615,7 +615,11 @@ static void virgl_cmd_submit_3d(VirtIOGPU *g,
> >>>            return;
> >>>        }
> >>>
> >>> -    buf = g_malloc(cs.size);
> >>> +    buf = g_try_malloc(cs.size);
> >>> +    if (!buf && cs.size) {
> >>> +        cmd->error = VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY;
> >>> +        return;
> >>> +    }
> >>>        s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num,
> >>>                       sizeof(cs), buf, cs.size);
> >>>        if (s != cs.size) {
> >>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >>> index 8607d0c0b581..9bacd38b4521 100644
> >>> --- a/hw/display/virtio-gpu.c
> >>> +++ b/hw/display/virtio-gpu.c
> >>> @@ -892,7 +892,10 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> >>>        }
> >>>
> >>>        esize = sizeof(*ents) * nr_entries;
> >>> -    ents = g_malloc(esize);
> >>> +    ents = g_try_malloc(esize);
> >>> +    if (!ents && esize) {
> >>> +        return -1;
> >>> +    }
> >>
> >> This converts the entry allocation to g_try_malloc(), but the resulting
> >> mapping arrays still grow with aborting g_renew() calls.
> >> dma_memory_map() may return only part of an entry; with fragmented IOMMU
> >> mappings, guest-controlled entries can expand into more IOVs than
> >> nr_entries.
> >>
> >
> > The goal is not to make QEMU OOM-safe. Even when malloc returns
> > non-zero we may reach that and get killed. This patch is more a safety
> > guard for easy guest triggered allocations that can crash QEMU.
> I agree we are not going to make QEMU OOM-safe, but I'm not sure where
> we draw the line between "easy" and "hard" guest-triggered allocations.
> Even if the IOMMU mapping isn't fragmented, the data from ents is
> essentially duplicated into iov right after, doubling the allocation
> size anyway. I feel it's easier to avoid reasoning by just consistently
> using g_try_* in this function.


Ok, maybe it doesn't make sense for bounded-allocation to use g_try
then. It's true that if the IOMMU mapping is fragmented, we could end
up with an allocation that could fail. I am not sure we can do much in
this case but rather have a reasonable limit like
VIRTIO_GPU_MAX_IOV_ENTRIES (16384 * 16). Otherwise, happy to convert
to g_try_renew() for now..