[Qemu-devel] [PULL for-2.6 0/5] vga security fixes (CVE-2016-3710, CVE-2016-3712)

[Qemu-devel] [PULL for-2.6 0/5] vga security fixes (CVE-2016-3710, CVE-2016-3712)

[Gerd Hoffmann]

  Hi,

Here comes a pull request for 2.6, fixing two security issues in the
vga emulation code.

The first one (CVE-2016-3710, patch #1) is pretty serious, allowing the
guest read and write host memory.  Possibly allows the guest to break
out of the vm.

The second one (CVE-2016-3712) is a read overflow.  DoS only (allows the
guest crash qemu).

Both flaws are simliar:  Programming the vga using both bochs vbe
registers and standard vga registers, create a unusual video mode,
bypass sanity checks that way.  See actual patch descriptions for more
details.

please pull,
  Gerd

The following changes since commit 277abf15a60f7653bfb05ffb513ed74ffdaea1b7:

  configure: Check if struct fsxattr is available from linux header (2016-05-02 13:04:26 +0100)

are available in the git repository at:

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

for you to fetch changes up to fd3c136b3e1482cd0ec7285d6bc2a3e6a62c38d7:

  vga: make sure vga register setup for vbe stays intact (CVE-2016-3712). (2016-05-02 16:02:59 +0200)

----------------------------------------------------------------
vga security fixes (CVE-2016-3710, CVE-2016-3712)

----------------------------------------------------------------
Gerd Hoffmann (5):
      vga: fix banked access bounds checking (CVE-2016-3710)
      vga: add vbe_enabled() helper
      vga: factor out vga register setup
      vga: update vga register setup on vbe changes
      vga: make sure vga register setup for vbe stays intact (CVE-2016-3712).

 hw/display/vga.c | 122 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 78 insertions(+), 44 deletions(-)

[Qemu-devel] [PULL 1/5] vga: fix banked access bounds checking (CVE-2016-3710)

[Gerd Hoffmann]

vga allows banked access to video memory using the window at 0xa00000
and it supports a different access modes with different address
calculations.

The VBE bochs extentions support banked access too, using the
VBE_DISPI_INDEX_BANK register.  The code tries to take the different
address calculations into account and applies different limits to
VBE_DISPI_INDEX_BANK depending on the current access mode.

Which is probably effective in stopping misprogramming by accident.
But from a security point of view completely useless as an attacker
can easily change access modes after setting the bank register.

Drop the bogus check, add range checks to vga_mem_{readb,writeb}
instead.

Fixes: CVE-2016-3710
Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 657e9f1..b9191ca 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -179,6 +179,7 @@ static void vga_update_memory_access(VGACommonState *s)
             size = 0x8000;
             break;
         }
+        assert(offset + size <= s->vram_size);
         memory_region_init_alias(&s->chain4_alias, memory_region_owner(&s->vram),
                                  "vga.chain4", &s->vram, offset, size);
         memory_region_add_subregion_overlap(s->legacy_address_space, base,
@@ -716,11 +717,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
             vbe_fixup_regs(s);
             break;
         case VBE_DISPI_INDEX_BANK:
-            if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
-              val &= (s->vbe_bank_mask >> 2);
-            } else {
-              val &= s->vbe_bank_mask;
-            }
+            val &= s->vbe_bank_mask;
             s->vbe_regs[s->vbe_index] = val;
             s->bank_offset = (val << 16);
             vga_update_memory_access(s);
@@ -819,13 +816,21 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr)
 
     if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) {
         /* chain 4 mode : simplest access */
+        assert(addr < s->vram_size);
         ret = s->vram_ptr[addr];
     } else if (s->gr[VGA_GFX_MODE] & 0x10) {
         /* odd/even mode (aka text mode mapping) */
         plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1);
-        ret = s->vram_ptr[((addr & ~1) << 1) | plane];
+        addr = ((addr & ~1) << 1) | plane;
+        if (addr >= s->vram_size) {
+            return 0xff;
+        }
+        ret = s->vram_ptr[addr];
     } else {
         /* standard VGA latched access */
+        if (addr * sizeof(uint32_t) >= s->vram_size) {
+            return 0xff;
+        }
         s->latch = ((uint32_t *)s->vram_ptr)[addr];
 
         if (!(s->gr[VGA_GFX_MODE] & 0x08)) {
@@ -882,6 +887,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         plane = addr & 3;
         mask = (1 << plane);
         if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) {
+            assert(addr < s->vram_size);
             s->vram_ptr[addr] = val;
 #ifdef DEBUG_VGA_MEM
             printf("vga: chain4: [0x" TARGET_FMT_plx "]\n", addr);
@@ -895,6 +901,9 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         mask = (1 << plane);
         if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) {
             addr = ((addr & ~1) << 1) | plane;
+            if (addr >= s->vram_size) {
+                return;
+            }
             s->vram_ptr[addr] = val;
 #ifdef DEBUG_VGA_MEM
             printf("vga: odd/even: [0x" TARGET_FMT_plx "]\n", addr);
@@ -968,6 +977,9 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val)
         mask = s->sr[VGA_SEQ_PLANE_WRITE];
         s->plane_updated |= mask; /* only used to detect font change */
         write_mask = mask16[mask];
+        if (addr * sizeof(uint32_t) >= s->vram_size) {
+            return;
+        }
         ((uint32_t *)s->vram_ptr)[addr] =
             (((uint32_t *)s->vram_ptr)[addr] & ~write_mask) |
             (val & write_mask);
-- 
1.8.3.1

[Qemu-devel] [PULL 2/5] vga: add vbe_enabled() helper

[Gerd Hoffmann]

Makes code a bit easier to read.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index b9191ca..0c1c5b5 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -142,6 +142,11 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
+static inline bool vbe_enabled(VGACommonState *s)
+{
+    return s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED;
+}
+
 static void vga_update_memory_access(VGACommonState *s)
 {
     hwaddr base, offset, size;
@@ -564,7 +569,7 @@ 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)) {
+    if (!vbe_enabled(s)) {
         /* vbe is turned off -- nothing to do */
         return;
     }
@@ -1058,7 +1063,7 @@ static void vga_get_offsets(VGACommonState *s,
 {
     uint32_t start_addr, line_offset, line_compare;
 
-    if (s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED) {
+    if (vbe_enabled(s)) {
         line_offset = s->vbe_line_offset;
         start_addr = s->vbe_start_addr;
         line_compare = 65535;
@@ -1383,7 +1388,7 @@ static int vga_get_bpp(VGACommonState *s)
 {
     int ret;
 
-    if (s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED) {
+    if (vbe_enabled(s)) {
         ret = s->vbe_regs[VBE_DISPI_INDEX_BPP];
     } else {
         ret = 0;
@@ -1395,7 +1400,7 @@ static void vga_get_resolution(VGACommonState *s, int *pwidth, int *pheight)
 {
     int width, height;
 
-    if (s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED) {
+    if (vbe_enabled(s)) {
         width = s->vbe_regs[VBE_DISPI_INDEX_XRES];
         height = s->vbe_regs[VBE_DISPI_INDEX_YRES];
     } else {
-- 
1.8.3.1

[Qemu-devel] [PULL 3/5] vga: factor out vga register setup

[Gerd Hoffmann]

When enabling vbe mode qemu will setup a bunch of vga registers to make
sure the vga emulation operates in correct mode for a linear
framebuffer.  Move that code to a separate function so we can call it
from other places too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 78 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 0c1c5b5..e12f5ac 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -644,6 +644,49 @@ static void vbe_fixup_regs(VGACommonState *s)
     s->vbe_start_addr  = offset / 4;
 }
 
+/* we initialize the VGA graphic mode */
+static void vbe_update_vgaregs(VGACommonState *s)
+{
+    int h, shift_control;
+
+    if (!vbe_enabled(s)) {
+        /* vbe is turned off -- nothing to do */
+        return;
+    }
+
+    /* graphic mode + memory map 1 */
+    s->gr[VGA_GFX_MISC] = (s->gr[VGA_GFX_MISC] & ~0x0c) | 0x04 |
+        VGA_GR06_GRAPHICS_MODE;
+    s->cr[VGA_CRTC_MODE] |= 3; /* no CGA modes */
+    s->cr[VGA_CRTC_OFFSET] = s->vbe_line_offset >> 3;
+    /* width */
+    s->cr[VGA_CRTC_H_DISP] =
+        (s->vbe_regs[VBE_DISPI_INDEX_XRES] >> 3) - 1;
+    /* height (only meaningful if < 1024) */
+    h = s->vbe_regs[VBE_DISPI_INDEX_YRES] - 1;
+    s->cr[VGA_CRTC_V_DISP_END] = h;
+    s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW] & ~0x42) |
+        ((h >> 7) & 0x02) | ((h >> 3) & 0x40);
+    /* line compare to 1023 */
+    s->cr[VGA_CRTC_LINE_COMPARE] = 0xff;
+    s->cr[VGA_CRTC_OVERFLOW] |= 0x10;
+    s->cr[VGA_CRTC_MAX_SCAN] |= 0x40;
+
+    if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
+        shift_control = 0;
+        s->sr[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */
+    } else {
+        shift_control = 2;
+        /* set chain 4 mode */
+        s->sr[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M;
+        /* activate all planes */
+        s->sr[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES;
+    }
+    s->gr[VGA_GFX_MODE] = (s->gr[VGA_GFX_MODE] & ~0x60) |
+        (shift_control << 5);
+    s->cr[VGA_CRTC_MAX_SCAN] &= ~0x9f; /* no double scan */
+}
+
 static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
 {
     VGACommonState *s = opaque;
@@ -730,52 +773,19 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
         case VBE_DISPI_INDEX_ENABLE:
             if ((val & VBE_DISPI_ENABLED) &&
                 !(s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED)) {
-                int h, shift_control;
 
                 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;
                 s->vbe_regs[VBE_DISPI_INDEX_ENABLE] |= VBE_DISPI_ENABLED;
                 vbe_fixup_regs(s);
+                vbe_update_vgaregs(s);
 
                 /* clear the screen */
                 if (!(val & VBE_DISPI_NOCLEARMEM)) {
                     memset(s->vram_ptr, 0,
                            s->vbe_regs[VBE_DISPI_INDEX_YRES] * s->vbe_line_offset);
                 }
-
-                /* we initialize the VGA graphic mode */
-                /* graphic mode + memory map 1 */
-                s->gr[VGA_GFX_MISC] = (s->gr[VGA_GFX_MISC] & ~0x0c) | 0x04 |
-                    VGA_GR06_GRAPHICS_MODE;
-                s->cr[VGA_CRTC_MODE] |= 3; /* no CGA modes */
-                s->cr[VGA_CRTC_OFFSET] = s->vbe_line_offset >> 3;
-                /* width */
-                s->cr[VGA_CRTC_H_DISP] =
-                    (s->vbe_regs[VBE_DISPI_INDEX_XRES] >> 3) - 1;
-                /* height (only meaningful if < 1024) */
-                h = s->vbe_regs[VBE_DISPI_INDEX_YRES] - 1;
-                s->cr[VGA_CRTC_V_DISP_END] = h;
-                s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW] & ~0x42) |
-                    ((h >> 7) & 0x02) | ((h >> 3) & 0x40);
-                /* line compare to 1023 */
-                s->cr[VGA_CRTC_LINE_COMPARE] = 0xff;
-                s->cr[VGA_CRTC_OVERFLOW] |= 0x10;
-                s->cr[VGA_CRTC_MAX_SCAN] |= 0x40;
-
-                if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
-                    shift_control = 0;
-                    s->sr[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */
-                } else {
-                    shift_control = 2;
-                    /* set chain 4 mode */
-                    s->sr[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M;
-                    /* activate all planes */
-                    s->sr[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES;
-                }
-                s->gr[VGA_GFX_MODE] = (s->gr[VGA_GFX_MODE] & ~0x60) |
-                    (shift_control << 5);
-                s->cr[VGA_CRTC_MAX_SCAN] &= ~0x9f; /* no double scan */
             } else {
                 s->bank_offset = 0;
             }
-- 
1.8.3.1

[Qemu-devel] [PULL 4/5] vga: update vga register setup on vbe changes

[Gerd Hoffmann]

Call the new vbe_update_vgaregs() function on vbe configuration
changes, to make sure vga registers are up-to-date.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index e12f5ac..eeeb9c8 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -763,6 +763,7 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
         case VBE_DISPI_INDEX_Y_OFFSET:
             s->vbe_regs[s->vbe_index] = val;
             vbe_fixup_regs(s);
+            vbe_update_vgaregs(s);
             break;
         case VBE_DISPI_INDEX_BANK:
             val &= s->vbe_bank_mask;
-- 
1.8.3.1

[Qemu-devel] [PULL 5/5] vga: make sure vga register setup for vbe stays intact (CVE-2016-3712).

[Gerd Hoffmann]

Call vbe_update_vgaregs() when the guest touches GFX, SEQ or CRT
registers, to make sure the vga registers will always have the
values needed by vbe mode.  This makes sure the sanity checks
applied by vbe_fixup_regs() are effective.

Without this guests can muck with shift_control, can turn on planar
vga modes or text mode emulation while VBE is active, making qemu
take code paths meant for CGA compatibility, but with the very
large display widths and heigts settable using VBE registers.

Which is good for one or another buffer overflow.  Not that
critical as they typically read overflows happening somewhere
in the display code.  So guests can DoS by crashing qemu with a
segfault, but it is probably not possible to break out of the VM.

Fixes: CVE-2016-3712
Reported-by: Zuozhi Fzz <zuozhi.fzz@alibaba-inc.com>
Reported-by: P J P <ppandit@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index eeeb9c8..4a55ec6 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -142,6 +142,8 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];
 
+static void vbe_update_vgaregs(VGACommonState *s);
+
 static inline bool vbe_enabled(VGACommonState *s)
 {
     return s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED;
@@ -484,6 +486,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         printf("vga: write SR%x = 0x%02x\n", s->sr_index, val);
 #endif
         s->sr[s->sr_index] = val & sr_mask[s->sr_index];
+        vbe_update_vgaregs(s);
         if (s->sr_index == VGA_SEQ_CLOCK_MODE) {
             s->update_retrace_info(s);
         }
@@ -515,6 +518,7 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         printf("vga: write GR%x = 0x%02x\n", s->gr_index, val);
 #endif
         s->gr[s->gr_index] = val & gr_mask[s->gr_index];
+        vbe_update_vgaregs(s);
         vga_update_memory_access(s);
         break;
     case VGA_CRT_IM:
@@ -533,10 +537,12 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             if (s->cr_index == VGA_CRTC_OVERFLOW) {
                 s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW] & ~0x10) |
                     (val & 0x10);
+                vbe_update_vgaregs(s);
             }
             return;
         }
         s->cr[s->cr_index] = val;
+        vbe_update_vgaregs(s);
 
         switch(s->cr_index) {
         case VGA_CRTC_H_TOTAL:
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.6 0/5] vga security fixes (CVE-2016-3710, CVE-2016-3712)

[Peter Maydell]

On 9 May 2016 at 13:51, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Here comes a pull request for 2.6, fixing two security issues in the
> vga emulation code.
>
> The first one (CVE-2016-3710, patch #1) is pretty serious, allowing the
> guest read and write host memory.  Possibly allows the guest to break
> out of the vm.
>
> The second one (CVE-2016-3712) is a read overflow.  DoS only (allows the
> guest crash qemu).
>
> Both flaws are simliar:  Programming the vga using both bochs vbe
> registers and standard vga registers, create a unusual video mode,
> bypass sanity checks that way.  See actual patch descriptions for more
> details.
>
> please pull,
>   Gerd
>
> The following changes since commit 277abf15a60f7653bfb05ffb513ed74ffdaea1b7:
>
>   configure: Check if struct fsxattr is available from linux header (2016-05-02 13:04:26 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/pull-vga-20160509-1
>
> for you to fetch changes up to fd3c136b3e1482cd0ec7285d6bc2a3e6a62c38d7:
>
>   vga: make sure vga register setup for vbe stays intact (CVE-2016-3712). (2016-05-02 16:02:59 +0200)
>
> ----------------------------------------------------------------
> vga security fixes (CVE-2016-3710, CVE-2016-3712)
>
> ----------------------------------------------------------------

Applied to master, thanks. That was all we were waiting for to
release 2.6, so I will tag rc5 this afternoon and barring disaster
tag the final release (same contents) on Wednesday.

thanks
-- PMM