[Qemu-devel] [PATCH 0/4] vmsvga: security fixes

[Qemu-devel] [PATCH 0/4] vmsvga: security fixes

[Gerd Hoffmann]

  Hi,

Here comes a series for the vmware svga, fixing security issues in the
fifo handling:

  CVE-2016-4453 qemu: Infinite loop in vmsvga_fifo_run() function
  CVE-2016-4454 qemu: Out-of-bounds read in vmsvga_fifo_read_raw() function 

please review,
  Gerd

Gerd Hoffmann (4):
  vmsvga: move fifo sanity checks to vmsvga_fifo_length
  vmsvga: add more fifo checks
  vmsvga: shadow fifo registers
  vmsvga: don't process more than 1024 fifo commands at once

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

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] vmsvga: move fifo sanity checks to vmsvga_fifo_length

[Gerd Hoffmann]

Sanity checks are applied when the fifo is enabled by the guest
(SVGA_REG_CONFIG_DONE write).  Which doesn't help much if the guest
changes the fifo registers afterwards.  Move the checks to
vmsvga_fifo_length so they are done each time qemu is about to read
from the fifo.

Fixes: CVE-2016-4454
Cc: P J P <ppandit@redhat.com>
Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 0c63fa8..63a7c05 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -555,6 +555,21 @@ static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
     if (!s->config || !s->enable) {
         return 0;
     }
+
+    /* Check range and alignment.  */
+    if ((CMD(min) | CMD(max) | CMD(next_cmd) | CMD(stop)) & 3) {
+        return 0;
+    }
+    if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *) s->fifo) {
+        return 0;
+    }
+    if (CMD(max) > SVGA_FIFO_SIZE) {
+        return 0;
+    }
+    if (CMD(max) < CMD(min) + 10 * 1024) {
+        return 0;
+    }
+
     num = CMD(next_cmd) - CMD(stop);
     if (num < 0) {
         num += CMD(max) - CMD(min);
@@ -1005,19 +1020,6 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
     case SVGA_REG_CONFIG_DONE:
         if (value) {
             s->fifo = (uint32_t *) s->fifo_ptr;
-            /* Check range and alignment.  */
-            if ((CMD(min) | CMD(max) | CMD(next_cmd) | CMD(stop)) & 3) {
-                break;
-            }
-            if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *) s->fifo) {
-                break;
-            }
-            if (CMD(max) > SVGA_FIFO_SIZE) {
-                break;
-            }
-            if (CMD(max) < CMD(min) + 10 * 1024) {
-                break;
-            }
             vga_dirty_log_stop(&s->vga);
         }
         s->config = !!value;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] vmsvga: add more fifo checks

[Gerd Hoffmann]

Make sure all fifo ptrs are within range.

Fixes: CVE-2016-4454
Cc: P J P <ppandit@redhat.com>
Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 63a7c05..a26e62e 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -563,7 +563,10 @@ static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
     if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *) s->fifo) {
         return 0;
     }
-    if (CMD(max) > SVGA_FIFO_SIZE) {
+    if (CMD(max) > SVGA_FIFO_SIZE ||
+        CMD(min) >= SVGA_FIFO_SIZE ||
+        CMD(stop) >= SVGA_FIFO_SIZE ||
+        CMD(next_cmd) >= SVGA_FIFO_SIZE) {
         return 0;
     }
     if (CMD(max) < CMD(min) + 10 * 1024) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] vmsvga: shadow fifo registers

[Gerd Hoffmann]

The fifo is normal ram.  So kvm vcpu threads and qemu iothread can
access the fifo in parallel without syncronization.  Which in turn
implies we can't use the fifo pointers in-place because the guest
can try changing them underneath us.  So add shadows for them, to
make sure the guest can't modify them after we've applied sanity
checks.

Fixes: CVE-2016-4454
Cc: P J P <ppandit@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 57 ++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index a26e62e..de2567b 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -66,17 +66,11 @@ struct vmsvga_state_s {
     uint8_t *fifo_ptr;
     unsigned int fifo_size;
 
-    union {
-        uint32_t *fifo;
-        struct QEMU_PACKED {
-            uint32_t min;
-            uint32_t max;
-            uint32_t next_cmd;
-            uint32_t stop;
-            /* Add registers here when adding capabilities.  */
-            uint32_t fifo[0];
-        } *cmd;
-    };
+    uint32_t *fifo;
+    uint32_t fifo_min;
+    uint32_t fifo_max;
+    uint32_t fifo_next;
+    uint32_t fifo_stop;
 
 #define REDRAW_FIFO_LEN  512
     struct vmsvga_rect_s {
@@ -198,7 +192,7 @@ enum {
      */
     SVGA_FIFO_MIN = 0,
     SVGA_FIFO_MAX,      /* The distance from MIN to MAX must be at least 10K */
-    SVGA_FIFO_NEXT_CMD,
+    SVGA_FIFO_NEXT,
     SVGA_FIFO_STOP,
 
     /*
@@ -546,8 +540,6 @@ static inline void vmsvga_cursor_define(struct vmsvga_state_s *s,
 }
 #endif
 
-#define CMD(f)  le32_to_cpu(s->cmd->f)
-
 static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
 {
     int num;
@@ -556,38 +548,44 @@ static inline int vmsvga_fifo_length(struct vmsvga_state_s *s)
         return 0;
     }
 
+    s->fifo_min  = le32_to_cpu(s->fifo[SVGA_FIFO_MIN]);
+    s->fifo_max  = le32_to_cpu(s->fifo[SVGA_FIFO_MAX]);
+    s->fifo_next = le32_to_cpu(s->fifo[SVGA_FIFO_NEXT]);
+    s->fifo_stop = le32_to_cpu(s->fifo[SVGA_FIFO_STOP]);
+
     /* Check range and alignment.  */
-    if ((CMD(min) | CMD(max) | CMD(next_cmd) | CMD(stop)) & 3) {
+    if ((s->fifo_min | s->fifo_max | s->fifo_next | s->fifo_stop) & 3) {
         return 0;
     }
-    if (CMD(min) < (uint8_t *) s->cmd->fifo - (uint8_t *) s->fifo) {
+    if (s->fifo_min < sizeof(uint32_t) * 4) {
         return 0;
     }
-    if (CMD(max) > SVGA_FIFO_SIZE ||
-        CMD(min) >= SVGA_FIFO_SIZE ||
-        CMD(stop) >= SVGA_FIFO_SIZE ||
-        CMD(next_cmd) >= SVGA_FIFO_SIZE) {
+    if (s->fifo_max > SVGA_FIFO_SIZE ||
+        s->fifo_min >= SVGA_FIFO_SIZE ||
+        s->fifo_stop >= SVGA_FIFO_SIZE ||
+        s->fifo_next >= SVGA_FIFO_SIZE) {
         return 0;
     }
-    if (CMD(max) < CMD(min) + 10 * 1024) {
+    if (s->fifo_max < s->fifo_min + 10 * 1024) {
         return 0;
     }
 
-    num = CMD(next_cmd) - CMD(stop);
+    num = s->fifo_next - s->fifo_stop;
     if (num < 0) {
-        num += CMD(max) - CMD(min);
+        num += s->fifo_max - s->fifo_min;
     }
     return num >> 2;
 }
 
 static inline uint32_t vmsvga_fifo_read_raw(struct vmsvga_state_s *s)
 {
-    uint32_t cmd = s->fifo[CMD(stop) >> 2];
+    uint32_t cmd = s->fifo[s->fifo_stop >> 2];
 
-    s->cmd->stop = cpu_to_le32(CMD(stop) + 4);
-    if (CMD(stop) >= CMD(max)) {
-        s->cmd->stop = s->cmd->min;
+    s->fifo_stop += 4;
+    if (s->fifo_stop >= s->fifo_max) {
+        s->fifo_stop = s->fifo_min;
     }
+    s->fifo[SVGA_FIFO_STOP] = cpu_to_le32(s->fifo_stop);
     return cmd;
 }
 
@@ -607,7 +605,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
     len = vmsvga_fifo_length(s);
     while (len > 0) {
         /* May need to go back to the start of the command if incomplete */
-        cmd_start = s->cmd->stop;
+        cmd_start = s->fifo_stop;
 
         switch (cmd = vmsvga_fifo_read(s)) {
         case SVGA_CMD_UPDATE:
@@ -766,7 +764,8 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             break;
 
         rewind:
-            s->cmd->stop = cmd_start;
+            s->fifo_stop = cmd_start;
+            s->fifo[SVGA_FIFO_STOP] = cpu_to_le32(s->fifo_stop);
             break;
         }
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] vmsvga: don't process more than 1024 fifo commands at once

[Gerd Hoffmann]

vmsvga_fifo_run is called in regular intervals (on each display update)
and will resume where it left off.  So we can simply exit the loop,
without having to worry about how processing will continue.

Fixes: CVE-2016-4453
Cc: P J P <ppandit@redhat.com>
Reported-by: 李强 <liqiang6-s@360.cn>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index de2567b..e51a05e 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -597,13 +597,13 @@ static inline uint32_t vmsvga_fifo_read(struct vmsvga_state_s *s)
 static void vmsvga_fifo_run(struct vmsvga_state_s *s)
 {
     uint32_t cmd, colour;
-    int args, len;
+    int args, len, maxloop = 1024;
     int x, y, dx, dy, width, height;
     struct vmsvga_cursor_definition_s cursor;
     uint32_t cmd_start;
 
     len = vmsvga_fifo_length(s);
-    while (len > 0) {
+    while (len > 0 && --maxloop > 0) {
         /* May need to go back to the start of the command if incomplete */
         cmd_start = s->fifo_stop;
 
-- 
1.8.3.1