[Qemu-devel] VNC heap corruption when display width is not a multiple of 16

[Qemu-devel] VNC heap corruption when display width is not a multiple of 16

[Andrew Lutomirski]

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


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);
         }
     }
 }
@@ -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);

[Qemu-devel] Re: VNC heap corruption when display width is not a multiple of 16

[Andrew Lutomirski]

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