server: allow failure in getvirt

This patch changed getvirt to continue working even if spice_critical
doesn't abort (i.e. SPICE_ABORT_LEVEL != -1). This is in preparation to
make getvirt not abort at all. The reason is that getvirt is run on
guest provided memory, so a bad driver can crash the vm.

5 files changed, 240 insertions, 89 deletions

diff --git a/server/red_memslots.c b/server/red_memslots.c
index ae2c6e46..523890d6 100644
--- a/server/red_memslots.c
+++ b/server/red_memslots.c
@@ -73,14 +73,16 @@ unsigned long get_virt_delta(RedMemSlotInfo *info, QXLPHYSICAL addr, int group_i
     return (slot->address_delta - (addr - __get_clean_virt(info, addr)));
 }
 
-void validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
-                   uint32_t add_size, uint32_t group_id)
+/* return 1 if validation successfull, 0 otherwise */
+int validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
+                  uint32_t add_size, uint32_t group_id)
 {
     MemSlot *slot;
 
     slot = &info->mem_slots[group_id][slot_id];
     if ((virt + add_size) < virt) {
         spice_critical("virtual address overlap");
+        return 0;
     }
 
     if (virt < slot->virt_start_addr || (virt + add_size) > slot->virt_end_addr) {
@@ -90,11 +92,17 @@ void validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
               "    slot=0x%lx-0x%lx delta=0x%lx",
               virt, add_size, slot_id, group_id,
               slot->virt_start_addr, slot->virt_end_addr, slot->address_delta);
+        return 0;
     }
+    return 1;
 }
 
+/*
+ * return virtual address if successful, which may be 0.
+ * returns 0 and sets error to 1 if an error condition occurs.
+ */
 unsigned long get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
-                       int group_id)
+                       int group_id, int *error)
 {
     int slot_id;
     int generation;
@@ -102,14 +110,19 @@ unsigned long get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
 
     MemSlot *slot;
 
+    *error = 0;
     if (group_id > info->num_memslots_groups) {
         spice_critical("group_id too big");
+        *error = 1;
+        return 0;
     }
 
     slot_id = get_memslot_id(info, addr);
     if (slot_id > info->num_memslots) {
         print_memslots(info);
         spice_critical("slot_id too big, addr=%" PRIx64, addr);
+        *error = 1;
+        return 0;
     }
 
     slot = &info->mem_slots[group_id][slot_id];
@@ -119,25 +132,38 @@ unsigned long get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
         print_memslots(info);
         spice_critical("address generation is not valid, group_id %d, slot_id %d, gen %d, slot_gen %d\n",
               group_id, slot_id, generation, slot->generation);
+        *error = 1;
+        return 0;
     }
 
     h_virt = __get_clean_virt(info, addr);
     h_virt += slot->address_delta;
 
-    validate_virt(info, h_virt, slot_id, add_size, group_id);
+    if (!validate_virt(info, h_virt, slot_id, add_size, group_id)) {
+        *error = 1;
+        return 0;
+    }
 
     return h_virt;
 }
 
-void *validate_chunk (RedMemSlotInfo *info, QXLPHYSICAL data, uint32_t group_id, uint32_t *data_size_out, QXLPHYSICAL *next_out)
+void *validate_chunk(RedMemSlotInfo *info, QXLPHYSICAL data, uint32_t group_id,
+                     uint32_t *data_size_out, QXLPHYSICAL *next_out, int *error)
 {
     QXLDataChunk *chunk;
     uint32_t data_size;
 
-    chunk = (QXLDataChunk *)get_virt(info, data, sizeof(QXLDataChunk), group_id);
+    chunk = (QXLDataChunk *)get_virt(info, data, sizeof(QXLDataChunk), group_id,
+                                     error);
+    if (*error) {
+        return NULL;
+    }
     data_size = chunk->data_size;
-    validate_virt(info, (unsigned long)chunk->data, get_memslot_id(info, data),
-                  data_size, group_id);
+    if (!validate_virt(info, (unsigned long)chunk->data, get_memslot_id(info, data),
+                       data_size, group_id)) {
+        *error = 1;
+        return NULL;
+    }
     *next_out = chunk->next_chunk;
     *data_size_out = data_size;
 
diff --git a/server/red_memslots.h b/server/red_memslots.h
index d50587f6..c4303bd0 100644
--- a/server/red_memslots.h
+++ b/server/red_memslots.h
@@ -54,12 +54,13 @@ static inline int get_generation(RedMemSlotInfo *info, uint64_t addr)
 }
 
 unsigned long get_virt_delta(RedMemSlotInfo *info, QXLPHYSICAL addr, int group_id);
-void validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
-                   uint32_t add_size, uint32_t group_id);
+int validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
+                  uint32_t add_size, uint32_t group_id);
 unsigned long get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
-                       int group_id);
+                       int group_id, int *error);
 
-void *validate_chunk (RedMemSlotInfo *info, QXLPHYSICAL data, uint32_t group_id, uint32_t *data_size_out, QXLPHYSICAL *next_out);
+void *validate_chunk(RedMemSlotInfo *info, QXLPHYSICAL data, uint32_t group_id,
+                     uint32_t *data_size_out, QXLPHYSICAL *next_out, int *error);
 void red_memslot_info_init(RedMemSlotInfo *info,
                            uint32_t num_groups, uint32_t num_slots,
                            uint8_t generation_bits,
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 811e427f..3abf6389 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -89,10 +89,13 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
 {
     RedDataChunk *red_prev;
     size_t data_size = 0;
+    int error;
 
     red->data_size = qxl->data_size;
     data_size += red->data_size;
-    validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id);
+    if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
+        return 0;
+    }
     red->data = qxl->data;
     red->prev_chunk = NULL;
 
@@ -100,11 +103,16 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo *slots, int group_id,
         red_prev = red;
         red = spice_new(RedDataChunk, 1);
         memslot_id = get_memslot_id(slots, qxl->next_chunk);
-        qxl = (QXLDataChunk*)get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id);
-
+        qxl = (QXLDataChunk*)get_virt(slots, qxl->next_chunk, sizeof(*qxl), group_id,
+                                      &error);
+        if (error) {
+            return 0;
+        }
         red->data_size = qxl->data_size;
         data_size += red->data_size;
-        validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id);
+        if (!validate_virt(slots, (intptr_t)qxl->data, memslot_id, red->data_size, group_id)) {
+            return 0;
+        }
         red->data = qxl->data;
         red->prev_chunk = red_prev;
         red_prev->next_chunk = red;
@@ -118,9 +126,13 @@ static size_t red_get_data_chunks(RedMemSlotInfo *slots, int group_id,
                                   RedDataChunk *red, QXLPHYSICAL addr)
 {
     QXLDataChunk *qxl;
+    int error;
     int memslot_id = get_memslot_id(slots, addr);
 
-    qxl = (QXLDataChunk*)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLDataChunk*)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return 0;
+    }
     return red_get_data_chunks_ptr(slots, group_id, memslot_id, red, qxl);
 }
 
@@ -170,8 +182,12 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
     int n_segments;
     int i;
     uint32_t count;
+    int error;
 
-    qxl = (QXLPath *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLPath *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return NULL;
+    }
     size = red_get_data_chunks_ptr(slots, group_id,
                                    get_memslot_id(slots, addr),
                                    &chunks, &qxl->chunk);
@@ -226,6 +242,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id,
     }
     /* Ensure guest didn't tamper with segment count */
     spice_assert(n_segments == red->num_segments);
+    return NULL;
 
     if (free_data) {
         free(data);
@@ -244,8 +261,12 @@ static SpiceClipRects *red_get_clip_rects(RedMemSlotInfo *slots, int group_id,
     bool free_data;
     size_t size;
     int i;
+    int error;
 
-    qxl = (QXLClipRects *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLClipRects *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return NULL;
+    }
     size = red_get_data_chunks_ptr(slots, group_id,
                                    get_memslot_id(slots, addr),
                                    &chunks, &qxl->chunk);
@@ -271,10 +292,14 @@ static SpiceChunks *red_get_image_data_flat(RedMemSlotInfo *slots, int group_id,
                                             QXLPHYSICAL addr, size_t size)
 {
     SpiceChunks *data;
+    int error;
 
     data = spice_chunks_new(1);
     data->data_size      = size;
-    data->chunk[0].data  = (void*)get_virt(slots, addr, size, group_id);
+    data->chunk[0].data  = (void*)get_virt(slots, addr, size, group_id, &error);
+    if (error) {
+        return 0;
+    }
     data->chunk[0].len   = size;
     return data;
 }
@@ -311,12 +336,16 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
     SpiceImage *red;
     size_t bitmap_size, size;
     uint8_t qxl_flags;
+    int error;
 
     if (addr == 0) {
         return NULL;
     }
 
-    qxl = (QXLImage *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLImage *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return NULL;
+    }
     red = spice_new0(SpiceImage, 1);
     red->descriptor.id     = qxl->descriptor.id;
     red->descriptor.type   = qxl->descriptor.type;
@@ -345,11 +374,16 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
             SpicePalette *rp;
             int i, num_ents;
             qp = (QXLPalette *)get_virt(slots, qxl->bitmap.palette,
-                                        sizeof(*qp), group_id);
+                                        sizeof(*qp), group_id, &error);
+            if (error) {
+                return NULL;
+            }
             num_ents = qp->num_ents;
-            validate_virt(slots, (intptr_t)qp->ents,
-                          get_memslot_id(slots, qxl->bitmap.palette),
-                          num_ents * sizeof(qp->ents[0]), group_id);
+            if (!validate_virt(slots, (intptr_t)qp->ents,
+                               get_memslot_id(slots, qxl->bitmap.palette),
+                               num_ents * sizeof(qp->ents[0]), group_id)) {
+                return NULL;
+            }
             rp = spice_malloc_n_m(num_ents, sizeof(rp->ents[0]), sizeof(*rp));
             rp->unique   = qp->unique;
             rp->num_ents = num_ents;
@@ -374,6 +408,9 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
             size = red_get_data_chunks(slots, group_id,
                                        &chunks, qxl->bitmap.data);
             spice_assert(size == bitmap_size);
+            if (size != bitmap_size) {
+                return NULL;
+            }
             red->u.bitmap.data = red_get_image_data_chunked(slots, group_id,
                                                             &chunks);
             red_put_data_chunks(&chunks);
@@ -391,6 +428,9 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
                                        get_memslot_id(slots, addr),
                                        &chunks, (QXLDataChunk *)qxl->quic.data);
         spice_assert(size == red->u.quic.data_size);
+        if (size != red->u.quic.data_size) {
+            return NULL;
+        }
         red->u.quic.data = red_get_image_data_chunked(slots, group_id,
                                                       &chunks);
         red_put_data_chunks(&chunks);
@@ -491,14 +531,18 @@ static void red_put_opaque(SpiceOpaque *red)
     red_put_qmask(&red->mask);
 }
 
-static void red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
-                             SpiceCopy *red, QXLCopy *qxl, uint32_t flags)
+static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
+                            SpiceCopy *red, QXLCopy *qxl, uint32_t flags)
 {
     red->src_bitmap      = red_get_image(slots, group_id, qxl->src_bitmap, flags);
-   red_get_rect_ptr(&red->src_area, &qxl->src_area);
-   red->rop_descriptor  = qxl->rop_descriptor;
-   red->scale_mode      = qxl->scale_mode;
-   red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);
+    if (!red->src_bitmap) {
+        return 1;
+    }
+    red_get_rect_ptr(&red->src_area, &qxl->src_area);
+    red->rop_descriptor  = qxl->rop_descriptor;
+    red->scale_mode      = qxl->scale_mode;
+    red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);
+    return 0;
 }
 
 static void red_put_copy(SpiceCopy *red)
@@ -580,10 +624,15 @@ static void red_put_rop3(SpiceRop3 *red)
     red_put_qmask(&red->mask);
 }
 
-static void red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
-                               SpiceStroke *red, QXLStroke *qxl, uint32_t flags)
+static int red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
+                              SpiceStroke *red, QXLStroke *qxl, uint32_t flags)
 {
+    int error;
+
     red->path = red_get_path(slots, group_id, qxl->path);
+    if (!red->path) {
+        return 1;
+    }
     red->attr.flags       = qxl->attr.flags;
     if (red->attr.flags & SPICE_LINE_FLAGS_STYLED) {
         int style_nseg;
@@ -594,7 +643,10 @@ static void red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
         red->attr.style_nseg  = style_nseg;
         spice_assert(qxl->attr.style);
         buf = (uint8_t *)get_virt(slots, qxl->attr.style,
-                                  style_nseg * sizeof(QXLFIXED), group_id);
+                                  style_nseg * sizeof(QXLFIXED), group_id, &error);
+        if (error) {
+            return error;
+        }
         memcpy(red->attr.style, buf, style_nseg * sizeof(QXLFIXED));
     } else {
         red->attr.style_nseg  = 0;
@@ -603,6 +655,7 @@ static void red_get_stroke_ptr(RedMemSlotInfo *slots, int group_id,
     red_get_brush_ptr(slots, group_id, &red->brush, &qxl->brush, flags);
     red->fore_mode        = qxl->fore_mode;
     red->back_mode        = qxl->back_mode;
+    return 0;
 }
 
 static void red_put_stroke(SpiceStroke *red)
@@ -626,11 +679,19 @@ static SpiceString *red_get_string(RedMemSlotInfo *slots, int group_id,
     bool free_data;
     size_t chunk_size, qxl_size, red_size, glyph_size;
     int glyphs, bpp = 0, i;
+    int error;
 
-    qxl = (QXLString *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLString *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return NULL;
+    }
     chunk_size = red_get_data_chunks_ptr(slots, group_id,
                                          get_memslot_id(slots, addr),
                                          &chunks, &qxl->chunk);
+    if (!chunk_size) {
+        /* XXX could be a zero sized string.. */
+        return NULL;
+    }
     data = red_linearize_chunk(&chunks, chunk_size, &free_data);
     red_put_data_chunks(&chunks);
 
@@ -760,13 +821,17 @@ static void red_put_clip(SpiceClip *red)
     }
 }
 
-static void red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
-                                    RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
+static int red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
+                                   RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
 {
     QXLDrawable *qxl;
     int i;
+    int error = 0;
 
-    qxl = (QXLDrawable *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLDrawable *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return error;
+    }
     red->release_info     = &qxl->release_info;
 
     red_get_rect_ptr(&red->bbox, &qxl->bbox);
@@ -796,7 +861,7 @@ static void red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
         red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
         break;
     case QXL_DRAW_COPY:
-        red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
+        error = red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
         break;
     case QXL_COPY_BITS:
         red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
@@ -816,7 +881,7 @@ static void red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
         red_get_rop3_ptr(slots, group_id, &red->u.rop3, &qxl->u.rop3, flags);
         break;
     case QXL_DRAW_STROKE:
-        red_get_stroke_ptr(slots, group_id, &red->u.stroke, &qxl->u.stroke, flags);
+        error = red_get_stroke_ptr(slots, group_id, &red->u.stroke, &qxl->u.stroke, flags);
         break;
     case QXL_DRAW_TEXT:
         red_get_text_ptr(slots, group_id, &red->u.text, &qxl->u.text, flags);
@@ -831,16 +896,22 @@ static void red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
         break;
     default:
         spice_error("unknown type %d", red->type);
+        error = 1;
         break;
     };
+    return error;
 }
 
-static void red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
-                                    RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
+static int red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
+                                   RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
 {
     QXLCompatDrawable *qxl;
+    int error;
 
-    qxl = (QXLCompatDrawable *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLCompatDrawable *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return error;
+    }
     red->release_info     = &qxl->release_info;
 
     red_get_rect_ptr(&red->bbox, &qxl->bbox);
@@ -869,7 +940,7 @@ static void red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
         red_get_blend_ptr(slots, group_id, &red->u.blend, &qxl->u.blend, flags);
         break;
     case QXL_DRAW_COPY:
-        red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
+        error = red_get_copy_ptr(slots, group_id, &red->u.copy, &qxl->u.copy, flags);
         break;
     case QXL_COPY_BITS:
         red_get_point_ptr(&red->u.copy_bits.src_pos, &qxl->u.copy_bits.src_pos);
@@ -896,7 +967,7 @@ static void red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
         red_get_rop3_ptr(slots, group_id, &red->u.rop3, &qxl->u.rop3, flags);
         break;
     case QXL_DRAW_STROKE:
-        red_get_stroke_ptr(slots, group_id, &red->u.stroke, &qxl->u.stroke, flags);
+        error = red_get_stroke_ptr(slots, group_id, &red->u.stroke, &qxl->u.stroke, flags);
         break;
     case QXL_DRAW_TEXT:
         red_get_text_ptr(slots, group_id, &red->u.text, &qxl->u.text, flags);
@@ -911,18 +982,23 @@ static void red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
         break;
     default:
         spice_error("unknown type %d", red->type);
+        error = 1;
         break;
     };
+    return error;
 }
 
-void red_get_drawable(RedMemSlotInfo *slots, int group_id,
+int red_get_drawable(RedMemSlotInfo *slots, int group_id,
                       RedDrawable *red, QXLPHYSICAL addr, uint32_t flags)
 {
+    int ret;
+
     if (flags & QXL_COMMAND_FLAG_COMPAT) {
-        red_get_compat_drawable(slots, group_id, red, addr, flags);
+        ret = red_get_compat_drawable(slots, group_id, red, addr, flags);
     } else {
-        red_get_native_drawable(slots, group_id, red, addr, flags);
+        ret = red_get_native_drawable(slots, group_id, red, addr, flags);
     }
+    return ret;
 }
 
 void red_put_drawable(RedDrawable *red)
@@ -968,17 +1044,22 @@ void red_put_drawable(RedDrawable *red)
     }
 }
 
-void red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
-                        RedUpdateCmd *red, QXLPHYSICAL addr)
+int red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
+                       RedUpdateCmd *red, QXLPHYSICAL addr)
 {
     QXLUpdateCmd *qxl;
+    int error;
 
-    qxl = (QXLUpdateCmd *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLUpdateCmd *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return 1;
+    }
     red->release_info     = &qxl->release_info;
 
     red_get_rect_ptr(&red->area, &qxl->area);
     red->update_id  = qxl->update_id;
     red->surface_id = qxl->surface_id;
+    return 0;
 }
 
 void red_put_update_cmd(RedUpdateCmd *red)
@@ -986,10 +1067,11 @@ void red_put_update_cmd(RedUpdateCmd *red)
     /* nothing yet */
 }
 
-void red_get_message(RedMemSlotInfo *slots, int group_id,
-                     RedMessage *red, QXLPHYSICAL addr)
+int red_get_message(RedMemSlotInfo *slots, int group_id,
+                    RedMessage *red, QXLPHYSICAL addr)
 {
     QXLMessage *qxl;
+    int error;
 
     /*
      * security alert:
@@ -997,9 +1079,13 @@ void red_get_message(RedMemSlotInfo *slots, int group_id,
      *   luckily this is for debug logging only,
      *   so we can just ignore it by default.
      */
-    qxl = (QXLMessage *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLMessage *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return 1;
+    }
     red->release_info  = &qxl->release_info;
     red->data          = qxl->data;
+    return 0;
 }
 
 void red_put_message(RedMessage *red)
@@ -1007,13 +1093,18 @@ void red_put_message(RedMessage *red)
     /* nothing yet */
 }
 
-void red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
-                         RedSurfaceCmd *red, QXLPHYSICAL addr)
+int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
+                        RedSurfaceCmd *red, QXLPHYSICAL addr)
 {
     QXLSurfaceCmd *qxl;
     size_t size;
+    int error;
 
-    qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLSurfaceCmd *)get_virt(slots, addr, sizeof(*qxl), group_id,
+                                    &error);
+    if (error) {
+        return 1;
+    }
     red->release_info     = &qxl->release_info;
 
     red->surface_id = qxl->surface_id;
@@ -1028,9 +1119,13 @@ void red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
         red->u.surface_create.stride = qxl->u.surface_create.stride;
         size = red->u.surface_create.height * abs(red->u.surface_create.stride);
         red->u.surface_create.data =
-            (uint8_t*)get_virt(slots, qxl->u.surface_create.data, size, group_id);
+            (uint8_t*)get_virt(slots, qxl->u.surface_create.data, size, group_id, &error);
+        if (error) {
+            return 1;
+        }
         break;
     }
+    return 0;
 }
 
 void red_put_surface_cmd(RedSurfaceCmd *red)
@@ -1038,16 +1133,20 @@ void red_put_surface_cmd(RedSurfaceCmd *red)
     /* nothing yet */
 }
 
-static void red_get_cursor(RedMemSlotInfo *slots, int group_id,
-                           SpiceCursor *red, QXLPHYSICAL addr)
+static int red_get_cursor(RedMemSlotInfo *slots, int group_id,
+                          SpiceCursor *red, QXLPHYSICAL addr)
 {
     QXLCursor *qxl;
     RedDataChunk chunks;
     size_t size;
     uint8_t *data;
     bool free_data;
+    int error;
 
-    qxl = (QXLCursor *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLCursor *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return 1;
+    }
 
     red->header.unique     = qxl->header.unique;
     red->header.type       = qxl->header.type;
@@ -1069,6 +1168,7 @@ static void red_get_cursor(RedMemSlotInfo *slots, int group_id,
         red->data = spice_malloc(size);
         memcpy(red->data, data, size);
     }
+    return 0;
 }
 
 static void red_put_cursor(SpiceCursor *red)
@@ -1076,12 +1176,16 @@ static void red_put_cursor(SpiceCursor *red)
     free(red->data);
 }
 
-void red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
-                        RedCursorCmd *red, QXLPHYSICAL addr)
+int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
+                       RedCursorCmd *red, QXLPHYSICAL addr)
 {
     QXLCursorCmd *qxl;
+    int error;
 
-    qxl = (QXLCursorCmd *)get_virt(slots, addr, sizeof(*qxl), group_id);
+    qxl = (QXLCursorCmd *)get_virt(slots, addr, sizeof(*qxl), group_id, &error);
+    if (error) {
+        return error;
+    }
     red->release_info     = &qxl->release_info;
 
     red->type = qxl->type;
@@ -1089,7 +1193,7 @@ void red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
     case QXL_CURSOR_SET:
         red_get_point16_ptr(&red->u.set.position, &qxl->u.set.position);
         red->u.set.visible  = qxl->u.set.visible;
-        red_get_cursor(slots, group_id,  &red->u.set.shape, qxl->u.set.shape);
+        error = red_get_cursor(slots, group_id,  &red->u.set.shape, qxl->u.set.shape);
         break;
     case QXL_CURSOR_MOVE:
         red_get_point16_ptr(&red->u.position, &qxl->u.position);
@@ -1099,6 +1203,7 @@ void red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
         red->u.trail.frequency = qxl->u.trail.frequency;
         break;
     }
+    return error;
 }
 
 void red_put_cursor_cmd(RedCursorCmd *red)
diff --git a/server/red_parse_qxl.h b/server/red_parse_qxl.h
index c2edfb92..d01d363e 100644
--- a/server/red_parse_qxl.h
+++ b/server/red_parse_qxl.h
@@ -113,25 +113,25 @@ typedef struct RedCursorCmd {
 
 void red_get_rect_ptr(SpiceRect *red, const QXLRect *qxl);
 
-void red_get_drawable(RedMemSlotInfo *slots, int group_id,
-                      RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
+int red_get_drawable(RedMemSlotInfo *slots, int group_id,
+                     RedDrawable *red, QXLPHYSICAL addr, uint32_t flags);
 void red_put_drawable(RedDrawable *red);
 void red_put_image(SpiceImage *red);
 
-void red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
-                        RedUpdateCmd *red, QXLPHYSICAL addr);
+int red_get_update_cmd(RedMemSlotInfo *slots, int group_id,
+                       RedUpdateCmd *red, QXLPHYSICAL addr);
 void red_put_update_cmd(RedUpdateCmd *red);
 
-void red_get_message(RedMemSlotInfo *slots, int group_id,
-                     RedMessage *red, QXLPHYSICAL addr);
+int red_get_message(RedMemSlotInfo *slots, int group_id,
+                    RedMessage *red, QXLPHYSICAL addr);
 void red_put_message(RedMessage *red);
 
-void red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
-                         RedSurfaceCmd *red, QXLPHYSICAL addr);
+int red_get_surface_cmd(RedMemSlotInfo *slots, int group_id,
+                        RedSurfaceCmd *red, QXLPHYSICAL addr);
 void red_put_surface_cmd(RedSurfaceCmd *red);
 
-void red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
-                        RedCursorCmd *red, QXLPHYSICAL addr);
+int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
+                       RedCursorCmd *red, QXLPHYSICAL addr);
 void red_put_cursor_cmd(RedCursorCmd *red);
 
 #endif
diff --git a/server/red_worker.c b/server/red_worker.c
index c17a7d06..07782c8d 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -4671,9 +4671,10 @@ static int red_process_cursor(RedWorker *worker, uint32_t max_pipe_size, int *ri
         case QXL_CMD_CURSOR: {
             RedCursorCmd *cursor = spice_new0(RedCursorCmd, 1);
 
-            red_get_cursor_cmd(&worker->mem_slots, ext_cmd.group_id,
-                               cursor, ext_cmd.cmd.data);
-            qxl_process_cursor(worker, cursor, ext_cmd.group_id);
+            if (!red_get_cursor_cmd(&worker->mem_slots, ext_cmd.group_id,
+                                    cursor, ext_cmd.cmd.data)) {
+                qxl_process_cursor(worker, cursor, ext_cmd.group_id);
+            }
             break;
         }
         default:
@@ -4727,8 +4728,10 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
         case QXL_CMD_DRAW: {
             RedDrawable *drawable = red_drawable_new(); // returns with 1 ref
 
-            red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
-                             drawable, ext_cmd.cmd.data, ext_cmd.flags);
+            if (red_get_drawable(&worker->mem_slots, ext_cmd.group_id,
+                                 drawable, ext_cmd.cmd.data, ext_cmd.flags)) {
+                break;
+            }
             red_process_drawable(worker, drawable, ext_cmd.group_id);
             // release the red_drawable
             put_red_drawable(worker, drawable, ext_cmd.group_id, NULL);
@@ -4738,8 +4741,10 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
             RedUpdateCmd update;
             QXLReleaseInfoExt release_info_ext;
 
-            red_get_update_cmd(&worker->mem_slots, ext_cmd.group_id,
-                               &update, ext_cmd.cmd.data);
+            if (red_get_update_cmd(&worker->mem_slots, ext_cmd.group_id,
+                                   &update, ext_cmd.cmd.data)) {
+                break;
+            }
             validate_surface(worker, update.surface_id);
             red_update_area(worker, &update.area, update.surface_id);
             worker->qxl->st->qif->notify_update(worker->qxl, update.update_id);
@@ -4753,8 +4758,10 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
             RedMessage message;
             QXLReleaseInfoExt release_info_ext;
 
-            red_get_message(&worker->mem_slots, ext_cmd.group_id,
-                            &message, ext_cmd.cmd.data);
+            if (red_get_message(&worker->mem_slots, ext_cmd.group_id,
+                                &message, ext_cmd.cmd.data)) {
+                break;
+            }
 #ifdef DEBUG
             /* alert: accessing message.data is insecure */
             spice_printerr("MESSAGE: %s", message.data);
@@ -4768,8 +4775,10 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
         case QXL_CMD_SURFACE: {
             RedSurfaceCmd *surface = spice_new0(RedSurfaceCmd, 1);
 
-            red_get_surface_cmd(&worker->mem_slots, ext_cmd.group_id,
-                                surface, ext_cmd.cmd.data);
+            if (red_get_surface_cmd(&worker->mem_slots, ext_cmd.group_id,
+                                    surface, ext_cmd.cmd.data)) {
+                break;
+            }
             red_process_surface(worker, surface, ext_cmd.group_id, FALSE);
             break;
         }
@@ -10417,6 +10426,7 @@ static void dev_create_primary_surface(RedWorker *worker, uint32_t surface_id,
                                        QXLDevSurfaceCreate surface)
 {
     uint8_t *line_0;
+    int error;
 
     spice_warn_if(surface_id != 0);
     spice_warn_if(surface.height == 0);
@@ -10424,7 +10434,11 @@ static void dev_create_primary_surface(RedWorker *worker, uint32_t surface_id,
              abs(surface.stride) * surface.height);
 
     line_0 = (uint8_t*)get_virt(&worker->mem_slots, surface.mem,
-                                surface.height * abs(surface.stride), surface.group_id);
+                                surface.height * abs(surface.stride),
+                                surface.group_id, &error);
+    if (error) {
+        return;
+    }
     if (surface.stride < 0) {
         line_0 -= (int32_t)(surface.stride * (surface.height -1));
     }
@@ -10830,8 +10844,13 @@ void handle_dev_loadvm_commands(void *opaque, void *payload)
         switch (ext[i].cmd.type) {
         case QXL_CMD_CURSOR:
             cursor_cmd = spice_new0(RedCursorCmd, 1);
-            red_get_cursor_cmd(&worker->mem_slots, ext[i].group_id,
-                               cursor_cmd, ext[i].cmd.data);
+            if (red_get_cursor_cmd(&worker->mem_slots, ext[i].group_id,
+                                   cursor_cmd, ext[i].cmd.data)) {
+                /* XXX allow failure in loadvm? */
+                spice_printerr("failed loadvm command type (%d)",
+                               ext[i].cmd.type);
+                continue;
+            }
             qxl_process_cursor(worker, cursor_cmd, ext[i].group_id);
             break;
         case QXL_CMD_SURFACE: