From dd248ed7e204ee8a1873914e02b8b526e8f1b80d Mon Sep 17 00:00:00 2001 From: Li Qiang Date: Sat, 21 Jan 2017 23:42:33 -0800 Subject: [PATCH 1/6] virtio-gpu: fix memory leak in set scanout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In virtio_gpu_set_scanout function, when creating the 'rect' its refcount is set to 2, by pixman_image_create_bits and qemu_create_displaysurface_pixman function. This can lead a memory leak issues. This patch avoid this issue. Signed-off-by: Li Qiang Reviewed-by: Marc-André Lureau Message-id: 5884626f.5b2f6b0a.1bfff.3037@mx.google.com Signed-off-by: Gerd Hoffmann --- 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 444ca064c1..9b530ab5b0 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -608,6 +608,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g, cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC; return; } + pixman_image_unref(rect); dpy_gfx_replace_surface(g->scanout[ss.scanout_id].con, scanout->ds); } From 5e8e3c4c75c199aa1017db816fca02be2a9f8798 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Mon, 23 Jan 2017 11:26:50 +0100 Subject: [PATCH 2/6] virtio-gpu: fix resource leak in virgl_cmd_resource_unref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the guest sends VIRTIO_GPU_CMD_RESOURCE_UNREF without detaching the backing storage beforehand (VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING) we'll leak memory. This patch fixes it for 3d mode, simliar to the 2d mode fix in commit "b8e2392 virtio-gpu: call cleanup mapping function in resource destroy". Reported-by: 李强 Signed-off-by: Gerd Hoffmann Message-id: 1485167210-4757-1-git-send-email-kraxel@redhat.com --- hw/display/virtio-gpu-3d.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c index f96a0c2e59..ecb09d17a1 100644 --- a/hw/display/virtio-gpu-3d.c +++ b/hw/display/virtio-gpu-3d.c @@ -77,10 +77,18 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { struct virtio_gpu_resource_unref unref; + struct iovec *res_iovs = NULL; + int num_iovs = 0; VIRTIO_GPU_FILL_CMD(unref); trace_virtio_gpu_cmd_res_unref(unref.resource_id); + virgl_renderer_resource_detach_iov(unref.resource_id, + &res_iovs, + &num_iovs); + if (res_iovs != NULL && num_iovs != 0) { + virtio_gpu_cleanup_mapping_iov(res_iovs, num_iovs); + } virgl_renderer_resource_unref(unref.resource_id); } From cf7dabeebc36bfb93413f175234779f5bfb2c0b1 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 8 Feb 2017 14:51:32 +0100 Subject: [PATCH 3/6] vga: replace debug printf with trace points MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Gerd Hoffmann Reviewed-by: Laurent Vivier Reviewed-by: Philippe Mathieu-Daudé Message-id: 1486561893-26470-1-git-send-email-kraxel@redhat.com --- hw/display/trace-events | 6 ++++++ hw/display/vga.c | 27 ++++----------------------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/hw/display/trace-events b/hw/display/trace-events index aadb612dcb..26910e2d47 100644 --- a/hw/display/trace-events +++ b/hw/display/trace-events @@ -119,3 +119,9 @@ qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d r qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]" qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d" qxl_render_update_area_done(void *cookie) "%p" + +# hw/display/vga.c +vga_std_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" +vga_std_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" +vga_vbe_read(uint32_t index, uint32_t val) "index 0x%x, val 0x%x" +vga_vbe_write(uint32_t index, uint32_t val) "index 0x%x, val 0x%x" diff --git a/hw/display/vga.c b/hw/display/vga.c index 2a88b3c1b4..69c3e1d674 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -34,12 +34,9 @@ #include "hw/xen/xen.h" #include "trace.h" -//#define DEBUG_VGA //#define DEBUG_VGA_MEM //#define DEBUG_VGA_REG -//#define DEBUG_BOCHS_VBE - /* 16 state changes per vertical frame @60 Hz */ #define VGA_TEXT_CURSOR_PERIOD_MS (1000 * 2 * 16 / 60) @@ -428,9 +425,7 @@ uint32_t vga_ioport_read(void *opaque, uint32_t addr) break; } } -#if defined(DEBUG_VGA) - printf("VGA: read addr=0x%04x data=0x%02x\n", addr, val); -#endif + trace_vga_std_read_io(addr, val); return val; } @@ -443,9 +438,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) if (vga_ioport_invalid(s, addr)) { return; } -#ifdef DEBUG_VGA - printf("VGA: write addr=0x%04x data=0x%02x\n", addr, val); -#endif + trace_vga_std_write_io(addr, val); switch(addr) { case VGA_ATT_W: @@ -733,9 +726,7 @@ uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr) } else { val = 0; } -#ifdef DEBUG_BOCHS_VBE - printf("VBE: read index=0x%x val=0x%x\n", s->vbe_index, val); -#endif + trace_vga_vbe_read(s->vbe_index, val); return val; } @@ -750,9 +741,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val) VGACommonState *s = opaque; if (s->vbe_index <= VBE_DISPI_INDEX_NB) { -#ifdef DEBUG_BOCHS_VBE - printf("VBE: write index=0x%x val=0x%x\n", s->vbe_index, val); -#endif + trace_vga_vbe_write(s->vbe_index, val); switch(s->vbe_index) { case VBE_DISPI_INDEX_ID: if (val == VBE_DISPI_ID0 || @@ -1543,17 +1532,9 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) height, format, s->line_offset, s->vram_ptr + (s->start_addr * 4)); dpy_gfx_replace_surface(s->con, surface); -#ifdef DEBUG_VGA - printf("VGA: Using shared surface for depth=%d swap=%d\n", - depth, byteswap); -#endif } else { qemu_console_resize(s->con, disp_width, height); surface = qemu_console_surface(s->con); -#ifdef DEBUG_VGA - printf("VGA: Using shadow surface for depth=%d swap=%d\n", - depth, byteswap); -#endif } s->last_scr_width = disp_width; s->last_scr_height = height; From ec87f206d708191abdd332fdfd48fc5b36da083c Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 8 Feb 2017 14:51:33 +0100 Subject: [PATCH 4/6] cirrus: replace debug printf with trace points MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Gerd Hoffmann Reviewed-by: Laurent Vivier Reviewed-by: Philippe Mathieu-Daudé Message-id: 1486561893-26470-2-git-send-email-kraxel@redhat.com --- hw/display/cirrus_vga.c | 11 +++++------ hw/display/trace-events | 6 ++++++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 16f27e8ac5..b272a70d60 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -28,6 +28,7 @@ */ #include "qemu/osdep.h" #include "qapi/error.h" +#include "trace.h" #include "hw/hw.h" #include "hw/pci/pci.h" #include "ui/console.h" @@ -1852,12 +1853,14 @@ static uint8_t cirrus_mmio_blt_read(CirrusVGAState * s, unsigned address) break; } + trace_vga_cirrus_write_blt(address, value); return (uint8_t) value; } static void cirrus_mmio_blt_write(CirrusVGAState * s, unsigned address, uint8_t value) { + trace_vga_cirrus_write_blt(address, value); switch (address) { case (CIRRUS_MMIO_BLTBGCOLOR + 0): cirrus_vga_write_gr(s, 0x00, value); @@ -2607,9 +2610,7 @@ static uint64_t cirrus_vga_ioport_read(void *opaque, hwaddr addr, break; } } -#if defined(DEBUG_VGA) - printf("VGA: read addr=0x%04x data=0x%02x\n", addr, val); -#endif + trace_vga_cirrus_read_io(addr, val); return val; } @@ -2626,9 +2627,7 @@ static void cirrus_vga_ioport_write(void *opaque, hwaddr addr, uint64_t val, if (vga_ioport_invalid(s, addr)) { return; } -#ifdef DEBUG_VGA - printf("VGA: write addr=0x%04x data=0x%02x\n", addr, val); -#endif + trace_vga_cirrus_write_io(addr, val); switch (addr) { case 0x3c0: diff --git a/hw/display/trace-events b/hw/display/trace-events index 26910e2d47..3e896d2e3f 100644 --- a/hw/display/trace-events +++ b/hw/display/trace-events @@ -125,3 +125,9 @@ vga_std_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" vga_std_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" vga_vbe_read(uint32_t index, uint32_t val) "index 0x%x, val 0x%x" vga_vbe_write(uint32_t index, uint32_t val) "index 0x%x, val 0x%x" + +# hw/display/cirrus_vga.c +vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" +vga_cirrus_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x" +vga_cirrus_read_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x" +vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x" From 95280c31cda79bb1d0968afc7b19a220b3a9d986 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 9 Feb 2017 14:02:20 +0100 Subject: [PATCH 5/6] cirrus: fix patterncopy checks The blit_region_is_unsafe checks don't work correctly for the patterncopy source. It's a fixed-sized region, which doesn't depend on cirrus_blt_{width,height}. So go do the check in cirrus_bitblt_common_patterncopy instead, then tell blit_is_unsafe that it doesn't need to verify the source. Also handle the case where we blit from cirrus_bitbuf correctly. This patch replaces 5858dd1801883309bdd208d72ddb81c4e9fee30c. Security impact: I think for the most part error on the safe side this time, refusing blits which should have been allowed. Only exception is placing the blit source at the end of the video ram, so cirrus_blt_srcaddr + 256 goes beyond the end of video memory. But even in that case I'm not fully sure this actually allows read access to host memory. To trick the commit 5858dd18 security checks one has to pick very small cirrus_blt_{width,height} values, which in turn implies only a fraction of the blit source will actually be used. Cc: Wolfgang Bumiller Cc: Dr. David Alan Gilbert Signed-off-by: Gerd Hoffmann Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Wolfgang Bumiller Reviewed-by: Laurent Vivier Message-id: 1486645341-5010-1-git-send-email-kraxel@redhat.com --- hw/display/cirrus_vga.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index b272a70d60..1bcf9a481f 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -684,14 +684,39 @@ static void cirrus_invalidate_region(CirrusVGAState * s, int off_begin, } } -static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s, - const uint8_t * src) +static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc) { + uint32_t patternsize; uint8_t *dst; + uint8_t *src; dst = s->vga.vram_ptr + s->cirrus_blt_dstaddr; - if (blit_is_unsafe(s, false, true)) { + if (videosrc) { + switch (s->vga.get_bpp(&s->vga)) { + case 8: + patternsize = 64; + break; + case 15: + case 16: + patternsize = 128; + break; + case 24: + case 32: + default: + patternsize = 256; + break; + } + s->cirrus_blt_srcaddr &= ~(patternsize - 1); + if (s->cirrus_blt_srcaddr + patternsize > s->vga.vram_size) { + return 0; + } + src = s->vga.vram_ptr + s->cirrus_blt_srcaddr; + } else { + src = s->cirrus_bltbuf; + } + + if (blit_is_unsafe(s, true, true)) { return 0; } @@ -732,8 +757,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop) static int cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s) { - return cirrus_bitblt_common_patterncopy(s, s->vga.vram_ptr + - (s->cirrus_blt_srcaddr & ~7)); + return cirrus_bitblt_common_patterncopy(s, true); } static int cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h) @@ -832,7 +856,7 @@ static void cirrus_bitblt_cputovideo_next(CirrusVGAState * s) if (s->cirrus_srccounter > 0) { if (s->cirrus_blt_mode & CIRRUS_BLTMODE_PATTERNCOPY) { - cirrus_bitblt_common_patterncopy(s, s->cirrus_bltbuf); + cirrus_bitblt_common_patterncopy(s, false); the_end: s->cirrus_srccounter = 0; cirrus_bitblt_reset(s); From 12e97ec39931e5321645fd483ab761319d48bf16 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Thu, 9 Feb 2017 14:02:21 +0100 Subject: [PATCH 6/6] Revert "cirrus: allow zero source pitch in pattern fill rops" This reverts commit 5858dd1801883309bdd208d72ddb81c4e9fee30c. Conflicts: hw/display/cirrus_vga.c Cc: Wolfgang Bumiller Cc: Dr. David Alan Gilbert Signed-off-by: Gerd Hoffmann Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Laurent Vivier Message-id: 1486645341-5010-2-git-send-email-kraxel@redhat.com --- hw/display/cirrus_vga.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 1bcf9a481f..1deb52070a 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -273,6 +273,9 @@ static void cirrus_update_memory_access(CirrusVGAState *s); static bool blit_region_is_unsafe(struct CirrusVGAState *s, int32_t pitch, int32_t addr) { + if (!pitch) { + return true; + } if (pitch < 0) { int64_t min = addr + ((int64_t)s->cirrus_blt_height - 1) * pitch @@ -291,11 +294,8 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s, return false; } -static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only, - bool zero_src_pitch_ok) +static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only) { - int32_t check_pitch; - /* should be the case, see cirrus_bitblt_start */ assert(s->cirrus_blt_width > 0); assert(s->cirrus_blt_height > 0); @@ -304,10 +304,6 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only, return true; } - if (!s->cirrus_blt_dstpitch) { - return true; - } - if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch, s->cirrus_blt_dstaddr)) { return true; @@ -315,13 +311,7 @@ static bool blit_is_unsafe(struct CirrusVGAState *s, bool dst_only, if (dst_only) { return false; } - - check_pitch = s->cirrus_blt_srcpitch; - if (!zero_src_pitch_ok && !check_pitch) { - check_pitch = s->cirrus_blt_width; - } - - if (blit_region_is_unsafe(s, check_pitch, + if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch, s->cirrus_blt_srcaddr)) { return true; } @@ -716,7 +706,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState *s, bool videosrc) src = s->cirrus_bltbuf; } - if (blit_is_unsafe(s, true, true)) { + if (blit_is_unsafe(s, true)) { return 0; } @@ -735,7 +725,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop) { cirrus_fill_t rop_func; - if (blit_is_unsafe(s, true, true)) { + if (blit_is_unsafe(s, true)) { return 0; } rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1]; @@ -835,7 +825,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, false, false)) + if (blit_is_unsafe(s, false)) return 0; return cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,