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

[Frediano Ziglio]

 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(-)

New commits:
commit 1b15983415665a7687f080a7dddf5159edbb7d91
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Sep 26 09:05:28 2016 +0100

    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 at redhat.com>
    Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

diff --git a/server/memslot.c b/server/memslot.c
index 99c63e4..75cb75f 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 a29837c..5046d0f 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 aa911f3..11da3a1 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 d5e7b6b..86a2d93 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 4383646..60c1ae9 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);