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

login
register
mail settings
Submitter Andrew Lutomirski
Date May 4, 2010, 10:12 p.m.
Message ID <o2xcb0375e11005041512i948aa9exb484565141a9f957@mail.gmail.com>
Download mbox | patch
Permalink /patch/51701/
State New
Headers show

Comments

Andrew Lutomirski - May 4, 2010, 10:12 p.m.
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);
Andrew Lutomirski - May 5, 2010, 4:02 a.m.
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

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