[Spice-commits] server/red_worker.c

[Frediano Ziglio]

 server/red_worker.c |   44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

New commits:
commit bd6ea0db84949ac903c27708166604de892f4671
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Jun 9 08:50:46 2015 +0100

    Avoid race conditions reading monitor configs from guest
    
    For security reasons do not assume guest do not change structures it
    pass to Qemu.
    Guest could change count field while Qemu is copying QXLMonitorsConfig
    structure leading to heap corruption.
    This patch avoid it reading count only once.
    
    This patch solves CVE-2015-3247.
    
    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 2f2d5a9..e2feb23 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -11222,19 +11222,18 @@ static inline void red_monitors_config_item_add(DisplayChannelClient *dcc)
 
 static void worker_update_monitors_config(RedWorker *worker,
                                           QXLMonitorsConfig *dev_monitors_config,
-                                          unsigned int max_monitors)
+                                          uint16_t count, uint16_t max_allowed)
 {
     int heads_size;
     MonitorsConfig *monitors_config;
     int i;
-    unsigned int count = MIN(dev_monitors_config->count, max_monitors);
 
     monitors_config_decref(worker->monitors_config);
 
     spice_debug("monitors config %d(%d)",
-                dev_monitors_config->count,
-                dev_monitors_config->max_allowed);
-    for (i = 0; i < dev_monitors_config->count; i++) {
+                count,
+                max_allowed);
+    for (i = 0; i < count; i++) {
         spice_debug("+%d+%d %dx%d",
                     dev_monitors_config->heads[i].x,
                     dev_monitors_config->heads[i].y,
@@ -11247,7 +11246,7 @@ static void worker_update_monitors_config(RedWorker *worker,
     monitors_config->refs = 1;
     monitors_config->worker = worker;
     monitors_config->count = count;
-    monitors_config->max_allowed = MIN(dev_monitors_config->max_allowed, max_monitors);
+    monitors_config->max_allowed = max_allowed;
     memcpy(monitors_config->heads, dev_monitors_config->heads, heads_size);
 }
 
@@ -11636,33 +11635,52 @@ void handle_dev_display_migrate(void *opaque, void *payload)
     red_migrate_display(worker, rcc);
 }
 
+static inline uint32_t qxl_monitors_config_size(uint32_t heads)
+{
+    return sizeof(QXLMonitorsConfig) + sizeof(QXLHead) * heads;
+}
+
 static void handle_dev_monitors_config_async(void *opaque, void *payload)
 {
     RedWorkerMessageMonitorsConfigAsync *msg = payload;
     RedWorker *worker = opaque;
-    int min_size = sizeof(QXLMonitorsConfig) + sizeof(QXLHead);
     int error;
+    uint16_t count, max_allowed;
     QXLMonitorsConfig *dev_monitors_config =
         (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
-                                     min_size, msg->group_id, &error);
+                                     qxl_monitors_config_size(1),
+                                     msg->group_id, &error);
 
     if (error) {
         /* TODO: raise guest bug (requires added QXL interface) */
         return;
     }
     worker->driver_cap_monitors_config = 1;
-    if (dev_monitors_config->count == 0) {
+    count = dev_monitors_config->count;
+    max_allowed = dev_monitors_config->max_allowed;
+    if (count == 0) {
         spice_warning("ignoring an empty monitors config message from driver");
         return;
     }
-    if (dev_monitors_config->count > dev_monitors_config->max_allowed) {
+    if (count > max_allowed) {
         spice_warning("ignoring malformed monitors_config from driver, "
                       "count > max_allowed %d > %d",
-                      dev_monitors_config->count,
-                      dev_monitors_config->max_allowed);
+                      count,
+                      max_allowed);
+        return;
+    }
+    /* get pointer again to check virtual size */
+    dev_monitors_config =
+        (QXLMonitorsConfig*)get_virt(&worker->mem_slots, msg->monitors_config,
+                                     qxl_monitors_config_size(count),
+                                     msg->group_id, &error);
+    if (error) {
+        /* TODO: raise guest bug (requires added QXL interface) */
         return;
     }
-    worker_update_monitors_config(worker, dev_monitors_config, msg->max_monitors);
+    worker_update_monitors_config(worker, dev_monitors_config,
+                                  MIN(count, msg->max_monitors),
+                                  MIN(max_allowed, msg->max_monitors));
     red_worker_push_monitors_config(worker);
 }