From bea51d967731a2be7741fcbeef799b9fd925343a Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@redhat.com>
Date: Wed, 9 Sep 2015 12:42:09 +0100
Subject: [PATCH 1/2] worker: validate correctly surfaces

Do not just give warning and continue to use an invalid index into
an array.

Resolves: CVE-2015-5260

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
---
 server/red_worker.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index 93e3398..c62dbcb 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1054,6 +1054,7 @@ typedef struct BitmapData {
     SpiceRect lossy_rect;
 } BitmapData;
 
+static inline int validate_surface(RedWorker *worker, uint32_t surface_id);
 static void red_draw_qxl_drawable(RedWorker *worker, Drawable *drawable);
 static void red_current_flush(RedWorker *worker, int surface_id);
 #ifdef DRAW_ALL
@@ -1267,11 +1268,6 @@ static inline int is_primary_surface(RedWorker *worker, uint32_t surface_id)
     return FALSE;
 }
 
-static inline void __validate_surface(RedWorker *worker, uint32_t surface_id)
-{
-    spice_warn_if(surface_id >= worker->n_surfaces);
-}
-
 static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
 {
         DrawContext *context;
@@ -1280,7 +1276,7 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
         /* surface_id must be validated before calling into
          * validate_drawable_bbox
          */
-        __validate_surface(worker, surface_id);
+        VALIDATE_SURFACE_RETVAL(worker, surface_id, FALSE);
         context = &worker->surfaces[surface_id].context;
 
         if (drawable->bbox.top < 0)
@@ -1301,7 +1297,10 @@ static int validate_drawable_bbox(RedWorker *worker, RedDrawable *drawable)
 
 static inline int validate_surface(RedWorker *worker, uint32_t surface_id)
 {
-    spice_warn_if(surface_id >= worker->n_surfaces);
+    if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
+        spice_warning("invalid surface_id %u", surface_id);
+        return 0;
+    }
     if (!worker->surfaces[surface_id].context.canvas) {
         spice_warning("canvas address is %p for %d (and is NULL)\n",
                    &(worker->surfaces[surface_id].context.canvas), surface_id);
@@ -4304,12 +4303,14 @@ static inline void red_create_surface(RedWorker *worker, uint32_t surface_id,uin
 static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
                                        uint32_t group_id, int loadvm)
 {
-    int surface_id;
+    uint32_t surface_id;
     RedSurface *red_surface;
     uint8_t *data;
 
     surface_id = surface->surface_id;
-    __validate_surface(worker, surface_id);
+    if SPICE_UNLIKELY(surface_id >= worker->n_surfaces) {
+        goto exit;
+    }
 
     red_surface = &worker->surfaces[surface_id];
 
@@ -4345,6 +4346,7 @@ static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface
     default:
             spice_error("unknown surface command");
     };
+exit:
     red_put_surface_cmd(surface);
     free(surface);
 }
@@ -11091,7 +11093,7 @@ void handle_dev_update(void *opaque, void *payload)
 {
     RedWorker *worker = opaque;
     RedWorkerMessageUpdate *msg = payload;
-    SpiceRect *rect = spice_new0(SpiceRect, 1);
+    SpiceRect *rect;
     RedSurface *surface;
     uint32_t surface_id = msg->surface_id;
     const QXLRect *qxl_area = msg->qxl_area;
@@ -11099,17 +11101,16 @@ void handle_dev_update(void *opaque, void *payload)
     QXLRect *qxl_dirty_rects = msg->qxl_dirty_rects;
     uint32_t clear_dirty_region = msg->clear_dirty_region;
 
+    VALIDATE_SURFACE_RET(worker, surface_id);
+
+    rect = spice_new0(SpiceRect, 1);
     surface = &worker->surfaces[surface_id];
     red_get_rect_ptr(rect, qxl_area);
     flush_display_commands(worker);
 
     spice_assert(worker->running);
 
-    if (validate_surface(worker, surface_id)) {
-        red_update_area(worker, rect, surface_id);
-    } else {
-        rendering_incorrect(__func__);
-    }
+    red_update_area(worker, rect, surface_id);
     free(rect);
 
     surface_dirty_region_to_rects(surface, qxl_dirty_rects, num_dirty_rects,
@@ -11148,6 +11149,7 @@ void handle_dev_del_memslot(void *opaque, void *payload)
  * surface_id == 0, maybe move the assert upward and merge the two functions? */
 static inline void destroy_surface_wait(RedWorker *worker, int surface_id)
 {
+    VALIDATE_SURFACE_RET(worker, surface_id);
     if (!worker->surfaces[surface_id].context.canvas) {
         return;
     }
@@ -11412,6 +11414,7 @@ void handle_dev_create_primary_surface(void *opaque, void *payload)
 
 static void dev_destroy_primary_surface(RedWorker *worker, uint32_t surface_id)
 {
+    VALIDATE_SURFACE_RET(worker, surface_id);
     spice_warn_if(surface_id != 0);
 
     spice_debug(NULL);
-- 
2.4.3