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

[Qemu-devel] [PATCH 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.

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 | 90 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 28 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 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>
---
 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 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>
---
 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..fc0a2a7 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

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

[Gonglei]

On 2014/10/14 15:45, Gerd Hoffmann wrote:

> 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>
> ---
>  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..fc0a2a7 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);

                                                                       ^
A superfluous space.

> +        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);


A superfluous space too.

> +        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;


Best regards,
-Gonglei

[Qemu-devel] [PATCH 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.
Invalid update requests will be ignored instead.

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

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index fc0a2a7..7564f88 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -356,36 +356,8 @@ 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;
-        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;
+    if (!vmsvga_verify_rect(surface, __func__, x, y, w, h)) {
+        return;
     }
 
     bypl = surface_stride(surface);
-- 
1.8.3.1

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

[BALATON Zoltan]

On Tue, 14 Oct 2014, Gerd Hoffmann wrote:
> Switch vmsvga_update_rect over to use vmsvga_verify_rect.  Slight change
> in behavior:  We don't try to automatically fixup rectangles any more.
> Invalid update requests will be ignored instead.

Are you sure this won't break clients? I remember that maybe Windows 
drivers did produce requests with partially off screen rectangles for 
objects that are partially visible. I don't recall if this was for windows 
dragged off screen or mouse pointer near the screen but there was a reason 
this fixup was added. Did you test this?

Regards,
BALATON Zoltan

>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/display/vmware_vga.c | 32 ++------------------------------
> 1 file changed, 2 insertions(+), 30 deletions(-)
>
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index fc0a2a7..7564f88 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -356,36 +356,8 @@ 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;
> -        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;
> +    if (!vmsvga_verify_rect(surface, __func__, x, y, w, h)) {
> +        return;
>     }
>
>     bypl = surface_stride(surface);
> -- 
> 1.8.3.1
>
>
>

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

[Gerd Hoffmann]

On Di, 2014-10-14 at 11:29 +0200, BALATON Zoltan wrote:
> On Tue, 14 Oct 2014, Gerd Hoffmann wrote:
> > Switch vmsvga_update_rect over to use vmsvga_verify_rect.  Slight change
> > in behavior:  We don't try to automatically fixup rectangles any more.
> > Invalid update requests will be ignored instead.
> 
> Are you sure this won't break clients? I remember that maybe Windows 
> drivers did produce requests with partially off screen rectangles for 
> objects that are partially visible. I don't recall if this was for windows 
> dragged off screen or mouse pointer near the screen but there was a reason 
> this fixup was added. Did you test this?

Not tested.  I don't have windows guests with vmware drivers.  The
fixups avoid qemu crashing for sure.  Possibly they are also needed to
prevent rendering problems.  Should that be the case I'd tend to simply
do a full-screen refresh as fallback should we see invalid rectangles
instead of keeping the fixup logic.  The fixups become quite complex for
the bitblit case, thats why I dropped them.

cheers,
  Gerd

[Qemu-devel] [PATCH 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 7564f88..d29470b 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
@@ -413,6 +413,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;
+    }
+    if (!vmsvga_verify_rect(surface, "vmsvga_copy_rect/dst", x1, y1, w, h)) {
+        return;
+    }
+
     if (y1 > y0) {
         ptr[0] = vram + bypp * x0 + bypl * (y0 + h - 1);
         ptr[1] = vram + bypp * x1 + bypl * (y1 + h - 1);
-- 
1.8.3.1

[Qemu-devel] [PATCH 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index d29470b..a52346c 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"
@@ -452,6 +450,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;
+    }
+
     col[0] = c;
     col[1] = c >> 8;
     col[2] = c >> 16;
-- 
1.8.3.1