From patchwork Mon Jun 30 07:24:49 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Lieven X-Patchwork-Id: 365494 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 831A41400EE for ; Mon, 30 Jun 2014 17:26:06 +1000 (EST) Received: from localhost ([::1]:60895 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1VyW-0006oD-G6 for incoming@patchwork.ozlabs.org; Mon, 30 Jun 2014 03:26:04 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Vxz-0006DH-GC for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:25:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1Vxt-0001oB-OI for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:25:31 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:34266 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Vxs-0001mx-Uu for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:25:25 -0400 Received: (qmail 7787 invoked by uid 89); 30 Jun 2014 07:25:23 -0000 Received: from [82.141.1.145] by client-16-kamp (envelope-from , uid 89) with qmail-scanner-2010/03/19-MF (clamdscan: 0.98.4/19142. hbedv: 8.3.20.22/7.11.157.152. spamassassin: 3.4.0. Clear:RC:1(82.141.1.145):SA:0(-1.2/5.0):. Processed in 2.318432 secs); 30 Jun 2014 07:25:23 -0000 Received: from ns.kamp-intra.net (HELO dns.kamp-intra.net) ([82.141.1.145]) by mx01.kamp.de with SMTP; 30 Jun 2014 07:25:20 -0000 X-GL_Whitelist: yes Received: from lieven-pc.kamp-intra.net (lieven-pc.kamp-intra.net [172.21.12.60]) by dns.kamp-intra.net (Postfix) with ESMTP id 8034920020; Mon, 30 Jun 2014 09:24:50 +0200 (CEST) Received: by lieven-pc.kamp-intra.net (Postfix, from userid 1000) id 785645FE2A; Mon, 30 Jun 2014 09:24:50 +0200 (CEST) From: Peter Lieven To: qemu-devel@nongnu.org Date: Mon, 30 Jun 2014 09:24:49 +0200 Message-Id: <1404113089-7154-1-git-send-email-pl@kamp.de> X-Mailer: git-send-email 1.7.9.5 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a02:248:0:51::16 Cc: Peter Lieven , kraxel@redhat.com, anthony@codemonkey.ws, xiawenc@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH] ui/vnc: fix potential memory corruption issues X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- ui/vnc.c | 149 +++++++++++++++++++++++++++++--------------------------------- ui/vnc.h | 14 +++--- 2 files changed, 77 insertions(+), 86 deletions(-) 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;