Re: [Qemu-devel] Unknown command 0xffffff in SVGA command FIFO

[Janne Huttunen <jahuttun@gmail.com>]

> Here's an experiment for sanity checking the lengths and leaving
> the command in the FIFO if it is not complete. It fixes the
> problem for me (running it right now), but I agree that it's not
> very elegant to look at :-/ .

And here's another version with couple of stupid bugs removed
(it obviously is not a good idea to busyloop if the command is
incomplete).

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 12bff48..839f715 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -526,6 +526,17 @@ static inline int vmsvga_fifo_empty(struct vmsvga_state_s *s)
      return (s->cmd->next_cmd == s->cmd->stop);
  }

+static inline int vmsvga_fifo_items(struct vmsvga_state_s *s)
+{
+    int num;
+    if (!s->config || !s->enable)
+        return 0;
+    num = CMD(next_cmd) - CMD(stop);
+    if (num < 0)
+        num += (CMD(max) - CMD(min));
+    return (num >> 2);
+}
+
  static inline uint32_t vmsvga_fifo_read_raw(struct vmsvga_state_s *s)
  {
      uint32_t cmd = s->fifo[CMD(stop) >> 2];
@@ -540,6 +551,14 @@ static inline uint32_t vmsvga_fifo_read(struct 
vmsvga_state_s *s)
      return le32_to_cpu(vmsvga_fifo_read_raw(s));
  }

+static inline uint32_t vmsvga_fifo_peek(struct vmsvga_state_s *s, uint32_t offs)
+{
+    offs = (offs << 2) + CMD(stop);
+    if (offs >= CMD(max))
+        offs = offs - CMD(max) + CMD(min);
+    return le32_to_cpu(s->fifo[offs >> 2]);
+}
+
  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
  {
      uint32_t cmd, colour;
@@ -547,9 +566,12 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
      int x, y, dx, dy, width, height;
      struct vmsvga_cursor_definition_s cursor;
      while (!vmsvga_fifo_empty(s))
-        switch (cmd = vmsvga_fifo_read(s)) {
+        switch (cmd = vmsvga_fifo_peek(s, 0)) {
          case SVGA_CMD_UPDATE:
          case SVGA_CMD_UPDATE_VERBOSE:
+            if (vmsvga_fifo_items(s) < 5)
+                goto out;
+            vmsvga_fifo_read(s);
              x = vmsvga_fifo_read(s);
              y = vmsvga_fifo_read(s);
              width = vmsvga_fifo_read(s);
@@ -558,6 +580,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
              break;

          case SVGA_CMD_RECT_FILL:
+            if (vmsvga_fifo_items(s) < 6)
+                goto out;
+            vmsvga_fifo_read(s);
              colour = vmsvga_fifo_read(s);
              x = vmsvga_fifo_read(s);
              y = vmsvga_fifo_read(s);
@@ -571,6 +596,9 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
  #endif

          case SVGA_CMD_RECT_COPY:
+            if (vmsvga_fifo_items(s) < 7)
+                goto out;
+            vmsvga_fifo_read(s);
              x = vmsvga_fifo_read(s);
              y = vmsvga_fifo_read(s);
              dx = vmsvga_fifo_read(s);
@@ -585,13 +613,20 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
  #endif

          case SVGA_CMD_DEFINE_CURSOR:
-            cursor.id = vmsvga_fifo_read(s);
-            cursor.hot_x = vmsvga_fifo_read(s);
-            cursor.hot_y = vmsvga_fifo_read(s);
-            cursor.width = x = vmsvga_fifo_read(s);
-            cursor.height = y = vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            cursor.bpp = vmsvga_fifo_read(s);
+            if (vmsvga_fifo_items(s) < 8)
+                goto out;
+            cursor.id = vmsvga_fifo_peek(s, 1);
+            cursor.hot_x = vmsvga_fifo_peek(s, 2);
+            cursor.hot_y = vmsvga_fifo_peek(s, 3);
+            cursor.width = x = vmsvga_fifo_peek(s, 4);
+            cursor.height = y = vmsvga_fifo_peek(s, 5);
+            cursor.bpp = vmsvga_fifo_peek(s, 7);
+
+            if (vmsvga_fifo_items(s) < SVGA_BITMAP_SIZE(x, y) + 
SVGA_PIXMAP_SIZE(x, y, cursor.bpp) + 8)
+                goto out;
+
+            for (args = 0; args < 8; args++)
+                vmsvga_fifo_read(s);

  	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask ||
  		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image) {
@@ -616,25 +651,43 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
           * for so we can avoid FIFO desync if driver uses them illegally.
           */
          case SVGA_CMD_DEFINE_ALPHA_CURSOR:
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            vmsvga_fifo_read(s);
-            x = vmsvga_fifo_read(s);
-            y = vmsvga_fifo_read(s);
+            if (vmsvga_fifo_items(s) < 6)
+                goto out;
+            x = vmsvga_fifo_peek(s, 4);
+            y = vmsvga_fifo_peek(s, 5);
+            if (vmsvga_fifo_items(s) < x * y + 6)
+                goto out;
+            for (args = 0; args < 6; args++)
+                vmsvga_fifo_read(s);
              args = x * y;
              goto badcmd;
          case SVGA_CMD_RECT_ROP_FILL:
+            if (vmsvga_fifo_items(s) < 7)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 6;
              goto badcmd;
          case SVGA_CMD_RECT_ROP_COPY:
+            if (vmsvga_fifo_items(s) < 8)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 7;
              goto badcmd;
          case SVGA_CMD_DRAW_GLYPH_CLIPPED:
+            if (vmsvga_fifo_items(s) < 4)
+                goto out;
+            args = 7 + (vmsvga_fifo_peek(s, 3) >> 2);
+            if (vmsvga_fifo_items(s) < args + 4)
+                goto out;
+            vmsvga_fifo_read(s);
+            vmsvga_fifo_read(s);
              vmsvga_fifo_read(s);
              vmsvga_fifo_read(s);
-            args = 7 + (vmsvga_fifo_read(s) >> 2);
              goto badcmd;
          case SVGA_CMD_SURFACE_ALPHA_BLEND:
+            if (vmsvga_fifo_items(s) < 13)
+                goto out;
+            vmsvga_fifo_read(s);
              args = 12;
              goto badcmd;

@@ -647,6 +700,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
          case SVGA_CMD_FRONT_ROP_FILL:
          case SVGA_CMD_FENCE:
          case SVGA_CMD_INVALID_CMD:
+            vmsvga_fifo_read(s);
              break; /* Nop */

          default:
@@ -658,6 +712,7 @@ static void vmsvga_fifo_run(struct vmsvga_state_s *s)
              break;
          }

+out:
      s->syncing = 0;
  }