From 1b15983415665a7687f080a7dddf5159edbb7d91 Mon Sep 17 00:00:00 2001
From: Frediano Ziglio <fziglio@redhat.com>
Date: Mon, 26 Sep 2016 09:05:28 +0100
Subject: [PATCH] Make QXLMessage handling safe

The QXLMessage has no size so potentially a guest could give an
address that cause the string to overflow out of the video memory.
The current solution is to parse the message, release the resources
associated without printing the message from the client.
This also considering that the QXLMessage usage was deprecated
a while ago (I don't know exactly when).
This patches limit the string to 100000 characters (guest can feed
so much logs in other way) and limit to video memory.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 server/memslot.c       | 14 ++++++++++++++
 server/memslot.h       |  3 +++
 server/red-parse-qxl.c | 11 +++++++++++
 server/red-parse-qxl.h |  1 +
 server/red-worker.c    |  3 +--
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/server/memslot.c b/server/memslot.c
index 99c63e42..75cb75f9 100644
--- a/server/memslot.c
+++ b/server/memslot.c
@@ -71,6 +71,20 @@ int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
     return 1;
 }
 
+unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
+                                    unsigned long virt, int slot_id,
+                                    uint32_t group_id)
+{
+    MemSlot *slot;
+
+    slot = &info->mem_slots[group_id][slot_id];
+
+    if (virt < slot->virt_start_addr || virt > slot->virt_end_addr) {
+        return 0;
+    }
+    return slot->virt_end_addr - virt;
+}
+
 /*
  * return virtual address if successful, which may be 0.
  * returns 0 and sets error to 1 if an error condition occurs.
diff --git a/server/memslot.h b/server/memslot.h
index a29837c3..5046d0fa 100644
--- a/server/memslot.h
+++ b/server/memslot.h
@@ -55,6 +55,9 @@ static inline int memslot_get_generation(RedMemSlotInfo *info, uint64_t addr)
 
 int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
                           uint32_t add_size, uint32_t group_id);
+unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
+                                    unsigned long virt, int slot_id,
+                                    uint32_t group_id);
 unsigned long memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
                        int group_id, int *error);
 
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index aa911f31..11da3a18 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1293,6 +1293,9 @@ int red_get_message(RedMemSlotInfo *slots, int group_id,
 {
     QXLMessage *qxl;
     int error;
+    int memslot_id;
+    unsigned long len;
+    uint8_t *end;
 
     /*
      * security alert:
@@ -1307,6 +1310,14 @@ int red_get_message(RedMemSlotInfo *slots, int group_id,
     red->release_info_ext.info      = &qxl->release_info;
     red->release_info_ext.group_id  = group_id;
     red->data                       = qxl->data;
+    memslot_id = memslot_get_id(slots, addr+sizeof(*qxl));
+    len = memslot_max_size_virt(slots, ((intptr_t) qxl)+sizeof(*qxl), memslot_id, group_id);
+    len = MIN(len, 100000);
+    end = (uint8_t *)memchr(qxl->data, 0, len);
+    if (end == NULL) {
+        return 1;
+    }
+    red->len = end - qxl->data;
     return 0;
 }
 
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index d5e7b6bf..86a2d933 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -76,6 +76,7 @@ typedef struct RedUpdateCmd {
 
 typedef struct RedMessage {
     QXLReleaseInfoExt release_info_ext;
+    int len;
     uint8_t *data;
 } RedMessage;
 
diff --git a/server/red-worker.c b/server/red-worker.c
index 43836468..60c1ae9a 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -234,8 +234,7 @@ static int red_process_display(RedWorker *worker, int *ring_is_empty)
                 break;
             }
 #ifdef DEBUG
-            /* alert: accessing message.data is insecure */
-            spice_warning("MESSAGE: %s", message.data);
+            spice_warning("MESSAGE: %.*s", message.len, message.data);
 #endif
             red_qxl_release_resource(worker->qxl, message.release_info_ext);
             red_put_message(&message);