[Qemu-devel] [RFC] qxl: don't panic on phys2virt

[Alon Levy]

Issues a qxl_guest_bug from qxl_phys2virt. Everywhere else will fail.
qxl_phys2virt requires an additional argument because all it's possible
return values are legit (well, I could use the fact it returns a pointer
so it should be word aligned but I don't want to go there, this is
totally internal).

Signed-off-by: Alon Levy <address@hidden>
---
 hw/qxl-logger.c |   38 +++++++++++++++++++----------
 hw/qxl-render.c |   14 +++++++++--
 hw/qxl.c        |   72 +++++++++++++++++++++++++++++++++++++++++++++----------
 hw/qxl.h        |    3 ++-
 4 files changed, 99 insertions(+), 28 deletions(-)

diff --git a/hw/qxl-logger.c b/hw/qxl-logger.c
index 367aad1..1b8baee 100644
--- a/hw/qxl-logger.c
+++ b/hw/qxl-logger.c
@@ -100,12 +100,16 @@ static const char *qxl_v2n(const char *n[], size_t l, int 
v)
 }
 #define qxl_name(_list, _value) qxl_v2n(_list, ARRAY_SIZE(_list), _value)
 
-static void qxl_log_image(PCIQXLDevice *qxl, QXLPHYSICAL addr, int group_id)
+static void qxl_log_image(PCIQXLDevice *qxl, QXLPHYSICAL addr, int group_id,
+                          int *error)
 {
     QXLImage *image;
     QXLImageDescriptor *desc;
 
-    image = qxl_phys2virt(qxl, addr, group_id);
+    image = qxl_phys2virt(qxl, addr, group_id, error);
+    if (*error) {
+        return;
+    }
     desc = &image->descriptor;
     fprintf(stderr, " (id %" PRIx64 " type %d flags %d width %d height %d",
             desc->id, desc->type, desc->flags, desc->width, desc->height);
@@ -130,17 +134,19 @@ static void qxl_log_rect(QXLRect *rect)
             rect->left, rect->top);
 }
 
-static void qxl_log_cmd_draw_copy(PCIQXLDevice *qxl, QXLCopy *copy, int 
group_id)
+static void qxl_log_cmd_draw_copy(PCIQXLDevice *qxl, QXLCopy *copy,
+                                  int group_id, int *error)
 {
     fprintf(stderr, " src %" PRIx64,
             copy->src_bitmap);
-    qxl_log_image(qxl, copy->src_bitmap, group_id);
+    qxl_log_image(qxl, copy->src_bitmap, group_id, error);
     fprintf(stderr, " area");
     qxl_log_rect(&copy->src_area);
     fprintf(stderr, " rop %d", copy->rop_descriptor);
 }
 
-static void qxl_log_cmd_draw(PCIQXLDevice *qxl, QXLDrawable *draw, int 
group_id)
+static void qxl_log_cmd_draw(PCIQXLDevice *qxl, QXLDrawable *draw, int 
group_id,
+                             int *error)
 {
     fprintf(stderr, ": surface_id %d type %s effect %s",
             draw->surface_id,
@@ -148,13 +154,13 @@ static void qxl_log_cmd_draw(PCIQXLDevice *qxl, 
QXLDrawable *draw, int group_id)
             qxl_name(qxl_draw_effect, draw->effect));
     switch (draw->type) {
     case QXL_DRAW_COPY:
-        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
+        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id, error);
         break;
     }
 }
 
 static void qxl_log_cmd_draw_compat(PCIQXLDevice *qxl, QXLCompatDrawable *draw,
-                                    int group_id)
+                                    int group_id, int *error)
 {
     fprintf(stderr, ": type %s effect %s",
             qxl_name(qxl_draw_type, draw->type),
@@ -166,7 +172,7 @@ static void qxl_log_cmd_draw_compat(PCIQXLDevice *qxl, 
QXLCompatDrawable *draw,
     }
     switch (draw->type) {
     case QXL_DRAW_COPY:
-        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id);
+        qxl_log_cmd_draw_copy(qxl, &draw->u.copy, group_id, error);
         break;
     }
 }
@@ -192,6 +198,7 @@ static void qxl_log_cmd_surface(PCIQXLDevice *qxl, 
QXLSurfaceCmd *cmd)
 void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id)
 {
     QXLCursor *cursor;
+    int error;
 
     fprintf(stderr, ": %s",
             qxl_name(qxl_cursor_cmd, cmd->type));
@@ -202,7 +209,10 @@ void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd 
*cmd, int group_id)
                 cmd->u.set.position.y,
                 cmd->u.set.visible ? "yes" : "no",
                 cmd->u.set.shape);
-        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id);
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id, &error);
+        if (error) {
+            return;
+        }
         fprintf(stderr, " type %s size %dx%d hot-spot +%d+%d"
                 " unique 0x%" PRIx64 " data-size %d",
                 qxl_name(spice_cursor_type, cursor->header.type),
@@ -220,6 +230,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, 
QXLCommandExt *ext)
 {
     bool compat = ext->flags & QXL_COMMAND_FLAG_COMPAT;
     void *data;
+    int error;
 
     if (!qxl->cmdlog) {
         return;
@@ -230,13 +241,16 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, 
QXLCommandExt *ext)
             qxl_name(qxl_type, ext->cmd.type),
             compat ? "(compat)" : "");
 
-    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id, &error);
+    if (error) {
+        return;
+    }
     switch (ext->cmd.type) {
     case QXL_CMD_DRAW:
         if (!compat) {
-            qxl_log_cmd_draw(qxl, data, ext->group_id);
+            qxl_log_cmd_draw(qxl, data, ext->group_id, &error);
         } else {
-            qxl_log_cmd_draw_compat(qxl, data, ext->group_id);
+            qxl_log_cmd_draw_compat(qxl, data, ext->group_id, &error);
         }
         break;
     case QXL_CMD_SURFACE:
diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 28ab182..04eca5c 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -229,10 +229,16 @@ fail:
 /* called from spice server thread context only */
 void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
 {
-    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    int error;
+    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                      &error);
     QXLCursor *cursor;
     QEMUCursor *c;
 
+    if (error) {
+        return;
+    }
+
     if (!qxl->ssd.ds->mouse_set || !qxl->ssd.ds->cursor_define) {
         return;
     }
@@ -244,7 +250,11 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt 
*ext)
     }
     switch (cmd->type) {
     case QXL_CURSOR_SET:
-        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id,
+                               &error);
+        if (error) {
+            return;
+        }
         if (cursor->chunk.data_size != cursor->data_size) {
             fprintf(stderr, "%s: multiple chunks\n", __FUNCTION__);
             return;
diff --git a/hw/qxl.c b/hw/qxl.c
index db2318e..e7ffe99 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -382,15 +382,32 @@ static void qxl_ring_set_dirty(PCIQXLDevice *qxl)
 /*
  * keep track of some command state, for savevm/loadvm.
  * called from spice server thread context only
+ *
+ * Returns 0 on success, 1 on failure.
+ *
+ * Failure is caused by:
+ *  non initialized slots (qxl_phys2virt failure)
+ *  illegal command
+ *   surface id larger then NUM_SURFACES
  */
-static void qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
+static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
 {
+    int error;
+
     switch (le32_to_cpu(ext->cmd.type)) {
     case QXL_CMD_SURFACE:
     {
-        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                           &error);
+        if (error) {
+            return 1;
+        }
         uint32_t id = le32_to_cpu(cmd->surface_id);
-        PANIC_ON(id >= NUM_SURFACES);
+
+        if (id >= NUM_SURFACES) {
+            qxl_guest_bug(qxl, "QXL_CMD_SURFACE id %d >= %d", id, 
NUM_SURFACES);
+            return 1;
+        }
         qemu_mutex_lock(&qxl->track_lock);
         if (cmd->type == QXL_SURFACE_CMD_CREATE) {
             qxl->guest_surfaces.cmds[id] = ext->cmd.data;
@@ -407,7 +424,12 @@ static void qxl_track_command(PCIQXLDevice *qxl, struct 
QXLCommandExt *ext)
     }
     case QXL_CMD_CURSOR:
     {
-        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        int error;
+        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                          &error);
+        if (error) {
+            return 1;
+        }
         if (cmd->type == QXL_CURSOR_SET) {
             qemu_mutex_lock(&qxl->track_lock);
             qxl->guest_cursor = ext->cmd.data;
@@ -416,6 +438,7 @@ static void qxl_track_command(PCIQXLDevice *qxl, struct 
QXLCommandExt *ext)
         break;
     }
     }
+    return 0;
 }
 
 /* spice display interface callbacks */
@@ -1087,25 +1110,45 @@ static void qxl_reset_surfaces(PCIQXLDevice *d)
 }
 
 /* can be also called from spice server thread context */
-void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
+/* NULL is returned on error, but it can also be a legitimate return
+ * value, so this function needs to be changed to pass error by another
+ * argument */
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
+                    int *error)
 {
     uint64_t phys   = le64_to_cpu(pqxl);
     uint32_t slot   = (phys >> (64 -  8)) & 0xff;
     uint64_t offset = phys & 0xffffffffffff;
 
+    assert(error);
+    *error = 1;
     switch (group_id) {
     case MEMSLOT_GROUP_HOST:
         return (void *)(intptr_t)offset;
     case MEMSLOT_GROUP_GUEST:
-        PANIC_ON(slot >= NUM_MEMSLOTS);
-        PANIC_ON(!qxl->guest_slots[slot].active);
-        PANIC_ON(offset < qxl->guest_slots[slot].delta);
+        if (slot >= NUM_MEMSLOTS) {
+            qxl_guest_bug(qxl, "slot too large %d >= %d", slot, NUM_MEMSLOTS);
+            return NULL;
+        }
+        if (!qxl->guest_slots[slot].active) {
+            qxl_guest_bug(qxl, "inactive slot %d\n", slot);
+            return NULL;
+        }
+        if (offset < qxl->guest_slots[slot].delta) {
+            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
+                          slot, offset, qxl->guest_slots[slot].delta);
+            return NULL;
+        }
         offset -= qxl->guest_slots[slot].delta;
-        PANIC_ON(offset > qxl->guest_slots[slot].size)
+        if (offset > qxl->guest_slots[slot].size) {
+            qxl_guest_bug(qxl, "slot %d offset %"PRIu64" > size %"PRIu64"\n",
+                          slot, offset, qxl->guest_slots[slot].size);
+            return NULL;
+        }
+        *error = 0;
         return qxl->guest_slots[slot].ptr + offset;
-    default:
-        PANIC_ON(1);
     }
+    return NULL;
 }
 
 static void qxl_create_guest_primary_complete(PCIQXLDevice *qxl)
@@ -1532,6 +1575,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
 {
     intptr_t vram_start;
     int i;
+    int error;
 
     if (qxl->mode != QXL_MODE_NATIVE && qxl->mode != QXL_MODE_COMPAT) {
         return;
@@ -1554,11 +1598,13 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
         }
 
         cmd = qxl_phys2virt(qxl, qxl->guest_surfaces.cmds[i],
-                            MEMSLOT_GROUP_GUEST);
+                            MEMSLOT_GROUP_GUEST, &error);
+        assert(!error);
         assert(cmd->type == QXL_SURFACE_CMD_CREATE);
         surface_offset = (intptr_t)qxl_phys2virt(qxl,
                                                  cmd->u.surface_create.data,
-                                                 MEMSLOT_GROUP_GUEST);
+                                                 MEMSLOT_GROUP_GUEST, &error);
+        assert(!error);
         surface_offset -= vram_start;
         surface_size = cmd->u.surface_create.height *
                        abs(cmd->u.surface_create.stride);
diff --git a/hw/qxl.h b/hw/qxl.h
index 11a0db3..4a48d41 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -126,7 +126,8 @@ typedef struct PCIQXLDevice {
 #define QXL_DEFAULT_REVISION QXL_REVISION_STABLE_V10
 
 /* qxl.c */
-void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id,
+                    int *error);
 void qxl_guest_bug(PCIQXLDevice *qxl, const char *msg, ...);
 
 void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
-- 
1.7.9.3