diff mbox

[to,consider,for,0.12] vmware_vga: Don't crash on too-big DEFINE_CURSOR command

Message ID adatyvp44ov.fsf@roland-alpha.cisco.com
State New
Headers show

Commit Message

Roland Dreier Dec. 17, 2009, 10:27 p.m. UTC 
Hi Anthony -- just sent this patch to qemu-devel (although I don't see
it in archives yet).  Anyway I realize it is really really late given
your release timeframe but I think the risk of this pretty minimal, and
the patch fixes a crash in a pretty reasonable config (running a modern
Linux distro with the fastest guest video adapter).  So please consider
this for 0.12.

Another possibility would be to just take the part of the patch that
bumps the array size in the structure, since that seems to have
essentially 0 risk and fixes the crash in the case I've seen.

Thanks,
  Roland

Comments

Roland Dreier Dec. 17, 2009, 10:41 p.m. UTC | #1 
> His last patch has the same fix without the printf().  The printf is
 > probably something to avoid since a malicious guest could create a
 > storm of them.  Since libvirt logs stderr by default, the result could
 > be pretty nasty.

By the way, are the

        fprintf(stderr, "%s: update width too large x: %d, w: %d\n",
                        __FUNCTION__, x, w);

        fprintf(stderr, "%s: update height too large y: %d, h: %d\n",
                        __FUNCTION__, y, h);

prints triggerable by a guest?  (I think so -- if so I can send a patch
removing them if you want)

How about the printf()s to stdout?  eg a guest can cause a flood of the

            printf("%s: Unknown command 0x%02x in SVGA command FIFO\n",
                            __FUNCTION__, cmd);

or

            printf("%s: guest runs %s.\n", __FUNCTION__,
                            vmsvga_guest_id[value - GUEST_OS_BASE]);

output if it wants pretty trivially.
diff mbox

Patch

===

QEMU crashes with vmware_vga when running a Linux guest with the latest
X.org vmware video driver if QEMU is using SDL for video output.  In
this case, vmware_vga advertises cursor acceleration to the guest, and
the crash comes when the guest does a DEFINE_CURSOR command with a 64x64
32bpp cursor.  This request overruns the image[] array in struct
vmsvga_cursor_definition_s and QEMU ends up segfaulting because of
memory corruption caused by writing past the end of the array.

Fix this by enlarging the image[] array to be able to hold 4096 32-bit
pixels so we don't fail for the case of 64*64*32bpp, and also add error
checking to avoid a crash if an even bigger request is sent by a guest.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
 hw/vmware_vga.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index f3e3749..d253a2e 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -462,7 +462,7 @@  struct vmsvga_cursor_definition_s {
     int hot_x;
     int hot_y;
     uint32_t mask[1024];
-    uint32_t image[1024];
+    uint32_t image[4096];	/* allow for 64x64 32bpp cursor */
 };
 
 #define SVGA_BITMAP_SIZE(w, h)		((((w) + 31) >> 5) * (h))
@@ -557,6 +557,15 @@  static void vmsvga_fifo_run(struct vmsvga_state_s *s)
             cursor.height = y = vmsvga_fifo_read(s);
             vmsvga_fifo_read(s);
             cursor.bpp = vmsvga_fifo_read(s);
+
+	    if (SVGA_BITMAP_SIZE(x, y) > sizeof cursor.mask / sizeof (uint32_t) ||
+		SVGA_PIXMAP_SIZE(x, y, cursor.bpp) > sizeof cursor.image / sizeof (uint32_t)) {
+		    fprintf(stderr, "%s: DEFINE_CSURSOR too large x: %d, y: %d bpp: %d\n",
+			    __FUNCTION__, x, y, cursor.bpp);
+		    args = SVGA_BITMAP_SIZE(x, y) + SVGA_PIXMAP_SIZE(x, y, cursor.bpp);
+		    goto badcmd;
+	    }
+
             for (args = 0; args < SVGA_BITMAP_SIZE(x, y); args ++)
                 cursor.mask[args] = vmsvga_fifo_read_raw(s);
             for (args = 0; args < SVGA_PIXMAP_SIZE(x, y, cursor.bpp); args ++)