[Spice-commits] 2 commits - server/red-parse-qxl.c server/red-parse-qxl.h server/red-worker.c

[Frediano Ziglio]

 server/red-parse-qxl.c |   49 ++++++++++++++++++++++++++++++++-----------------
 server/red-parse-qxl.h |    3 +++
 server/red-worker.c    |   11 +++++++++--
 3 files changed, 44 insertions(+), 19 deletions(-)

New commits:
commit 69628ea1375282cb7ca5b4dc4410e7aa67e0fc02
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed Mar 2 12:35:41 2016 +0000

    improve primary surface parameter checks
    
    Primary surface, as additional surfaces, can be used to access
    host memory from the guest using invalid parameters.
    
    The removed warning is not enough to prevent all cases. Also a warning
    is not enough to stop an escalation to happen.
    The red_validate_surface do different checks to make sure surface
    request is valid and not cause possible buffer/integer overflows:
    - format is valid;
    - width is not large to cause overflow compared to stride;
    - stride is not -2^31 (a number which negate is still <0);
    - stride * height does not overflow.
    
    This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1312980.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/red-worker.c b/server/red-worker.c
index e754bd2..121a2e5 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -657,8 +657,15 @@ static void dev_create_primary_surface(RedWorker *worker, uint32_t surface_id,
     spice_debug(NULL);
     spice_warn_if_fail(surface_id == 0);
     spice_warn_if_fail(surface.height != 0);
-    spice_warn_if_fail(((uint64_t)abs(surface.stride) * (uint64_t)surface.height) ==
-             abs(surface.stride) * surface.height);
+
+    /* surface can arrive from guest unchecked so make sure
+     * guest is not a malicious one and drop invalid requests
+     */
+    if (!red_validate_surface(surface.width, surface.height,
+                              surface.stride, surface.format)) {
+        spice_warning("wrong primary surface creation request");
+        return;
+    }
 
     line_0 = (uint8_t*)memslot_get_virt(&worker->mem_slots, surface.mem,
                                         surface.height * abs(surface.stride),
commit 790d8f3e53d324f496fc719498422e433aae8654
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Feb 29 14:24:03 2016 +0000

    factor out red_validate_surface function to validate surface parameters
    
    Make possible to reuse it outside red-parse-qxl.c.
    
    Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 721c861..d75e27e 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1339,13 +1339,41 @@ static unsigned int surface_format_to_bpp(uint32_t format)
     return 0;
 }
 
+bool red_validate_surface(uint32_t width, uint32_t height,
+                          int32_t stride, uint32_t format)
+{
+    unsigned int bpp;
+    uint64_t size;
+
+    bpp = surface_format_to_bpp(format);
+
+    /* check if format is valid */
+    if (!bpp) {
+        return false;
+    }
+
+    /* check stride is larger than required bytes */
+    size = ((uint64_t) width * bpp + 7u) / 8u;
+    /* the uint32_t conversion is here to avoid problems with -2^31 value */
+    if (stride == G_MININT32 || size > (uint32_t) abs(stride)) {
+        return false;
+    }
+
+    /* the multiplication can overflow, also abs(-2^31) may return a negative value */
+    size = (uint64_t) height * abs(stride);
+    if (size > MAX_DATA_CHUNK) {
+        return false;
+    }
+
+    return true;
+}
+
 int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
                         RedSurfaceCmd *red, QXLPHYSICAL addr)
 {
     QXLSurfaceCmd *qxl;
     uint64_t size;
     int error;
-    unsigned int bpp;
 
     qxl = (QXLSurfaceCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id,
                                             &error);
@@ -1365,26 +1393,13 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
         red->u.surface_create.width  = qxl->u.surface_create.width;
         red->u.surface_create.height = qxl->u.surface_create.height;
         red->u.surface_create.stride = qxl->u.surface_create.stride;
-        bpp = surface_format_to_bpp(red->u.surface_create.format);
 
-        /* check if format is valid */
-        if (!bpp) {
+        if (!red_validate_surface(red->u.surface_create.width, red->u.surface_create.height,
+                                  red->u.surface_create.stride, red->u.surface_create.format)) {
             return 1;
         }
 
-        /* check stride is larger than required bytes */
-        size = ((uint64_t) red->u.surface_create.width * bpp + 7u) / 8u;
-        /* the uint32_t conversion is here to avoid problems with -2^31 value */
-        if (red->u.surface_create.stride == G_MININT32
-            || size > (uint32_t) abs(red->u.surface_create.stride)) {
-            return 1;
-        }
-
-        /* the multiplication can overflow, also abs(-2^31) may return a negative value */
-        size = (uint64_t) red->u.surface_create.height * abs(red->u.surface_create.stride);
-        if (size > MAX_DATA_CHUNK) {
-            return 1;
-        }
+        size = red->u.surface_create.height * abs(red->u.surface_create.stride);
         red->u.surface_create.data =
             (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data, size, group_id, &error);
         if (error) {
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index c92fc84..0da20ad 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -127,6 +127,9 @@ int red_get_message(RedMemSlotInfo *slots, int group_id,
                     RedMessage *red, QXLPHYSICAL addr);
 void red_put_message(RedMessage *red);
 
+bool red_validate_surface(uint32_t width, uint32_t height,
+                          int32_t stride, uint32_t format);
+
 int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
                         RedSurfaceCmd *red, QXLPHYSICAL addr);
 void red_put_surface_cmd(RedSurfaceCmd *red);