[Qemu-devel] [CVE-2014-3615 PATCH v2 0/3] vbe: bochs dispi interface fixes

[Qemu-devel] [CVE-2014-3615 PATCH v2 0/3] vbe: bochs dispi interface fixes

[Gerd Hoffmann]

  Hi,

Two fixes for the bochs dispi interface, one of them fixing a minor
security issue.

New in v2:  Got a CVE number.  Investigation & patch review found a
related issue in the spice code, so there is an additional patch.

/me plans to send a pull tomorrow, so this can go in fast enougth for
being cherry-picked into stable for the qemu 2.1.1 release.

please review,
  Gerd

Gerd Hoffmann (3):
  vbe: make bochs dispi interface return the correct memory size with
    qxl
  vbe: rework sanity checks
  spice: make sure we don't overflow ssd->buf

 hw/display/qxl.c     |   1 +
 hw/display/vga.c     | 159 ++++++++++++++++++++++++++++++++-------------------
 hw/display/vga_int.h |   1 +
 ui/spice-display.c   |  16 ++++--
 4 files changed, 113 insertions(+), 64 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [CVE-2014-3615 PATCH v2 1/3] vbe: make bochs dispi interface return the correct memory size with qxl

[Gerd Hoffmann]

VgaState->vram_size is the size of the pci bar.  In case of qxl not the
whole pci bar can be used as vga framebuffer.  Add a new variable
vbe_size to handle that case.  By default (if unset) it equals
vram_size, but qxl can set vbe_size to something else.

This makes sure VBE_DISPI_INDEX_VIDEO_MEMORY_64K returns correct results
and sanity checks are done with the correct size too.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/qxl.c     | 1 +
 hw/display/vga.c     | 7 +++++--
 hw/display/vga_int.h | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index d43aa49..652af99 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2063,6 +2063,7 @@ static int qxl_init_primary(PCIDevice *dev)
 
     qxl->id = 0;
     qxl_init_ramsize(qxl);
+    vga->vbe_size = qxl->vgamem_size;
     vga->vram_size_mb = qxl->vga.vram_size >> 20;
     vga_common_init(vga, OBJECT(dev), true);
     vga_init(vga, OBJECT(dev),
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 65dab8d..99251d7 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -610,7 +610,7 @@ uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
             val = s->vbe_regs[s->vbe_index];
         }
     } else if (s->vbe_index == VBE_DISPI_INDEX_VIDEO_MEMORY_64K) {
-        val = s->vram_size / (64 * 1024);
+        val = s->vbe_size / (64 * 1024);
     } else {
         val = 0;
     }
@@ -749,7 +749,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
                     line_offset = w >> 1;
                 else
                     line_offset = w * ((s->vbe_regs[VBE_DISPI_INDEX_BPP] + 7) >> 3);
-                h = s->vram_size / line_offset;
+                h = s->vbe_size / line_offset;
                 /* XXX: support weird bochs semantics ? */
                 if (h < s->vbe_regs[VBE_DISPI_INDEX_YRES])
                     return;
@@ -2285,6 +2285,9 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
         s->vram_size <<= 1;
     }
     s->vram_size_mb = s->vram_size >> 20;
+    if (!s->vbe_size) {
+        s->vbe_size = s->vram_size;
+    }
 
     s->is_vbe_vmstate = 1;
     memory_region_init_ram(&s->vram, obj, "vga.vram", s->vram_size);
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 641f8f4..bbc0cb2 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -93,6 +93,7 @@ typedef struct VGACommonState {
     MemoryRegion vram_vbe;
     uint32_t vram_size;
     uint32_t vram_size_mb; /* property */
+    uint32_t vbe_size;
     uint32_t latch;
     bool has_chain4_alias;
     MemoryRegion chain4_alias;
-- 
1.8.3.1

[Qemu-devel] [CVE-2014-3615 PATCH v2 2/3] vbe: rework sanity checks

[Gerd Hoffmann]

Plug a bunch of holes in the bochs dispi interface parameter checking.
Add a function doing verification on all registers.  Call that
unconditionally on every register write.  That way we should catch
everything, even changing one register affecting the valid range of
another register.

Some of the holes have been added by commit
e9c6149f6ae6873f14a12eea554925b6aa4c4dec.  Before that commit the
maximum possible framebuffer (VBE_DISPI_MAX_XRES * VBE_DISPI_MAX_YRES *
32 bpp) has been smaller than the qemu vga memory (8MB) and the checking
for VBE_DISPI_MAX_XRES + VBE_DISPI_MAX_YRES + VBE_DISPI_MAX_BPP was ok.

Some of the holes have been there forever, such as
VBE_DISPI_INDEX_X_OFFSET and VBE_DISPI_INDEX_Y_OFFSET register writes
lacking any verification.

Security impact:

(1) Guest can make the ui (gtk/vnc/...) use memory rages outside the vga
frame buffer as source  ->  host memory leak.  Memory isn't leaked to
the guest but to the vnc client though.

(2) Qemu will segfault in case the memory range happens to include
unmapped areas  ->  Guest can DoS itself.

The guest can not modify host memory, so I don't think this can be used
by the guest to escape.

CVE-2014-3615

Cc: qemu-stable@nongnu.org
Cc: secalert@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/display/vga.c | 154 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 95 insertions(+), 59 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 99251d7..62e6243 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -576,6 +576,93 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+/*
+ * Sanity check vbe register writes.
+ *
+ * As we don't have a way to signal errors to the guest in the bochs
+ * dispi interface we'll go adjust the registers to the closest valid
+ * value.
+ */
+static void vbe_fixup_regs(VGACommonState *s)
+{
+    uint16_t *r = s->vbe_regs;
+    uint32_t bits, linelength, maxy, offset;
+
+    if (!(r[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED)) {
+        /* vbe is turned off -- nothing to do */
+        return;
+    }
+
+    /* check depth */
+    switch (r[VBE_DISPI_INDEX_BPP]) {
+    case 4:
+    case 8:
+    case 16:
+    case 24:
+    case 32:
+        bits = r[VBE_DISPI_INDEX_BPP];
+        break;
+    case 15:
+        bits = 16;
+        break;
+    default:
+        bits = r[VBE_DISPI_INDEX_BPP] = 8;
+        break;
+    }
+
+    /* check width */
+    r[VBE_DISPI_INDEX_XRES] &= ~7u;
+    if (r[VBE_DISPI_INDEX_XRES] == 0) {
+        r[VBE_DISPI_INDEX_XRES] = 8;
+    }
+    if (r[VBE_DISPI_INDEX_XRES] > VBE_DISPI_MAX_XRES) {
+        r[VBE_DISPI_INDEX_XRES] = VBE_DISPI_MAX_XRES;
+    }
+    r[VBE_DISPI_INDEX_VIRT_WIDTH] &= ~7u;
+    if (r[VBE_DISPI_INDEX_VIRT_WIDTH] > VBE_DISPI_MAX_XRES) {
+        r[VBE_DISPI_INDEX_VIRT_WIDTH] = VBE_DISPI_MAX_XRES;
+    }
+    if (r[VBE_DISPI_INDEX_VIRT_WIDTH] < r[VBE_DISPI_INDEX_XRES]) {
+        r[VBE_DISPI_INDEX_VIRT_WIDTH] = r[VBE_DISPI_INDEX_XRES];
+    }
+
+    /* check height */
+    linelength = r[VBE_DISPI_INDEX_VIRT_WIDTH] * bits / 8;
+    maxy = s->vbe_size / linelength;
+    if (r[VBE_DISPI_INDEX_YRES] == 0) {
+        r[VBE_DISPI_INDEX_YRES] = 1;
+    }
+    if (r[VBE_DISPI_INDEX_YRES] > VBE_DISPI_MAX_YRES) {
+        r[VBE_DISPI_INDEX_YRES] = VBE_DISPI_MAX_YRES;
+    }
+    if (r[VBE_DISPI_INDEX_YRES] > maxy) {
+        r[VBE_DISPI_INDEX_YRES] = maxy;
+    }
+
+    /* check offset */
+    if (r[VBE_DISPI_INDEX_X_OFFSET] > VBE_DISPI_MAX_XRES) {
+        r[VBE_DISPI_INDEX_X_OFFSET] = VBE_DISPI_MAX_XRES;
+    }
+    if (r[VBE_DISPI_INDEX_Y_OFFSET] > VBE_DISPI_MAX_YRES) {
+        r[VBE_DISPI_INDEX_Y_OFFSET] = VBE_DISPI_MAX_YRES;
+    }
+    offset = r[VBE_DISPI_INDEX_X_OFFSET] * bits / 8;
+    offset += r[VBE_DISPI_INDEX_Y_OFFSET] * linelength;
+    if (offset + r[VBE_DISPI_INDEX_YRES] * linelength > s->vbe_size) {
+        r[VBE_DISPI_INDEX_Y_OFFSET] = 0;
+        offset = r[VBE_DISPI_INDEX_X_OFFSET] * bits / 8;
+        if (offset + r[VBE_DISPI_INDEX_YRES] * linelength > s->vbe_size) {
+            r[VBE_DISPI_INDEX_X_OFFSET] = 0;
+            offset = 0;
+        }
+    }
+
+    /* update vga state */
+    r[VBE_DISPI_INDEX_VIRT_HEIGHT] = maxy;
+    s->vbe_line_offset = linelength;
+    s->vbe_start_addr  = offset / 4;
+}
+
 static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
 {
     VGACommonState *s = opaque;
@@ -645,22 +732,13 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
             }
             break;
         case VBE_DISPI_INDEX_XRES:
-            if ((val <= VBE_DISPI_MAX_XRES) && ((val & 7) == 0)) {
-                s->vbe_regs[s->vbe_index] = val;
-            }
-            break;
         case VBE_DISPI_INDEX_YRES:
-            if (val <= VBE_DISPI_MAX_YRES) {
-                s->vbe_regs[s->vbe_index] = val;
-            }
-            break;
         case VBE_DISPI_INDEX_BPP:
-            if (val == 0)
-                val = 8;
-            if (val == 4 || val == 8 || val == 15 ||
-                val == 16 || val == 24 || val == 32) {
-                s->vbe_regs[s->vbe_index] = val;
-            }
+        case VBE_DISPI_INDEX_VIRT_WIDTH:
+        case VBE_DISPI_INDEX_X_OFFSET:
+        case VBE_DISPI_INDEX_Y_OFFSET:
+            s->vbe_regs[s->vbe_index] = val;
+            vbe_fixup_regs(s);
             break;
         case VBE_DISPI_INDEX_BANK:
             if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
@@ -677,19 +755,11 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
                 !(s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED)) {
                 int h, shift_control;
 
-                s->vbe_regs[VBE_DISPI_INDEX_VIRT_WIDTH] =
-                    s->vbe_regs[VBE_DISPI_INDEX_XRES];
-                s->vbe_regs[VBE_DISPI_INDEX_VIRT_HEIGHT] =
-                    s->vbe_regs[VBE_DISPI_INDEX_YRES];
+                s->vbe_regs[VBE_DISPI_INDEX_VIRT_WIDTH] = 0;
                 s->vbe_regs[VBE_DISPI_INDEX_X_OFFSET] = 0;
                 s->vbe_regs[VBE_DISPI_INDEX_Y_OFFSET] = 0;
-
-                if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4)
-                    s->vbe_line_offset = s->vbe_regs[VBE_DISPI_INDEX_XRES] >> 1;
-                else
-                    s->vbe_line_offset = s->vbe_regs[VBE_DISPI_INDEX_XRES] *
-                        ((s->vbe_regs[VBE_DISPI_INDEX_BPP] + 7) >> 3);
-                s->vbe_start_addr = 0;
+                s->vbe_regs[VBE_DISPI_INDEX_ENABLE] |= VBE_DISPI_ENABLED;
+                vbe_fixup_regs(s);
 
                 /* clear the screen (should be done in BIOS) */
                 if (!(val & VBE_DISPI_NOCLEARMEM)) {
@@ -738,40 +808,6 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
             s->vbe_regs[s->vbe_index] = val;
             vga_update_memory_access(s);
             break;
-        case VBE_DISPI_INDEX_VIRT_WIDTH:
-            {
-                int w, h, line_offset;
-
-                if (val < s->vbe_regs[VBE_DISPI_INDEX_XRES])
-                    return;
-                w = val;
-                if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4)
-                    line_offset = w >> 1;
-                else
-                    line_offset = w * ((s->vbe_regs[VBE_DISPI_INDEX_BPP] + 7) >> 3);
-                h = s->vbe_size / line_offset;
-                /* XXX: support weird bochs semantics ? */
-                if (h < s->vbe_regs[VBE_DISPI_INDEX_YRES])
-                    return;
-                s->vbe_regs[VBE_DISPI_INDEX_VIRT_WIDTH] = w;
-                s->vbe_regs[VBE_DISPI_INDEX_VIRT_HEIGHT] = h;
-                s->vbe_line_offset = line_offset;
-            }
-            break;
-        case VBE_DISPI_INDEX_X_OFFSET:
-        case VBE_DISPI_INDEX_Y_OFFSET:
-            {
-                int x;
-                s->vbe_regs[s->vbe_index] = val;
-                s->vbe_start_addr = s->vbe_line_offset * s->vbe_regs[VBE_DISPI_INDEX_Y_OFFSET];
-                x = s->vbe_regs[VBE_DISPI_INDEX_X_OFFSET];
-                if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4)
-                    s->vbe_start_addr += x >> 1;
-                else
-                    s->vbe_start_addr += x * ((s->vbe_regs[VBE_DISPI_INDEX_BPP] + 7) >> 3);
-                s->vbe_start_addr >>= 2;
-            }
-            break;
         default:
             break;
         }
-- 
1.8.3.1

[Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Gerd Hoffmann]

Related spice-only bug.  We have a fixed 16 MB buffer here, being
presented to the spice-server as qxl video memory in case spice is
used with a non-qxl card.  It's also used with qxl in vga mode.

When using display resolutions requiring more than 16 MB of memory we
are going to overflow that buffer.  In theory the guest can write,
indirectly via spice-server.  The spice-server clears the memory after
setting a new video mode though, triggering a segfault in the overflow
case, so qemu crashes before the guest has a chance to do something
evil.

Fix that by switching to dynamic allocation for the buffer.

CVE-2014-3615

Cc: qemu-stable@nongnu.org
Cc: secalert@redhat.com
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 66e2578..57a8fd0 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -334,6 +334,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
+    uint64_t surface_size;
 
     memset(&surface, 0, sizeof(surface));
 
@@ -347,9 +348,18 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
     surface.mouse_mode = true;
     surface.flags      = 0;
     surface.type       = 0;
-    surface.mem        = (uintptr_t)ssd->buf;
     surface.group_id   = MEMSLOT_GROUP_HOST;
 
+    surface_size = surface.width * surface.height * 4;
+    if (ssd->bufsize < surface_size) {
+        ssd->bufsize = surface_size;
+        fprintf(stderr, "%s: %" PRId64 " (%dx%d)\n", __func__,
+                surface_size, surface.width, surface.height);
+        g_free(ssd->buf);
+        ssd->buf = g_malloc(ssd->bufsize);
+    }
+    surface.mem = (uintptr_t)ssd->buf;
+
     qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC);
 }
 
@@ -369,8 +379,6 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
     if (ssd->num_surfaces == 0) {
         ssd->num_surfaces = 1024;
     }
-    ssd->bufsize = (16 * 1024 * 1024);
-    ssd->buf = g_malloc(ssd->bufsize);
 }
 
 /* display listener callbacks */
@@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->num_memslots = NUM_MEMSLOTS;
     info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
     info->internal_groupslot_id = 0;
-    info->qxl_ram_size = ssd->bufsize;
+    info->qxl_ram_size = 16 * 1024 * 1024;
     info->n_surfaces = ssd->num_surfaces;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Laszlo Ersek]

On 09/04/14 09:04, Gerd Hoffmann wrote:
> Related spice-only bug.  We have a fixed 16 MB buffer here, being
> presented to the spice-server as qxl video memory in case spice is
> used with a non-qxl card.  It's also used with qxl in vga mode.
> 
> When using display resolutions requiring more than 16 MB of memory we
> are going to overflow that buffer.  In theory the guest can write,
> indirectly via spice-server.  The spice-server clears the memory after
> setting a new video mode though, triggering a segfault in the overflow
> case, so qemu crashes before the guest has a chance to do something
> evil.
> 
> Fix that by switching to dynamic allocation for the buffer.
> 
> CVE-2014-3615
> 
> Cc: qemu-stable@nongnu.org
> Cc: secalert@redhat.com
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/spice-display.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 66e2578..57a8fd0 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -334,6 +334,7 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>  {
>      QXLDevSurfaceCreate surface;
> +    uint64_t surface_size;
>  
>      memset(&surface, 0, sizeof(surface));
>  
> @@ -347,9 +348,18 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>      surface.mouse_mode = true;
>      surface.flags      = 0;
>      surface.type       = 0;
> -    surface.mem        = (uintptr_t)ssd->buf;
>      surface.group_id   = MEMSLOT_GROUP_HOST;
>  
> +    surface_size = surface.width * surface.height * 4;

(1) surface.width and surface.height are uint32_t fields
[spice-server/server/spice.h]:

struct QXLDevSurfaceCreate {
    uint32_t width;
    uint32_t height;

uint32_t equals "unsigned int" in our case, hence the multiplication
will be carried out in "unsigned int" -- 32-bits. Given that the
dimensions here are under guest control, I suggest to write it as

    surface_size = (uint64_t)surface.width * surface.height * 4;

(2) However, I have a concern even that way.

The above multiplication (due to the *4) can overflow in uint64_t as well.

I can see that "surface.width" and "surface.height" come from
pixman_image_get_width() and pixman_image_get_height(), which seem to
return "int" values. Hence,

    (uint64_t)0x7FFF_FFFF * 0x7FFF_FFFF * 4 == 0xFFFF_FFFC_0000_0004

which is OK. But, what if pixman returns a negative value? Can we create
an image that big?

>From qemu_spice_create_one_update():

    int bw, bh;

    bw       = rect->right - rect->left;
    bh       = rect->bottom - rect->top;

    dest = pixman_image_create_bits(PIXMAN_x8r8g8b8, bw, bh,
                                    (void *)update->bitmap, bw * 4);

where

typedef struct SPICE_ATTR_PACKED QXLRect {
    int32_t top;
    int32_t left;
    int32_t bottom;
    int32_t right;
} QXLRect;

....

I can't track this back far enough. I'd feel safer if you checked that
the multiplication can't overflow even in uint64_t.

(3) In addition:

> +    if (ssd->bufsize < surface_size) {
> +        ssd->bufsize = surface_size;

struct SimpleSpiceDisplay {
[...]
    int bufsize;

Meaning, even if the multiplication fits okay in an uint64_t, the above
assignment can overflow ssd->bufsize, because that's just an int.

> +        fprintf(stderr, "%s: %" PRId64 " (%dx%d)\n", __func__,
> +                surface_size, surface.width, surface.height);

(4) surface_size is uint64_t, it should be printed with PRIu64, not
PRId64. Similarly, surface.width and surface.height are uint32_t, they
should be printed with either %u or PRIu32, not %d.

> +        g_free(ssd->buf);
> +        ssd->buf = g_malloc(ssd->bufsize);

Then, g_malloc() takes a "gsize", which means "unsigned long":

https://developer.gnome.org/glib/2.28/glib-Memory-Allocation.html#g-malloc
https://developer.gnome.org/glib/2.28/glib-Basic-Types.html#gsize

which has the range of uint32_t in an ILP32 (i686) build. Therefore even
changing the type of ssd->bufsize to uint64_t wouldn't help.

(5) Instead, you really need to make sure that the very first
multiplication fits in a signed int:

    int width, height;

    width = surface_width(ssd->ds);
    height = surface_height(ssd->ds);

    if (width <= 0 || height <= 0 ||
        width > INT_MAX / 4 ||
        height > INT_MAX / (width * 4)) {
        /* won't ever fit */
        abort();
    }

    if (ssd->bufsize < width * height * 4) {
        ssd->bufsize = width * height * 4;
        /* do the rest of the realloc */
    }

and do everything else after, even the population of "surface"'s fields.

> +    }
> +    surface.mem = (uintptr_t)ssd->buf;
> +
>      qemu_spice_create_primary_surface(ssd, 0, &surface, QXL_SYNC);
>  }
>  
> @@ -369,8 +379,6 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
>      if (ssd->num_surfaces == 0) {
>          ssd->num_surfaces = 1024;
>      }
> -    ssd->bufsize = (16 * 1024 * 1024);
> -    ssd->buf = g_malloc(ssd->bufsize);
>  }

I'm okay with this as long as you guarantee that the dimensions the
guest specifies will be strictly greater than zero. Otherwise, the
product could be zero, and I quite dislike g_malloc(0) calls -- "If
n_bytes is 0 it returns NULL", which could be problematic elsewhere.

The code I proposed above makes sure that both dimensions are positive.

>  
>  /* display listener callbacks */
> @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>      info->num_memslots = NUM_MEMSLOTS;
>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>      info->internal_groupslot_id = 0;
> -    info->qxl_ram_size = ssd->bufsize;
> +    info->qxl_ram_size = 16 * 1024 * 1024;
>      info->n_surfaces = ssd->num_surfaces;
>  }

Is it safe to detach these two from each other? You could actually leave
the initial 16MB alloc in place.

Thanks
Laszlo

Re: [Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Gerd Hoffmann]

  Hi,

> I can't track this back far enough. I'd feel safer if you checked that
> the multiplication can't overflow even in uint64_t.

Effectively it comes from the emulated graphics hardware (anything in
hw/display/*).  The gfx emulation must make sure that the framebuffer
fits into the video memory, which in turn pretty much implies that we
can't overflow uint64_t.  I think even uint32_t can't overflow with the
gfx hardware we are emulating today.

> (5) Instead, you really need to make sure that the very first
> multiplication fits in a signed int:

Makes sense.  I think it is easier to just multiply in 64bit, then check
the result is small enougth (new patch attached).

> >  /* display listener callbacks */
> > @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
> >      info->num_memslots = NUM_MEMSLOTS;
> >      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
> >      info->internal_groupslot_id = 0;
> > -    info->qxl_ram_size = ssd->bufsize;
> > +    info->qxl_ram_size = 16 * 1024 * 1024;
> >      info->n_surfaces = ssd->num_surfaces;
> >  }

spice-server doesn't do anything with it, other than passing to
spice-client.  Not fully sure what spice-client uses this for, maybe as
some kind of hint for resource management.  Maybe not at all.

It surely doesn't matter at all for ssd->buf size.

cheers,
  Gerd

Re: [Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Laszlo Ersek]

On 09/05/14 10:58, Gerd Hoffmann wrote:
>   Hi,
> 
>> I can't track this back far enough. I'd feel safer if you checked that
>> the multiplication can't overflow even in uint64_t.
> 
> Effectively it comes from the emulated graphics hardware (anything in
> hw/display/*).  The gfx emulation must make sure that the framebuffer
> fits into the video memory, which in turn pretty much implies that we
> can't overflow uint64_t.  I think even uint32_t can't overflow with the
> gfx hardware we are emulating today.
> 
>> (5) Instead, you really need to make sure that the very first
>> multiplication fits in a signed int:
> 
> Makes sense.  I think it is easier to just multiply in 64bit, then check
> the result is small enougth (new patch attached).

Okay, if you can guarantee that the product fits in uint64_t, then such
a check would suffice.

New patch has not been attached though :)

> 
>>>  /* display listener callbacks */
>>> @@ -495,7 +503,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>>>      info->num_memslots = NUM_MEMSLOTS;
>>>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>>>      info->internal_groupslot_id = 0;
>>> -    info->qxl_ram_size = ssd->bufsize;
>>> +    info->qxl_ram_size = 16 * 1024 * 1024;
>>>      info->n_surfaces = ssd->num_surfaces;
>>>  }
> 
> spice-server doesn't do anything with it, other than passing to
> spice-client.  Not fully sure what spice-client uses this for, maybe as
> some kind of hint for resource management.  Maybe not at all.
> 
> It surely doesn't matter at all for ssd->buf size.

Okay, I'll trust you on this.

Thanks
Laszlo

Re: [Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

On Fr, 2014-09-05 at 11:06 +0200, Laszlo Ersek wrote:
> > Makes sense.  I think it is easier to just multiply in 64bit, then
> check
> > the result is small enougth (new patch attached).
> 
> Okay, if you can guarantee that the product fits in uint64_t, then
> such
> a check would suffice.
> 
> New patch has not been attached though :)

Oops.  Here we go.

cheers,
  Gerd

[-- Attachment #2: 0001-spice-make-sure-we-don-t-overflow-ssd-buf.patch --]
[-- Type: text/x-patch, Size: 2840 bytes --]

>From 33c5c3d1736fd577fc1279a1f3c50d2414e98fe3 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 3 Sep 2014 15:50:08 +0200
Subject: [PATCH] spice: make sure we don't overflow ssd->buf

Related spice-only bug.  We have a fixed 16 MB buffer here, being
presented to the spice-server as qxl video memory in case spice is
used with a non-qxl card.  It's also used with qxl in vga mode.

When using display resolutions requiring more than 16 MB of memory we
are going to overflow that buffer.  In theory the guest can write,
indirectly via spice-server.  The spice-server clears the memory after
setting a new video mode though, triggering a segfault in the overflow
case, so qemu crashes before the guest has a chance to do something
evil.

Fix that by switching to dynamic allocation for the buffer.

CVE-2014-3615

Cc: qemu-stable@nongnu.org
Cc: secalert@redhat.com
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 66e2578..def7b52 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -334,11 +334,23 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
 void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
 {
     QXLDevSurfaceCreate surface;
+    uint64_t surface_size;
 
     memset(&surface, 0, sizeof(surface));
 
-    dprint(1, "%s/%d: %dx%d\n", __func__, ssd->qxl.id,
-           surface_width(ssd->ds), surface_height(ssd->ds));
+    surface_size = (uint64_t) surface_width(ssd->ds) *
+        surface_height(ssd->ds) * 4;
+    assert(surface_size > 0);
+    assert(surface_size < INT_MAX);
+    if (ssd->bufsize < surface_size) {
+        ssd->bufsize = surface_size;
+        g_free(ssd->buf);
+        ssd->buf = g_malloc(ssd->bufsize);
+    }
+
+    dprint(1, "%s/%d: %ux%u (size %" PRIu64 "/%d)\n", __func__, ssd->qxl.id,
+           surface_width(ssd->ds), surface_height(ssd->ds),
+           surface_size, ssd->bufsize);
 
     surface.format     = SPICE_SURFACE_FMT_32_xRGB;
     surface.width      = surface_width(ssd->ds);
@@ -369,8 +381,6 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
     if (ssd->num_surfaces == 0) {
         ssd->num_surfaces = 1024;
     }
-    ssd->bufsize = (16 * 1024 * 1024);
-    ssd->buf = g_malloc(ssd->bufsize);
 }
 
 /* display listener callbacks */
@@ -495,7 +505,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
     info->num_memslots = NUM_MEMSLOTS;
     info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
     info->internal_groupslot_id = 0;
-    info->qxl_ram_size = ssd->bufsize;
+    info->qxl_ram_size = 16 * 1024 * 1024;
     info->n_surfaces = ssd->num_surfaces;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [CVE-2014-3615 PATCH v2 3/3] spice: make sure we don't overflow ssd->buf

[Laszlo Ersek]

On 09/05/14 11:33, Gerd Hoffmann wrote:
> On Fr, 2014-09-05 at 11:06 +0200, Laszlo Ersek wrote:
>>> > > Makes sense.  I think it is easier to just multiply in 64bit, then
>> > check
>>> > > the result is small enougth (new patch attached).
>> > 
>> > Okay, if you can guarantee that the product fits in uint64_t, then
>> > such
>> > a check would suffice.
>> > 
>> > New patch has not been attached though :)
> Oops.  Here we go.
> 
> cheers,
>   Gerd
> 
> 
> 0001-spice-make-sure-we-don-t-overflow-ssd-buf.patch
> 
> 
> From 33c5c3d1736fd577fc1279a1f3c50d2414e98fe3 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 3 Sep 2014 15:50:08 +0200
> Subject: [PATCH] spice: make sure we don't overflow ssd->buf
> 
> Related spice-only bug.  We have a fixed 16 MB buffer here, being
> presented to the spice-server as qxl video memory in case spice is
> used with a non-qxl card.  It's also used with qxl in vga mode.
> 
> When using display resolutions requiring more than 16 MB of memory we
> are going to overflow that buffer.  In theory the guest can write,
> indirectly via spice-server.  The spice-server clears the memory after
> setting a new video mode though, triggering a segfault in the overflow
> case, so qemu crashes before the guest has a chance to do something
> evil.
> 
> Fix that by switching to dynamic allocation for the buffer.
> 
> CVE-2014-3615
> 
> Cc: qemu-stable@nongnu.org
> Cc: secalert@redhat.com
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/spice-display.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 66e2578..def7b52 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -334,11 +334,23 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay *ssd)
>  void qemu_spice_create_host_primary(SimpleSpiceDisplay *ssd)
>  {
>      QXLDevSurfaceCreate surface;
> +    uint64_t surface_size;
>  
>      memset(&surface, 0, sizeof(surface));
>  
> -    dprint(1, "%s/%d: %dx%d\n", __func__, ssd->qxl.id,
> -           surface_width(ssd->ds), surface_height(ssd->ds));
> +    surface_size = (uint64_t) surface_width(ssd->ds) *
> +        surface_height(ssd->ds) * 4;
> +    assert(surface_size > 0);
> +    assert(surface_size < INT_MAX);
> +    if (ssd->bufsize < surface_size) {
> +        ssd->bufsize = surface_size;
> +        g_free(ssd->buf);
> +        ssd->buf = g_malloc(ssd->bufsize);
> +    }
> +
> +    dprint(1, "%s/%d: %ux%u (size %" PRIu64 "/%d)\n", __func__, ssd->qxl.id,
> +           surface_width(ssd->ds), surface_height(ssd->ds),
> +           surface_size, ssd->bufsize);
>  
>      surface.format     = SPICE_SURFACE_FMT_32_xRGB;
>      surface.width      = surface_width(ssd->ds);
> @@ -369,8 +381,6 @@ void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd)
>      if (ssd->num_surfaces == 0) {
>          ssd->num_surfaces = 1024;
>      }
> -    ssd->bufsize = (16 * 1024 * 1024);
> -    ssd->buf = g_malloc(ssd->bufsize);
>  }
>  
>  /* display listener callbacks */
> @@ -495,7 +505,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info)
>      info->num_memslots = NUM_MEMSLOTS;
>      info->num_memslots_groups = NUM_MEMSLOTS_GROUPS;
>      info->internal_groupslot_id = 0;
> -    info->qxl_ram_size = ssd->bufsize;
> +    info->qxl_ram_size = 16 * 1024 * 1024;
>      info->n_surfaces = ssd->num_surfaces;
>  }
>  
> -- 1.8.3.1
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>