From 85d9d044471f93c48c5c396f7e217b4ef12f69f8 Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Tue, 1 Nov 2016 05:37:57 -0700 Subject: [PATCH 1/5] virtio-gpu: fix information leak in capset get dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In virgl_cmd_get_capset function, it uses g_malloc to allocate a response struct to the guest. As the 'resp'struct hasn't been full initialized it will lead the 'resp->padding' field to the guest. Use g_malloc0 to avoid this. Signed-off-by: Li Qiang Reviewed-by: Marc-André Lureau Message-id: 58188cae.4a6ec20a.3d2d1.aff2@mx.google.com [ kraxel: resolved conflict ] Signed-off-by: Gerd Hoffmann --- hw/display/virtio-gpu-3d.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c index b13ced38fa..f96a0c2e59 100644 --- a/hw/display/virtio-gpu-3d.c +++ b/hw/display/virtio-gpu-3d.c @@ -379,7 +379,7 @@ static void virgl_cmd_get_capset(VirtIOGPU *g, return; } - resp = g_malloc(sizeof(*resp) + max_size); + resp = g_malloc0(sizeof(*resp) + max_size); resp->hdr.type = VIRTIO_GPU_RESP_OK_CAPSET; virgl_renderer_fill_caps(gc.capset_id, gc.capset_version, From 913a87885f589d263e682c2eb6637c6e14538061 Mon Sep 17 00:00:00 2001 From: Bruce Rogers Date: Mon, 9 Jan 2017 13:35:20 -0700 Subject: [PATCH 2/5] display: cirrus: ignore source pitch value as needed in blit_is_unsafe Commit 4299b90 added a check which is too broad, given that the source pitch value is not required to be initialized for solid fill operations. This patch refines the blit_is_unsafe() check to ignore source pitch in that case. After applying the above commit as a security patch, we noticed the SLES 11 SP4 guest gui failed to initialize properly. Signed-off-by: Bruce Rogers Message-id: 20170109203520.5619-1-brogers@suse.com Signed-off-by: Gerd Hoffmann --- hw/display/cirrus_vga.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index bdb092ee9d..379910db2d 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -294,7 +294,7 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s, return false; } -static bool blit_is_unsafe(struct CirrusVGAState *s) +static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only) { /* should be the case, see cirrus_bitblt_start */ assert(s->cirrus_blt_width > 0); @@ -308,6 +308,9 @@ static bool blit_is_unsafe(struct CirrusVGAState *s) s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) { return true; } + if (dst_only) { + return false; + } if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch, s->cirrus_blt_srcaddr & s->cirrus_addr_mask)) { return true; @@ -673,7 +676,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s, dst = s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask); - if (blit_is_unsafe(s)) + if (blit_is_unsafe(s, false)) return 0; (*s->cirrus_rop) (s, dst, src, @@ -691,7 +694,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop) { cirrus_fill_t rop_func; - if (blit_is_unsafe(s)) { + if (blit_is_unsafe(s, true)) { return 0; } rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1]; @@ -795,7 +798,7 @@ static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s) { - if (blit_is_unsafe(s)) + if (blit_is_unsafe(s, false)) return 0; return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr, From 039aa5db0e7d9edb2bd807c2d4e09d8d7be4c9c4 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 9 Jan 2017 13:38:42 +0000 Subject: [PATCH 3/5] virtio-gpu: Recalculate VirtIOGPU::hostmem on VM load The 'hostmem' field in VirtIOGPU is used to track the total memory used in pixmaps so that we can impose a maximum limit on it. However this field is neither migrated nor recalculated on VM load, which means that after a migration it will be incorrectly too low, which can allow the guest to use more pixmap memory than it should. The per-resource hostmem fields are not filled in either as we reallocate them in the load function. Recalculate the memory used for each pixmap and the total memory used as we reallocate the pixmaps in virtio_gpu_load(). Signed-off-by: Peter Maydell Message-id: 1483969123-14839-2-git-send-email-peter.maydell@linaro.org Signed-off-by: Gerd Hoffmann --- hw/display/virtio-gpu.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index ca88cf478d..c3cf47e57f 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1038,6 +1038,8 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size) uint32_t resource_id, pformat; int i; + g->hostmem = 0; + resource_id = qemu_get_be32(f); while (resource_id != 0) { res = g_new0(struct virtio_gpu_simple_resource, 1); @@ -1059,6 +1061,8 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size) return -EINVAL; } + res->hostmem = PIXMAN_FORMAT_BPP(pformat) * res->width * res->height; + res->addrs = g_new(uint64_t, res->iov_cnt); res->iov = g_new(struct iovec, res->iov_cnt); @@ -1081,6 +1085,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size) } QTAILQ_INSERT_HEAD(&g->reslist, res, next); + g->hostmem += res->hostmem; resource_id = qemu_get_be32(f); } From c84f0f25db2eaab101665ddb60c1ddf1decce76a Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 9 Jan 2017 13:38:43 +0000 Subject: [PATCH 4/5] virtio-gpu: Fix memory leak in virtio_gpu_load() Coverity points out that if we fail in the "creating resources" loop in virtio_gpu_load() we will leak various resources (CID 1356431). Failing a VM load is going to leave the simulation in a complete mess, but we can tidy up to the point that a full system reset should get us back to sanity. Signed-off-by: Peter Maydell Message-id: 1483969123-14839-3-git-send-email-peter.maydell@linaro.org Signed-off-by: Gerd Hoffmann --- hw/display/virtio-gpu.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index c3cf47e57f..cef736cebf 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1052,12 +1052,14 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size) /* allocate */ pformat = get_pixman_format(res->format); if (!pformat) { + g_free(res); return -EINVAL; } res->image = pixman_image_create_bits(pformat, res->width, res->height, NULL, 0); if (!res->image) { + g_free(res); return -EINVAL; } @@ -1080,6 +1082,16 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size) res->iov[i].iov_base = cpu_physical_memory_map(res->addrs[i], &len, 1); if (!res->iov[i].iov_base || len != res->iov[i].iov_len) { + /* Clean up the half-a-mapping we just created... */ + if (res->iov[i].iov_base) { + cpu_physical_memory_unmap(res->iov[i].iov_base, + len, 0, 0); + } + /* ...and the mappings for previous loop iterations */ + res->iov_cnt = i; + virtio_gpu_cleanup_mapping(res); + pixman_image_unref(res->image); + g_free(res); return -EINVAL; } } From a2056e09b02745e5d000351a8a7938fa8a292ba7 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 9 Jan 2017 14:55:38 +0100 Subject: [PATCH 5/5] virtio-gpu: tag as not hotpluggable qemu can't hotplug display devices. Signed-off-by: Gerd Hoffmann Reviewed-by: Michael S. Tsirkin Message-id: 1483970138-20360-1-git-send-email-kraxel@redhat.com --- hw/display/virtio-gpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index cef736cebf..7a15c61c76 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1299,6 +1299,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data) dc->props = virtio_gpu_properties; dc->vmsd = &vmstate_virtio_gpu; + dc->hotpluggable = false; } static const TypeInfo virtio_gpu_info = {