From 54a85d462447c1cb8a1638578a7fd086350b4d2d Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 26 Aug 2014 14:16:30 +0200 Subject: [PATCH 1/3] vbe: make bochs dispi interface return the correct memory size with qxl 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 Reviewed-by: Laszlo Ersek --- 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 d43aa49eb8..652af99001 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 65dab8dbed..99251d7698 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 641f8f41d2..bbc0cb2ad8 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; From c1b886c45dc70f247300f549dce9833f3fa2def5 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 26 Aug 2014 15:35:23 +0200 Subject: [PATCH 2/3] vbe: rework sanity checks 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 Reviewed-by: Laszlo Ersek --- 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 99251d7698..62e6243c6f 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; } From ab9509cceabef28071e41bdfa073083859c949a7 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Wed, 3 Sep 2014 15:50:08 +0200 Subject: [PATCH 3/3] 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 Signed-off-by: Gerd Hoffmann Reviewed-by: Laszlo Ersek --- 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 66e25788ce..def7b52e9c 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; }