[Qemu-devel] [PULL 0/6] vga patch queue

[Qemu-devel] [PULL 0/6] vga patch queue

[Gerd Hoffmann]

  Hi,

Here comes the vga patch queue, bringing bugfixes
for cirrus and virtio-gpu.

please pull,
  Gerd

The following changes since commit 33d076ebd05491d9f7cace682b4ed70c33263f12:

  Merge remote-tracking branch 'remotes/stsquad/tags/pull-travis-10022017-1' into staging (2017-02-10 15:05:37 +0000)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-vga-20170213-1

for you to fetch changes up to 12e97ec39931e5321645fd483ab761319d48bf16:

  Revert "cirrus: allow zero source pitch in pattern fill rops" (2017-02-10 16:49:45 +0100)

----------------------------------------------------------------
vga: bugfixes for cirrus and virtio-gpu

----------------------------------------------------------------
Gerd Hoffmann (5):
      virtio-gpu: fix resource leak in virgl_cmd_resource_unref
      vga: replace debug printf with trace points
      cirrus: replace debug printf with trace points
      cirrus: fix patterncopy checks
      Revert "cirrus: allow zero source pitch in pattern fill rops"

Li Qiang (1):
      virtio-gpu: fix memory leak in set scanout

 hw/display/cirrus_vga.c    | 71 +++++++++++++++++++++++++++-------------------
 hw/display/trace-events    | 12 ++++++++
 hw/display/vga.c           | 27 +++---------------
 hw/display/virtio-gpu-3d.c |  8 ++++++
 hw/display/virtio-gpu.c    |  1 +
 5 files changed, 67 insertions(+), 52 deletions(-)

[Qemu-devel] [PULL 1/6] virtio-gpu: fix memory leak in set scanout

[Gerd Hoffmann]

From: Li Qiang <liqiang6-s@360.cn>

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 <liqiang6-s@360.cn>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 5884626f.5b2f6b0a.1bfff.3037@mx.google.com
Signed-off-by: Gerd Hoffmann <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 444ca06..9b530ab 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);
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 2/6] virtio-gpu: fix resource leak in virgl_cmd_resource_unref

[Gerd Hoffmann]

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: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
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 f96a0c2..ecb09d1 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);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 3/6] vga: replace debug printf with trace points

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 aadb612..26910e2 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 2a88b3c..69c3e1d 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;
-- 
1.8.3.1

[Qemu-devel] [PULL 4/6] cirrus: replace debug printf with trace points

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 16f27e8..b272a70 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 26910e2..3e896d2 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"
-- 
1.8.3.1

[Qemu-devel] [PULL 5/6] cirrus: fix patterncopy checks

[Gerd Hoffmann]

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 <w.bumiller@proxmox.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
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 b272a70..1bcf9a4 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);
-- 
1.8.3.1

[Qemu-devel] [PULL 6/6] Revert "cirrus: allow zero source pitch in pattern fill rops"

[Gerd Hoffmann]

This reverts commit 5858dd1801883309bdd208d72ddb81c4e9fee30c.

Conflicts:
	hw/display/cirrus_vga.c

Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
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 1bcf9a4..1deb520 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,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 0/6] vga patch queue

[Peter Maydell]

On 13 February 2017 at 08:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here comes the vga patch queue, bringing bugfixes
> for cirrus and virtio-gpu.
>
> please pull,
>   Gerd
>
> The following changes since commit 33d076ebd05491d9f7cace682b4ed70c33263f12:
>
>   Merge remote-tracking branch 'remotes/stsquad/tags/pull-travis-10022017-1' into staging (2017-02-10 15:05:37 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-vga-20170213-1
>
> for you to fetch changes up to 12e97ec39931e5321645fd483ab761319d48bf16:
>
>   Revert "cirrus: allow zero source pitch in pattern fill rops" (2017-02-10 16:49:45 +0100)
>
> ----------------------------------------------------------------
> vga: bugfixes for cirrus and virtio-gpu
>

Applied, thanks.

-- PMM