[Spice-devel] [PATCH 14/19] Prevent DoS from guest trying to allocate too much data on host for chunks

[Frediano Ziglio]

Limit number of chunks to a given amount to avoid guest trying to
allocate too much memory. Using circular or nested chunks lists
guest could try to allocate huge amounts of memory.
Considering the list can be infinite and guest can change data this
also prevents strange security attacks from guest.

Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
---
 server/red_parse_qxl.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index f425869..5513e82 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -37,6 +37,13 @@
 
 G_STATIC_ASSERT(MAX_DATA_CHUNK <= G_MAXINT32);
 
+/* Limit number of chunks.
+ * The guest can attempt to make host allocate too much memory
+ * just with a large number of small chunks.
+ * Prevent that the chunk list take more memory than the data itself.
+ */
+#define MAX_CHUNKS (MAX_DATA_CHUNK/1024u)
+
 #if 0
 static void hexdump_qxl(RedMemSlotInfo *slots, int group_id,
                         QXLPHYSICAL addr, uint8_t bytes)
@@ -100,9 +107,11 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
                                       RedDataChunk *red, QXLDataChunk *qxl)
 {
     RedDataChunk *red_prev;
-    size_t data_size = 0;
+    uint64_t data_size = 0;
+    uint32_t chunk_data_size;
     int error;
     QXLPHYSICAL next_chunk;
+    unsigned num_chunks = 0;
 
     red->data_size = qxl->data_size;
     data_size += red->data_size;
@@ -114,19 +123,43 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
     }
 
     while ((next_chunk = qxl->next_chunk) != 0) {
+        /* somebody is trying to use too much memory using a lot of chunks.
+         * Or made a circular list of chunks
+         */
+        if (++num_chunks >= MAX_CHUNKS) {
+            spice_warning("data split in too many chunks, avoiding DoS\n");
+            goto error;
+        }
+
+        memslot_id = get_memslot_id(slots, next_chunk);
+        qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl),
+                                       group_id, &error);
+        if (error)
+            goto error;
+
+        /* do not waste space for empty chunks.
+         * This could be just a driver issue or an attempt
+         * to allocate too much memory or a circular list.
+         * All above cases are handled by the check for number
+         * of chunks.
+         */
+        chunk_data_size = qxl->data_size;
+        if (chunk_data_size == 0)
+            continue;
+
         red_prev = red;
         red = spice_new0(RedDataChunk, 1);
+        red->data_size = chunk_data_size;
         red->prev_chunk = red_prev;
+        red->data = qxl->data;
         red_prev->next_chunk = red;
 
-        memslot_id = get_memslot_id(slots, next_chunk);
-        qxl = (QXLDataChunk *)get_virt(slots, next_chunk, sizeof(*qxl), group_id,
-                                      &error);
-        if (error)
+        data_size += chunk_data_size;
+        /* this can happen if client is sending nested chunks */
+        if (data_size > MAX_DATA_CHUNK) {
+            spice_warning("too much data inside chunks, avoiding DoS\n");
             goto error;
-        red->data_size = qxl->data_size;
-        data_size += red->data_size;
-        red->data = qxl->data;
+        }
         if (!validate_virt(slots, (intptr_t)red->data, memslot_id, red->data_size, group_id))
             goto error;
     }
-- 
2.4.3