[Qemu-devel] [PATCH v3 0/5] vmware-vga: fix CVE-2014-3689

[Qemu-devel] [PATCH v3 0/5] vmware-vga: fix CVE-2014-3689

[Gerd Hoffmann]

  Hi,

vmware-vga emulation lacks sanity checks in the hardware acceleration
(blit + fill) functions.  This patch series plugs the holes.

v3 changes:
 * throw badcmd errors in case the rectangles fail the sanity checks.
v2 changes:
 * small whitespace fixup.
 * do fullscreen update on invalid update requests.

cheers,
  Gerd

Gerd Hoffmann (5):
  vmware-vga: CVE-2014-3689: turn off hw accel
  vmware-vga: add vmsvga_verify_rect
  vmware-vga: use vmsvga_verify_rect in vmsvga_update_rect
  vmware-vga: use vmsvga_verify_rect in vmsvga_copy_rect
  vmware-vga: use vmsvga_verify_rect in vmsvga_fill_rect

 hw/display/vmware_vga.c | 116 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 78 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/5] vmware-vga: CVE-2014-3689: turn off hw accel

[Gerd Hoffmann]

Quick & easy stopgap for CVE-2014-3689:  We just compile out the
hardware acceleration functions which lack sanity checks.  Thankfully
we have capability bits for them (SVGA_CAP_RECT_COPY and
SVGA_CAP_RECT_FILL), so guests should deal just fine, in theory.

Subsequent patches will add the missing checks and re-enable the
hardware acceleration emulation.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Don Koch <dkoch@verizon.com>
---
 hw/display/vmware_vga.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0c36c72..ec63290 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -29,8 +29,10 @@
 #include "hw/pci/pci.h"
 
 #undef VERBOSE
+#if 0
 #define HW_RECT_ACCEL
 #define HW_FILL_ACCEL
+#endif
 #define HW_MOUSE_ACCEL
 
 #include "vga_int.h"
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/5] vmware-vga: add vmsvga_verify_rect

[Gerd Hoffmann]

Add verification function for rectangles, returning
true if verification passes and false otherwise.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Don Koch <dkoch@verizon.com>
---
 hw/display/vmware_vga.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index ec63290..ba73a1c 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -294,8 +294,59 @@ enum {
     SVGA_CURSOR_ON_RESTORE_TO_FB = 3,
 };
 
+static inline bool vmsvga_verify_rect(DisplaySurface *surface,
+                                      const char *name,
+                                      int x, int y, int w, int h)
+{
+    if (x < 0) {
+        fprintf(stderr, "%s: x was < 0 (%d)\n", name, x);
+        return false;
+    }
+    if (x > SVGA_MAX_WIDTH) {
+        fprintf(stderr, "%s: x was > %d (%d)\n", name, SVGA_MAX_WIDTH, x);
+        return false;
+    }
+    if (w < 0) {
+        fprintf(stderr, "%s: w was < 0 (%d)\n", name, w);
+        return false;
+    }
+    if (w > SVGA_MAX_WIDTH) {
+        fprintf(stderr, "%s: w was > %d (%d)\n", name, SVGA_MAX_WIDTH, w);
+        return false;
+    }
+    if (x + w > surface_width(surface)) {
+        fprintf(stderr, "%s: width was > %d (x: %d, w: %d)\n",
+                name, surface_width(surface), x, w);
+        return false;
+    }
+
+    if (y < 0) {
+        fprintf(stderr, "%s: y was < 0 (%d)\n", name, y);
+        return false;
+    }
+    if (y > SVGA_MAX_HEIGHT) {
+        fprintf(stderr, "%s: y was > %d (%d)\n", name, SVGA_MAX_HEIGHT, y);
+        return false;
+    }
+    if (h < 0) {
+        fprintf(stderr, "%s: h was < 0 (%d)\n", name, h);
+        return false;
+    }
+    if (h > SVGA_MAX_HEIGHT) {
+        fprintf(stderr, "%s: h was > %d (%d)\n", name, SVGA_MAX_HEIGHT, h);
+        return false;
+    }
+    if (y + h > surface_height(surface)) {
+        fprintf(stderr, "%s: update height > %d (y: %d, h: %d)\n",
+                name, surface_height(surface), y, h);
+        return false;
+    }
+
+    return true;
+}
+
 static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
-                int x, int y, int w, int h)
+                                      int x, int y, int w, int h)
 {
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
     int line;
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/5] vmware-vga: use vmsvga_verify_rect in vmsvga_update_rect

[Gerd Hoffmann]

Switch vmsvga_update_rect over to use vmsvga_verify_rect.  Slight change
in behavior:  We don't try to automatically fixup rectangles any more.
In case we find invalid update requests we'll do a full-screen update
instead.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Don Koch <dkoch@verizon.com>
---
 hw/display/vmware_vga.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index ba73a1c..9d79de6 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -356,36 +356,12 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
     uint8_t *src;
     uint8_t *dst;
 
-    if (x < 0) {
-        fprintf(stderr, "%s: update x was < 0 (%d)\n", __func__, x);
-        w += x;
+    if (!vmsvga_verify_rect(surface, __func__, x, y, w, h)) {
+        /* go for a fullscreen update as fallback */
         x = 0;
-    }
-    if (w < 0) {
-        fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w);
-        w = 0;
-    }
-    if (x + w > surface_width(surface)) {
-        fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
-                __func__, x, w);
-        x = MIN(x, surface_width(surface));
-        w = surface_width(surface) - x;
-    }
-
-    if (y < 0) {
-        fprintf(stderr, "%s: update y was < 0 (%d)\n",  __func__, y);
-        h += y;
         y = 0;
-    }
-    if (h < 0) {
-        fprintf(stderr, "%s: update h was < 0 (%d)\n",  __func__, h);
-        h = 0;
-    }
-    if (y + h > surface_height(surface)) {
-        fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
-                __func__, y, h);
-        y = MIN(y, surface_height(surface));
-        h = surface_height(surface) - y;
+        w = surface_width(surface);
+        h = surface_height(surface);
     }
 
     bypl = surface_stride(surface);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 4/5] vmware-vga: use vmsvga_verify_rect in vmsvga_copy_rect

[Gerd Hoffmann]

Add verification to vmsvga_copy_rect, re-enable HW_RECT_ACCEL.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 9d79de6..73a1b52 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -29,8 +29,8 @@
 #include "hw/pci/pci.h"
 
 #undef VERBOSE
-#if 0
 #define HW_RECT_ACCEL
+#if 0
 #define HW_FILL_ACCEL
 #endif
 #define HW_MOUSE_ACCEL
@@ -406,7 +406,7 @@ static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
 }
 
 #ifdef HW_RECT_ACCEL
-static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
+static inline int vmsvga_copy_rect(struct vmsvga_state_s *s,
                 int x0, int y0, int x1, int y1, int w, int h)
 {
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
@@ -417,6 +417,13 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
     int line = h;
     uint8_t *ptr[2];
 
+    if (!vmsvga_verify_rect(surface, "vmsvga_copy_rect/src", x0, y0, w, h)) {
+        return -1;
+    }
+    if (!vmsvga_verify_rect(surface, "vmsvga_copy_rect/dst", x1, y1, w, h)) {
+        return -1;
+    }
+
     if (y1 > y0) {
         ptr[0] = vram + bypp * x0 + bypl * (y0 + h - 1);
         ptr[1] = vram + bypp * x1 + bypl * (y1 + h - 1);
@@ -432,6 +439,7 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
     }
 
     vmsvga_update_rect_delayed(s, x1, y1, w, h);
+    return 0;
 }
 #endif
 
@@ -625,12 +633,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             width = vmsvga_fifo_read(s);
             height = vmsvga_fifo_read(s);
 #ifdef HW_RECT_ACCEL
-            vmsvga_copy_rect(s, x, y, dx, dy, width, height);
-            break;
-#else
+            if (vmsvga_copy_rect(s, x, y, dx, dy, width, height) == 0) {
+                break;
+            }
+#endif
             args = 0;
             goto badcmd;
-#endif
 
         case SVGA_CMD_DEFINE_CURSOR:
             len -= 8;
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 5/5] vmware-vga: use vmsvga_verify_rect in vmsvga_fill_rect

[Gerd Hoffmann]

Add verification to vmsvga_fill_rect, re-enable HW_FILL_ACCEL.

Cc: qemu-stable@nongnu.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 73a1b52..1751f19 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -30,9 +30,7 @@
 
 #undef VERBOSE
 #define HW_RECT_ACCEL
-#if 0
 #define HW_FILL_ACCEL
-#endif
 #define HW_MOUSE_ACCEL
 
 #include "vga_int.h"
@@ -444,7 +442,7 @@ static inline int vmsvga_copy_rect(struct vmsvga_state_s *s,
 #endif
 
 #ifdef HW_FILL_ACCEL
-static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
+static inline int vmsvga_fill_rect(struct vmsvga_state_s *s,
                 uint32_t c, int x, int y, int w, int h)
 {
     DisplaySurface *surface = qemu_console_surface(s->vga.con);
@@ -457,6 +455,10 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
     uint8_t *src;
     uint8_t col[4];
 
+    if (!vmsvga_verify_rect(surface, __func__, x, y, w, h)) {
+        return -1;
+    }
+
     col[0] = c;
     col[1] = c >> 8;
     col[2] = c >> 16;
@@ -481,6 +483,7 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
     }
 
     vmsvga_update_rect_delayed(s, x, y, w, h);
+    return 0;
 }
 #endif
 
@@ -613,12 +616,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             width = vmsvga_fifo_read(s);
             height = vmsvga_fifo_read(s);
 #ifdef HW_FILL_ACCEL
-            vmsvga_fill_rect(s, colour, x, y, width, height);
-            break;
-#else
+            if (vmsvga_fill_rect(s, colour, x, y, width, height) == 0) {
+                break;
+            }
+#endif
             args = 0;
             goto badcmd;
-#endif
 
         case SVGA_CMD_RECT_COPY:
             len -= 7;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/5] vmware-vga: use vmsvga_verify_rect in vmsvga_copy_rect

[Don Koch]

On Tue, 28 Oct 2014 10:50:41 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Add verification to vmsvga_copy_rect, re-enable HW_RECT_ACCEL.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

Reviewed-by: Don Koch <dkoch@verizon.com>

>  hw/display/vmware_vga.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 9d79de6..73a1b52 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -29,8 +29,8 @@
>  #include "hw/pci/pci.h"
>  
>  #undef VERBOSE
> -#if 0
>  #define HW_RECT_ACCEL
> +#if 0
>  #define HW_FILL_ACCEL
>  #endif
>  #define HW_MOUSE_ACCEL
> @@ -406,7 +406,7 @@ static inline void vmsvga_update_rect_flush(struct vmsvga_state_s *s)
>  }
>  
>  #ifdef HW_RECT_ACCEL
> -static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
> +static inline int vmsvga_copy_rect(struct vmsvga_state_s *s,
>                  int x0, int y0, int x1, int y1, int w, int h)
>  {
>      DisplaySurface *surface = qemu_console_surface(s->vga.con);
> @@ -417,6 +417,13 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
>      int line = h;
>      uint8_t *ptr[2];
>  
> +    if (!vmsvga_verify_rect(surface, "vmsvga_copy_rect/src", x0, y0, w, h)) {
> +        return -1;
> +    }
> +    if (!vmsvga_verify_rect(surface, "vmsvga_copy_rect/dst", x1, y1, w, h)) {
> +        return -1;
> +    }
> +
>      if (y1 > y0) {
>          ptr[0] = vram + bypp * x0 + bypl * (y0 + h - 1);
>          ptr[1] = vram + bypp * x1 + bypl * (y1 + h - 1);
> @@ -432,6 +439,7 @@ static inline void vmsvga_copy_rect(struct vmsvga_state_s *s,
>      }
>  
>      vmsvga_update_rect_delayed(s, x1, y1, w, h);
> +    return 0;
>  }
>  #endif
>  
> @@ -625,12 +633,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>              width = vmsvga_fifo_read(s);
>              height = vmsvga_fifo_read(s);
>  #ifdef HW_RECT_ACCEL
> -            vmsvga_copy_rect(s, x, y, dx, dy, width, height);
> -            break;
> -#else
> +            if (vmsvga_copy_rect(s, x, y, dx, dy, width, height) == 0) {
> +                break;
> +            }
> +#endif
>              args = 0;
>              goto badcmd;
> -#endif
>  
>          case SVGA_CMD_DEFINE_CURSOR:
>              len -= 8;
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v3 5/5] vmware-vga: use vmsvga_verify_rect in vmsvga_fill_rect

[Don Koch]

On Tue, 28 Oct 2014 10:50:42 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Add verification to vmsvga_fill_rect, re-enable HW_FILL_ACCEL.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

Reviewed-by: Don Koch <dkoch@verizon.com>

>  hw/display/vmware_vga.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 73a1b52..1751f19 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -30,9 +30,7 @@
>  
>  #undef VERBOSE
>  #define HW_RECT_ACCEL
> -#if 0
>  #define HW_FILL_ACCEL
> -#endif
>  #define HW_MOUSE_ACCEL
>  
>  #include "vga_int.h"
> @@ -444,7 +442,7 @@ static inline int vmsvga_copy_rect(struct vmsvga_state_s *s,
>  #endif
>  
>  #ifdef HW_FILL_ACCEL
> -static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
> +static inline int vmsvga_fill_rect(struct vmsvga_state_s *s,
>                  uint32_t c, int x, int y, int w, int h)
>  {
>      DisplaySurface *surface = qemu_console_surface(s->vga.con);
> @@ -457,6 +455,10 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
>      uint8_t *src;
>      uint8_t col[4];
>  
> +    if (!vmsvga_verify_rect(surface, __func__, x, y, w, h)) {
> +        return -1;
> +    }
> +
>      col[0] = c;
>      col[1] = c >> 8;
>      col[2] = c >> 16;
> @@ -481,6 +483,7 @@ static inline void vmsvga_fill_rect(struct vmsvga_state_s *s,
>      }
>  
>      vmsvga_update_rect_delayed(s, x, y, w, h);
> +    return 0;
>  }
>  #endif
>  
> @@ -613,12 +616,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
>              width = vmsvga_fifo_read(s);
>              height = vmsvga_fifo_read(s);
>  #ifdef HW_FILL_ACCEL
> -            vmsvga_fill_rect(s, colour, x, y, width, height);
> -            break;
> -#else
> +            if (vmsvga_fill_rect(s, colour, x, y, width, height) == 0) {
> +                break;
> +            }
> +#endif
>              args = 0;
>              goto badcmd;
> -#endif
>  
>          case SVGA_CMD_RECT_COPY:
>              len -= 7;
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v3 0/5] vmware-vga: fix CVE-2014-3689

[Don Koch]

On Tue, 28 Oct 2014 10:50:37 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> vmware-vga emulation lacks sanity checks in the hardware acceleration
> (blit + fill) functions.  This patch series plugs the holes.
> 
> v3 changes:
>  * throw badcmd errors in case the rectangles fail the sanity checks.
> v2 changes:
>  * small whitespace fixup.
>  * do fullscreen update on invalid update requests.
> 
> cheers,
>   Gerd
> 
> Gerd Hoffmann (5):
>   vmware-vga: CVE-2014-3689: turn off hw accel
>   vmware-vga: add vmsvga_verify_rect
>   vmware-vga: use vmsvga_verify_rect in vmsvga_update_rect
>   vmware-vga: use vmsvga_verify_rect in vmsvga_copy_rect
>   vmware-vga: use vmsvga_verify_rect in vmsvga_fill_rect
> 
>  hw/display/vmware_vga.c | 116 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 78 insertions(+), 38 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Whole series looks good to me, now.

A bit of a big hammer, but much better than before. I had started working on
fixing the clipping a month ago. I might still do that based on this update
which is a better base than the original.

Thanks, Gerd.

-d