diff mbox

ui/vnc: fix potential memory corruption issues

Message ID 1404113089-7154-1-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven June 30, 2014, 7:24 a.m. UTC 
this patch addresses 2 memory corruption issues.

The first was actually discovered during playing
around with a Windows 7 vServer. During resolution
change in Windows 7 it happens sometimes that Windows
changes to an intermediate resolution where
server_stride % cmp_bytes != 0 (in vnc_refresh_server_surface).
This happens only if width % VNC_DIRTY_PIXELS_PER_BIT != 0.
The patch fixes the issue by clamping cmp_bytes in that case
and it finally makes those resolutions work correctly.
This can be easily tested by setting VNC_DIRTY_PIXELS_PER_BIT
to a bigger power of 2 value different than 16.

The second is a theoretical issue, but is maybe exploitable
by the guest. If for some reason the surface size is bigger
than VNC_MAX_WIDTH x VNC_MAX_HEIGHT we end up in severe corruption.
This can be easily reproduced by playing around with VNC_MAX_WIDTH
and VNC_MAX_HEIGHT. This patch modifies the VNC server to only
track and copy the area up to the maximum possible size.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c |  149 +++++++++++++++++++++++++++++---------------------------------
 ui/vnc.h |   14 +++---
 2 files changed, 77 insertions(+), 86 deletions(-)

Comments

Gerd Hoffmann June 30, 2014, 7:52 a.m. UTC | #1 
On Mo, 2014-06-30 at 09:24 +0200, Peter Lieven wrote:
> this patch addresses 2 memory corruption issues.
> 
> The first was actually discovered during playing
> around with a Windows 7 vServer. During resolution
> change in Windows 7 it happens sometimes that Windows
> changes to an intermediate resolution where
> server_stride % cmp_bytes != 0 (in vnc_refresh_server_surface).
> This happens only if width % VNC_DIRTY_PIXELS_PER_BIT != 0.
> The patch fixes the issue by clamping cmp_bytes in that case
> and it finally makes those resolutions work correctly.
> This can be easily tested by setting VNC_DIRTY_PIXELS_PER_BIT
> to a bigger power of 2 value different than 16.
> 
> The second is a theoretical issue, but is maybe exploitable
> by the guest. If for some reason the surface size is bigger
> than VNC_MAX_WIDTH x VNC_MAX_HEIGHT we end up in severe corruption.
> This can be easily reproduced by playing around with VNC_MAX_WIDTH
> and VNC_MAX_HEIGHT. This patch modifies the VNC server to only
> track and copy the area up to the maximum possible size.

So this basically makes vnc work correctly in case guest surface and
server surface have different sizes, then fixes the two bugs on top of
that.  And it obsoletes the other corruption patch send Friday.
Correct?

cheers,
  Gerd
Peter Lieven June 30, 2014, 8:01 a.m. UTC | #2 
On 30.06.2014 09:52, Gerd Hoffmann wrote:
> On Mo, 2014-06-30 at 09:24 +0200, Peter Lieven wrote:
>> this patch addresses 2 memory corruption issues.
>>
>> The first was actually discovered during playing
>> around with a Windows 7 vServer. During resolution
>> change in Windows 7 it happens sometimes that Windows
>> changes to an intermediate resolution where
>> server_stride % cmp_bytes != 0 (in vnc_refresh_server_surface).
>> This happens only if width % VNC_DIRTY_PIXELS_PER_BIT != 0.
>> The patch fixes the issue by clamping cmp_bytes in that case
>> and it finally makes those resolutions work correctly.
>> This can be easily tested by setting VNC_DIRTY_PIXELS_PER_BIT
>> to a bigger power of 2 value different than 16.
>>
>> The second is a theoretical issue, but is maybe exploitable
>> by the guest. If for some reason the surface size is bigger
>> than VNC_MAX_WIDTH x VNC_MAX_HEIGHT we end up in severe corruption.
>> This can be easily reproduced by playing around with VNC_MAX_WIDTH
>> and VNC_MAX_HEIGHT. This patch modifies the VNC server to only
>> track and copy the area up to the maximum possible size.
> So this basically makes vnc work correctly in case guest surface and
> server surface have different sizes, then fixes the two bugs on top of
> that.  And it obsoletes the other corruption patch send Friday.
> Correct?

Yes and yes.

Basically the server surface is adjusted to not exceed VNC_MAX_WIDTH x VNC_MAX_HEIGHT
and on top the width is rounded up to multiple of VNC_DIRTY_PIXELS_PER_BIT.

If you have a resolution whose width is not dividable by VNC_DIRTY_PIXELS_PER_BIT you get
a small black bar on the right of the screen. First I wanted to fix only that and then
I noticed that VNC_MAX_WIDTH and VNC_MAX_HEIGHT are nowhere enforced. I do not
know if this is actually exploitable by the guest.

If the surface is too big to fit the limits only the upper left area is shown.

I ran several sessions with altered values of VNC_MAX_WIDTH, VNC_MAX_HEIGHT and VNC_DIRTY_PIXELS_PER_BIT
in valgrind, but additional testing is welcome.

Peter
Gerd Hoffmann June 30, 2014, 8:35 a.m. UTC | #3 
On Mo, 2014-06-30 at 10:01 +0200, Peter Lieven wrote:
> On 30.06.2014 09:52, Gerd Hoffmann wrote:
> > So this basically makes vnc work correctly in case guest surface and
> > server surface have different sizes, then fixes the two bugs on top of
> > that.  And it obsoletes the other corruption patch send Friday.
> > Correct?
> 
> Yes and yes.

Good, so I'm reading the code correctly ;)
Can you put that information into the patch description please?
Otherwise the patch is fine.

thanks,
  Gerd
diff mbox

Patch

diff --git a/ui/vnc.c b/ui/vnc.c
index 14a86c3..a6d748b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -432,14 +432,10 @@  static void framebuffer_update_request(VncState *vs, int incremental,
 static void vnc_refresh(DisplayChangeListener *dcl);
 static int vnc_refresh_server_surface(VncDisplay *vd);
 
-static void vnc_dpy_update(DisplayChangeListener *dcl,
-                           int x, int y, int w, int h)
-{
-    VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
-    struct VncSurface *s = &vd->guest;
-    int width = surface_width(vd->ds);
-    int height = surface_height(vd->ds);
-
+static void vnc_set_area_dirty(DECLARE_BITMAP(dirty[VNC_MAX_HEIGHT],
+                               VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT),
+                               int width, int height,
+                               int x, int y, int w, int h) {
     /* this is needed this to ensure we updated all affected
      * blocks if x % VNC_DIRTY_PIXELS_PER_BIT != 0 */
     w += (x % VNC_DIRTY_PIXELS_PER_BIT);
@@ -451,11 +447,22 @@  static void vnc_dpy_update(DisplayChangeListener *dcl,
     h = MIN(y + h, height);
 
     for (; y < h; y++) {
-        bitmap_set(s->dirty[y], x / VNC_DIRTY_PIXELS_PER_BIT,
+        bitmap_set(dirty[y], x / VNC_DIRTY_PIXELS_PER_BIT,
                    DIV_ROUND_UP(w, VNC_DIRTY_PIXELS_PER_BIT));
     }
 }
 
+static void vnc_dpy_update(DisplayChangeListener *dcl,
+                           int x, int y, int w, int h)
+{
+    VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
+    struct VncSurface *s = &vd->guest;
+    int width = pixman_image_get_width(vd->server);
+    int height = pixman_image_get_height(vd->server);
+
+    vnc_set_area_dirty(s->dirty, width, height, x, y, w, h);
+}
+
 void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
                             int32_t encoding)
 {
@@ -517,17 +524,15 @@  void buffer_advance(Buffer *buf, size_t len)
 
 static void vnc_desktop_resize(VncState *vs)
 {
-    DisplaySurface *ds = vs->vd->ds;
-
     if (vs->csock == -1 || !vnc_has_feature(vs, VNC_FEATURE_RESIZE)) {
         return;
     }
-    if (vs->client_width == surface_width(ds) &&
-        vs->client_height == surface_height(ds)) {
+    if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
+        vs->client_height == pixman_image_get_height(vs->vd->server)) {
         return;
     }
-    vs->client_width = surface_width(ds);
-    vs->client_height = surface_height(ds);
+    vs->client_width = pixman_image_get_width(vs->vd->server);
+    vs->client_height = pixman_image_get_height(vs->vd->server);
     vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
@@ -571,31 +576,24 @@  void *vnc_server_fb_ptr(VncDisplay *vd, int x, int y)
     ptr += x * VNC_SERVER_FB_BYTES;
     return ptr;
 }
-/* this sets only the visible pixels of a dirty bitmap */
-#define VNC_SET_VISIBLE_PIXELS_DIRTY(bitmap, w, h) {\
-        int y;\
-        memset(bitmap, 0x00, sizeof(bitmap));\
-        for (y = 0; y < h; y++) {\
-            bitmap_set(bitmap[y], 0,\
-                       DIV_ROUND_UP(w, VNC_DIRTY_PIXELS_PER_BIT));\
-        } \
-    }
 
 static void vnc_dpy_switch(DisplayChangeListener *dcl,
                            DisplaySurface *surface)
 {
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     VncState *vs;
+    int width, height;
 
     vnc_abort_display_jobs(vd);
 
     /* server surface */
     qemu_pixman_image_unref(vd->server);
     vd->ds = surface;
+    width = MIN(VNC_MAX_WIDTH, ROUND_UP(surface_width(vd->ds),
+                                        VNC_DIRTY_PIXELS_PER_BIT));
+    height = MIN(VNC_MAX_HEIGHT, surface_height(vd->ds));
     vd->server = pixman_image_create_bits(VNC_SERVER_FB_FORMAT,
-                                          surface_width(vd->ds),
-                                          surface_height(vd->ds),
-                                          NULL, 0);
+                                          width, height, NULL, 0);
 
     /* guest surface */
 #if 0 /* FIXME */
@@ -605,9 +603,9 @@  static void vnc_dpy_switch(DisplayChangeListener *dcl,
     qemu_pixman_image_unref(vd->guest.fb);
     vd->guest.fb = pixman_image_ref(surface->image);
     vd->guest.format = surface->format;
-    VNC_SET_VISIBLE_PIXELS_DIRTY(vd->guest.dirty,
-                                 surface_width(vd->ds),
-                                 surface_height(vd->ds));
+    memset(vd->guest.dirty, 0x00, sizeof(vd->guest.dirty));
+    vnc_set_area_dirty(vd->guest.dirty, width, height, 0, 0,
+                       width, height);
 
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
@@ -615,9 +613,9 @@  static void vnc_dpy_switch(DisplayChangeListener *dcl,
         if (vs->vd->cursor) {
             vnc_cursor_define(vs);
         }
-        VNC_SET_VISIBLE_PIXELS_DIRTY(vs->dirty,
-                                     surface_width(vd->ds),
-                                     surface_height(vd->ds));
+        memset(vs->dirty, 0x00, sizeof(vs->dirty));
+        vnc_set_area_dirty(vs->dirty, width, height, 0, 0,
+                           width, height);
     }
 }
 
@@ -911,8 +909,8 @@  static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
          */
         job = vnc_job_new(vs);
 
-        height = MIN(pixman_image_get_height(vd->server), vs->client_height);
-        width = MIN(pixman_image_get_width(vd->server), vs->client_width);
+        height = pixman_image_get_height(vd->server);
+        width = pixman_image_get_width(vd->server);
 
         y = 0;
         for (;;) {
@@ -1501,8 +1499,8 @@  static void check_pointer_type_change(Notifier *notifier, void *data)
         vnc_write_u8(vs, 0);
         vnc_write_u16(vs, 1);
         vnc_framebuffer_update(vs, absolute, 0,
-                               surface_width(vs->vd->ds),
-                               surface_height(vs->vd->ds),
+                               pixman_image_get_width(vs->vd->server),
+                               pixman_image_get_height(vs->vd->server),
                                VNC_ENCODING_POINTER_TYPE_CHANGE);
         vnc_unlock_output(vs);
         vnc_flush(vs);
@@ -1520,8 +1518,8 @@  static void pointer_event(VncState *vs, int button_mask, int x, int y)
         [INPUT_BUTTON_WHEEL_DOWN] = 0x10,
     };
     QemuConsole *con = vs->vd->dcl.con;
-    int width = surface_width(vs->vd->ds);
-    int height = surface_height(vs->vd->ds);
+    int width = pixman_image_get_width(vs->vd->server);
+    int height = pixman_image_get_height(vs->vd->server);
 
     if (vs->last_bmask != button_mask) {
         qemu_input_update_buttons(con, bmap, vs->last_bmask, button_mask);
@@ -1869,29 +1867,18 @@  static void ext_key_event(VncState *vs, int down,
 }
 
 static void framebuffer_update_request(VncState *vs, int incremental,
-                                       int x_position, int y_position,
-                                       int w, int h)
+                                       int x, int y, int w, int h)
 {
-    int i;
-    const size_t width = surface_width(vs->vd->ds) / VNC_DIRTY_PIXELS_PER_BIT;
-    const size_t height = surface_height(vs->vd->ds);
-
-    if (y_position > height) {
-        y_position = height;
-    }
-    if (y_position + h >= height) {
-        h = height - y_position;
-    }
+    int width = pixman_image_get_width(vs->vd->server);
+    int height = pixman_image_get_height(vs->vd->server);
 
     vs->need_update = 1;
-    if (!incremental) {
-        vs->force_update = 1;
-        for (i = 0; i < h; i++) {
-            bitmap_set(vs->dirty[y_position + i], 0, width);
-            bitmap_clear(vs->dirty[y_position + i], width,
-                         VNC_DIRTY_BITS - width);
-        }
+
+    if (incremental) {
+        return;
     }
+
+    vnc_set_area_dirty(vs->dirty, width, height, x, y, w, h);
 }
 
 static void send_ext_key_event_ack(VncState *vs)
@@ -1901,8 +1888,8 @@  static void send_ext_key_event_ack(VncState *vs)
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1);
     vnc_framebuffer_update(vs, 0, 0,
-                           surface_width(vs->vd->ds),
-                           surface_height(vs->vd->ds),
+                           pixman_image_get_width(vs->vd->server),
+                           pixman_image_get_height(vs->vd->server),
                            VNC_ENCODING_EXT_KEY_EVENT);
     vnc_unlock_output(vs);
     vnc_flush(vs);
@@ -1915,8 +1902,8 @@  static void send_ext_audio_ack(VncState *vs)
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1);
     vnc_framebuffer_update(vs, 0, 0,
-                           surface_width(vs->vd->ds),
-                           surface_height(vs->vd->ds),
+                           pixman_image_get_width(vs->vd->server),
+                           pixman_image_get_height(vs->vd->server),
                            VNC_ENCODING_AUDIO);
     vnc_unlock_output(vs);
     vnc_flush(vs);
@@ -2094,8 +2081,8 @@  static void vnc_colordepth(VncState *vs)
         vnc_write_u8(vs, 0);
         vnc_write_u16(vs, 1); /* number of rects */
         vnc_framebuffer_update(vs, 0, 0,
-                               surface_width(vs->vd->ds),
-                               surface_height(vs->vd->ds),
+                               pixman_image_get_width(vs->vd->server),
+                               pixman_image_get_height(vs->vd->server),
                                VNC_ENCODING_WMVi);
         pixel_format_message(vs);
         vnc_unlock_output(vs);
@@ -2310,8 +2297,8 @@  static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
     }
     vnc_set_share_mode(vs, mode);
 
-    vs->client_width = surface_width(vs->vd->ds);
-    vs->client_height = surface_height(vs->vd->ds);
+    vs->client_width = pixman_image_get_width(vs->vd->server);
+    vs->client_height = pixman_image_get_height(vs->vd->server);
     vnc_write_u16(vs, vs->client_width);
     vnc_write_u16(vs, vs->client_height);
 
@@ -2678,12 +2665,12 @@  static void vnc_rect_updated(VncDisplay *vd, int x, int y, struct timeval * tv)
 
 static int vnc_refresh_server_surface(VncDisplay *vd)
 {
-    int width = pixman_image_get_width(vd->guest.fb);
-    int height = pixman_image_get_height(vd->guest.fb);
-    int y;
+    int width = MIN(pixman_image_get_width(vd->guest.fb),
+                    pixman_image_get_width(vd->server));
+    int height = MIN(pixman_image_get_height(vd->guest.fb),
+                     pixman_image_get_height(vd->server));
+    int cmp_bytes, server_stride, min_stride, guest_stride, y = 0;
     uint8_t *guest_row0 = NULL, *server_row0;
-    int guest_stride = 0, server_stride;
-    int cmp_bytes;
     VncState *vs;
     int has_dirty = 0;
     pixman_image_t *tmpbuf = NULL;
@@ -2700,10 +2687,10 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
      * Check and copy modified bits from guest to server surface.
      * Update server dirty map.
      */
-    cmp_bytes = VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES;
-    if (cmp_bytes > vnc_server_fb_stride(vd)) {
-        cmp_bytes = vnc_server_fb_stride(vd);
-    }
+    server_row0 = (uint8_t *)pixman_image_get_data(vd->server);
+    server_stride = guest_stride = pixman_image_get_stride(vd->server);
+    cmp_bytes = MIN(VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES,
+                    server_stride);
     if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
         int width = pixman_image_get_width(vd->server);
         tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
@@ -2711,10 +2698,8 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
         guest_row0 = (uint8_t *)pixman_image_get_data(vd->guest.fb);
         guest_stride = pixman_image_get_stride(vd->guest.fb);
     }
-    server_row0 = (uint8_t *)pixman_image_get_data(vd->server);
-    server_stride = pixman_image_get_stride(vd->server);
+    min_stride = MIN(server_stride, guest_stride);
 
-    y = 0;
     for (;;) {
         int x;
         uint8_t *guest_ptr, *server_ptr;
@@ -2740,13 +2725,17 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
 
         for (; x < DIV_ROUND_UP(width, VNC_DIRTY_PIXELS_PER_BIT);
              x++, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) {
+            int _cmp_bytes = cmp_bytes;
             if (!test_and_clear_bit(x, vd->guest.dirty[y])) {
                 continue;
             }
-            if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) {
+            if ((x + 1) * cmp_bytes > min_stride) {
+                _cmp_bytes = min_stride - x * cmp_bytes;
+            }
+            if (memcmp(server_ptr, guest_ptr, _cmp_bytes) == 0) {
                 continue;
             }
-            memcpy(server_ptr, guest_ptr, cmp_bytes);
+            memcpy(server_ptr, guest_ptr, _cmp_bytes);
             if (!vd->non_adaptive) {
                 vnc_rect_updated(vd, x * VNC_DIRTY_PIXELS_PER_BIT,
                                  y, &tv);
diff --git a/ui/vnc.h b/ui/vnc.h
index 07af9f7..8f582fd 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -77,14 +77,15 @@  typedef void VncSendHextileTile(VncState *vs,
                                 void *last_fg,
                                 int *has_bg, int *has_fg);
 
-/* VNC_MAX_WIDTH must be a multiple of 16. */
-#define VNC_MAX_WIDTH 2560
-#define VNC_MAX_HEIGHT 2048
-
 /* VNC_DIRTY_PIXELS_PER_BIT is the number of dirty pixels represented
- * by one bit in the dirty bitmap */
+ * by one bit in the dirty bitmap, should be a power of 2 */
 #define VNC_DIRTY_PIXELS_PER_BIT 16
 
+/* VNC_MAX_WIDTH must be a multiple of VNC_DIRTY_PIXELS_PER_BIT. */
+
+#define VNC_MAX_WIDTH ROUND_UP(2560, VNC_DIRTY_PIXELS_PER_BIT)
+#define VNC_MAX_HEIGHT 2048
+
 /* VNC_DIRTY_BITS is the number of bits in the dirty bitmap. */
 #define VNC_DIRTY_BITS (VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT)
 
@@ -126,7 +127,8 @@  typedef struct VncRectStat VncRectStat;
 struct VncSurface
 {
     struct timeval last_freq_check;
-    DECLARE_BITMAP(dirty[VNC_MAX_HEIGHT], VNC_MAX_WIDTH / 16);
+    DECLARE_BITMAP(dirty[VNC_MAX_HEIGHT],
+                   VNC_MAX_WIDTH / VNC_DIRTY_PIXELS_PER_BIT);
     VncRectStat stats[VNC_STAT_ROWS][VNC_STAT_COLS];
     pixman_image_t *fb;
     pixman_format_code_t format;