[Qemu-devel] [PULL for-2.2 0/2] cirrus: fix blit region check (cve-2014-8106)

[Qemu-devel] [PULL for-2.2 0/2] cirrus: fix blit region check (cve-2014-8106)

[Gerd Hoffmann]

  Hi,

Last minute pull req for 2.2, carrying a security
fix for cirrus bitblit ops.

please pull,
  Gerd

The following changes since commit db12451decf7dfe0f083564183e135f2095228b9:

  Fix for crash after migration in virtio-rng on bi-endian targets (2014-11-28 13:06:00 +0000)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/pull-cve-2014-8106-20141204-1

for you to fetch changes up to bf25983345ca44aec3dd92c57142be45452bd38a:

  cirrus: don't overflow CirrusVGAState->cirrus_bltbuf (2014-12-01 10:25:46 +0100)

----------------------------------------------------------------
cirrus: fix blit region check

----------------------------------------------------------------
Gerd Hoffmann (2):
      cirrus: fix blit region check
      cirrus: don't overflow CirrusVGAState->cirrus_bltbuf

 hw/display/cirrus_vga.c | 65 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 17 deletions(-)

[Qemu-devel] [PULL 1/2] cirrus: fix blit region check

[Gerd Hoffmann]

Issues:
 * Doesn't check pitches correctly in case it is negative.
 * Doesn't check width at all.

Turn macro into functions while being at it, also factor out the check
for one region which we then can simply call twice for src + dst.

This is CVE-2014-8106.

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/display/cirrus_vga.c | 61 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 8a5b76c..d54fb06 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -173,20 +173,6 @@
 
 #define CIRRUS_PNPMMIO_SIZE         0x1000
 
-#define BLTUNSAFE(s) \
-    ( \
-        ( /* check dst is within bounds */ \
-            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_dstpitch) \
-                + ((s)->cirrus_blt_dstaddr & (s)->cirrus_addr_mask) > \
-                    (s)->vga.vram_size \
-        ) || \
-        ( /* check src is within bounds */ \
-            (s)->cirrus_blt_height * ABS((s)->cirrus_blt_srcpitch) \
-                + ((s)->cirrus_blt_srcaddr & (s)->cirrus_addr_mask) > \
-                    (s)->vga.vram_size \
-        ) \
-    )
-
 struct CirrusVGAState;
 typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
                                      uint8_t * dst, const uint8_t * src,
@@ -279,6 +265,46 @@ static void cirrus_update_memory_access(CirrusVGAState *s);
  *
  ***************************************/
 
+static bool blit_region_is_unsafe(struct CirrusVGAState *s,
+                                  int32_t pitch, int32_t addr)
+{
+    if (pitch < 0) {
+        int64_t min = addr
+            + ((int64_t)s->cirrus_blt_height-1) * pitch;
+        int32_t max = addr
+            + s->cirrus_blt_width;
+        if (min < 0 || max >= s->vga.vram_size) {
+            return true;
+        }
+    } else {
+        int64_t max = addr
+            + ((int64_t)s->cirrus_blt_height-1) * pitch
+            + s->cirrus_blt_width;
+        if (max >= s->vga.vram_size) {
+            return true;
+        }
+    }
+    return false;
+}
+
+static bool blit_is_unsafe(struct CirrusVGAState *s)
+{
+    /* should be the case, see cirrus_bitblt_start */
+    assert(s->cirrus_blt_width > 0);
+    assert(s->cirrus_blt_height > 0);
+
+    if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
+                              s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
+        return true;
+    }
+    if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
+                              s->cirrus_blt_srcaddr & s->cirrus_addr_mask)) {
+        return true;
+    }
+
+    return false;
+}
+
 static void cirrus_bitblt_rop_nop(CirrusVGAState *s,
                                   uint8_t *dst,const uint8_t *src,
                                   int dstpitch,int srcpitch,
@@ -636,7 +662,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState * s,
 
     dst = s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask);
 
-    if (BLTUNSAFE(s))
+    if (blit_is_unsafe(s))
         return 0;
 
     (*s->cirrus_rop) (s, dst, src,
@@ -654,8 +680,9 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int blt_rop)
 {
     cirrus_fill_t rop_func;
 
-    if (BLTUNSAFE(s))
+    if (blit_is_unsafe(s)) {
         return 0;
+    }
     rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 1];
     rop_func(s, s->vga.vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask),
              s->cirrus_blt_dstpitch,
@@ -752,7 +779,7 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
 {
-    if (BLTUNSAFE(s))
+    if (blit_is_unsafe(s))
         return 0;
 
     cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->vga.start_addr,
-- 
1.8.3.1

[Qemu-devel] [PULL 2/2] cirrus: don't overflow CirrusVGAState->cirrus_bltbuf

[Gerd Hoffmann]

This is CVE-2014-8106.

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

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index d54fb06..2725264 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -293,6 +293,10 @@ static bool blit_is_unsafe(struct CirrusVGAState *s)
     assert(s->cirrus_blt_width > 0);
     assert(s->cirrus_blt_height > 0);
 
+    if (s->cirrus_blt_width > CIRRUS_BLTBUFSIZE) {
+        return true;
+    }
+
     if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
                               s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
         return true;
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.2 0/2] cirrus: fix blit region check (cve-2014-8106)

[Peter Maydell]

On 4 December 2014 at 12:13, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Last minute pull req for 2.2, carrying a security
> fix for cirrus bitblit ops.

Applied, thanks. We'll need to do an rc5 now; is there
anything else in the pipeline?

thanks
-- PMM