From 3e095b753b0ff9dd22bf572ab146b9cad7628237 Mon Sep 17 00:00:00 2001 Message-Id: <3e095b753b0ff9dd22bf572ab146b9cad7628237.1427148003.git.jen@redhat.com> In-Reply-To: References: From: Gerd Hoffmann Date: Wed, 11 Mar 2015 15:11:02 -0500 Subject: [CHANGE 8/9] vbe: rework sanity checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To: rhvirt-patches@redhat.com, jen@redhat.com RH-Author: Gerd Hoffmann Message-id: <1426086663-17937-9-git-send-email-kraxel@redhat.com> Patchwork-id: 64290 O-Subject: [RHEL-6.7 qemu-kvm PATCH v4 8/9] vbe: rework sanity checks Bugzilla: 1053039 RH-Acked-by: Dr. David Alan Gilbert RH-Acked-by: Marc-André Lureau RH-Acked-by: Laszlo Ersek 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 (cherry picked from commit c1b886c45dc70f247300f549dce9833f3fa2def5) Signed-off-by: Jeff E. Nelson Conflicts: hw/vga.c --- hw/vga.c | 154 +++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 95 insertions(+), 59 deletions(-) Signed-off-by: Jeff E. Nelson --- hw/vga.c | 154 +++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 95 insertions(+), 59 deletions(-) diff --git a/hw/vga.c b/hw/vga.c index cb4ae18..6cedfe1 100644 --- a/hw/vga.c +++ b/hw/vga.c @@ -510,6 +510,93 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) } #ifdef CONFIG_BOCHS_VBE +/* + * 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; @@ -579,22 +666,13 @@ static 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) { @@ -610,19 +688,11 @@ static 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)) { @@ -664,40 +734,6 @@ static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val) s->dac_8bit = (val & VBE_DISPI_8BIT_DAC) > 0; s->vbe_regs[s->vbe_index] = val; 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; } -- 2.1.0