diff mbox

VNC heap corruption when display width is not a multiple of 16

Message ID o2xcb0375e11005041512i948aa9exb484565141a9f957@mail.gmail.com
State New
Headers show

Commit Message

Andrew Lutomirski May 4, 2010, 10:12 p.m. UTC 
Hi all-

qemu-kvm quite reliably crashes when running with a VNC viewer
connected at 1400x1050.  (The crash happens when changing resolution
*from* 1400x1050 or disconnecting and reconnecting a client.)

The problem is that  vnc_refresh_server_surface overruns server->data
and corrupts heap metadata, causing glibc to explode when the buffer
is freed.

The patch below (obviously only for testing) demonstrates the problem
and prevents the crash, but it introduces a black line 8 pixels wide
on the right when running at 1400x1050.  I'm not sure what's going on
-- either I messed something up (entirely possible) or there's another
bug somewhere else.

Note: I've only reproduced this on Avi's qemu-kvm and on Fedora's
packages -- upstream qemu crashes somewhere else.  The code in
question looks the same upstream, though.

--Andy


 }
@@ -2320,8 +2329,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
      * Check and copy modified bits from guest to server surface.
      * Update server dirty map.
      */
-    vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS);
-    cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
+    vnc_set_bits(width_mask, ((ds_get_width(vd->ds) + 15) / 16),
VNC_DIRTY_WORDS);
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;
     for (y = 0; y < vd->guest.ds->height; y++) {
@@ -2332,12 +2340,20 @@ static int vnc_refresh_server_surface(VncDisplay *vd)

             guest_ptr  = guest_row;
             server_ptr = server_row;
+            int bpp = ds_get_bytes_per_pixel(vd->ds);
+            cmp_bytes = 16 * bpp;

             for (x = 0; x < vd->guest.ds->width;
                     x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
                 if (!vnc_get_bit(vd->guest.dirty[y], (x / 16)))
                     continue;
                 vnc_clear_bit(vd->guest.dirty[y], (x / 16));
+
+		// If this happens, we'll overrun the line.  If it
+		// happens on the last row, we'll corrupt the heap.
+		if (cmp_bytes > bpp * (vd->guest.ds->width - x))
+		    cmp_bytes = bpp * (vd->guest.ds->width - x);
+	
                 if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0)
                     continue;
                 memcpy(server_ptr, guest_ptr, cmp_bytes);

Comments

Andrew Lutomirski May 5, 2010, 4:02 a.m. UTC | #1 
On Tue, May 4, 2010 at 6:12 PM, Andrew Lutomirski <luto@mit.edu> wrote:
> Hi all-
>

> The patch below (obviously only for testing) demonstrates the problem
> and prevents the crash, but it introduces a black line 8 pixels wide
> on the right when running at 1400x1050.  I'm not sure what's going on
> -- either I messed something up (entirely possible) or there's another
> bug somewhere else.

Nope -- the black line is there in unpatched qemu as well.  I just
didn't notice it because qemu was too busy crashing :)

--Andy
diff mbox

Patch

diff --git a/vnc.c b/vnc.c
index e678fcc..f9d0ad3 100644
--- a/vnc.c
+++ b/vnc.c
@@ -527,6 +527,15 @@  static void vnc_dpy_resize(DisplayState *ds)
     vd->server->data = qemu_mallocz(vd->server->linesize *
                                     vd->server->height);

+    printf("vnc_dpy_resize: linesize = %d, height = %d, bpp = %d, "
+	   "width = %d, height = %d, width%%16 = %d, data = %p\n",
+	   vd->server->linesize, vd->server->height,
+	   ds_get_bytes_per_pixel(ds),
+	   ds_get_width(ds), ds_get_height(ds),
+	   ds_get_width(ds) % 16,
+	   vd->server->data);
+
+
     /* guest surface */
     if (!vd->guest.ds)
         vd->guest.ds = qemu_mallocz(sizeof(*vd->guest.ds));
@@ -1740,7 +1749,7 @@  static void framebuffer_update_request(VncState
*vs, int incremental,
         vs->force_update = 1;
         for (i = 0; i < h; i++) {
             vnc_set_bits(vs->dirty[y_position + i],
-                         (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS);
+                         ((ds_get_width(vs->ds) + 15) / 16), VNC_DIRTY_WORDS);
         }
     }