From patchwork Fri Jan 25 17:23:24 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Tokarev X-Patchwork-Id: 215816 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A34C52C0085 for ; Sat, 26 Jan 2013 04:24:04 +1100 (EST) Received: from localhost ([::1]:34851 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tyn0U-0007K6-O4 for incoming@patchwork.ozlabs.org; Fri, 25 Jan 2013 12:24:02 -0500 Received: from eggs.gnu.org ([208.118.235.92]:44832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tyn0D-000725-6z for qemu-devel@nongnu.org; Fri, 25 Jan 2013 12:23:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tyn07-000601-3V for qemu-devel@nongnu.org; Fri, 25 Jan 2013 12:23:45 -0500 Received: from [86.62.121.231] (port=59060 helo=isrv.corpit.ru) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tyn06-0005zl-Ng for qemu-devel@nongnu.org; Fri, 25 Jan 2013 12:23:39 -0500 Received: from gandalf.tls.msk.ru (mjt.vpn.tls.msk.ru [192.168.177.99]) by isrv.corpit.ru (Postfix) with ESMTP id 237EEA1C6A; Fri, 25 Jan 2013 21:23:38 +0400 (MSK) Received: by gandalf.tls.msk.ru (Postfix, from userid 1000) id 4FA2D2DB; Fri, 25 Jan 2013 21:23:37 +0400 (MSK) From: Michael Tokarev To: aliguori@us.ibm.com Date: Fri, 25 Jan 2013 21:23:24 +0400 Message-Id: <1359134604-27516-2-git-send-email-mjt@msgid.tls.msk.ru> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1359134604-27516-1-git-send-email-mjt@msgid.tls.msk.ru> References: <1359134604-27516-1-git-send-email-mjt@msgid.tls.msk.ru> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 86.62.121.231 Cc: Marek Vasut , Serge Hallyn , Michael Tokarev , qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCH v2] vmware_vga: fix out of bounds and invalid rects updating 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 is a follow up for several attempts to fix this issue. Previous incarnations: 1. http://thread.gmane.org/gmane.linux.ubuntu.bugs.general/3156089 https://bugs.launchpad.net/bugs/918791 "qemu-kvm dies when using vmvga driver and unity in the guest" bug. Fix by Serge Hallyn: https://launchpadlibrarian.net/94916786/qemu-vmware.debdiff This fix is incomplete, since it does not check width and height for being negative. Serge weren't sure if that's the right place to fix it, maybe the fix should be up the stack somewhere. 2. http://thread.gmane.org/gmane.comp.emulators.qemu/166064 by Marek Vasut: "vmware_vga: Redraw only visible area" This one adds the (incomplete) check to vmsvga_update_rect_delayed(), the routine just queues the rect updating but does no interesting stuff. It is also incomplete in the same way as patch by Serge, but also does not touch width&height at all after adjusting x&y, which is wrong. As far as I can see, when processing guest requests, the device places them into a queue (vmsvga_update_rect_delayed()) and processes this queue in different place/time, namely, in vmsvga_update_rect(). Sometimes, vmsvga_update_rect() is called directly, without placing the request to the gueue. This is the place this patch changes, which is the last (deepest) in the stack. I'm not sure if this is the right place still, since it is possible we have some queue optimization (or may have in the future) which will be upset by negative/wrong values here, so maybe we should check for validity of input right when receiving request from the guest (and maybe even use unsigned types there). But I don't know the protocol and implementation enough to have a definitive answer. But since vmsvga_update_rect() has other sanity checks already, I'm adding the missing ones there as well. Cc'ing BALATON Zoltan and Andrzej Zaborowski who shows in `git blame' output and may know something in this area. If this patch is accepted, it should be applied to all active stable branches (at least since 1.1, maybe even before), with minor context change (ds_get_*(s->vga.ds) => s->*). I'm not Cc'ing -stable yet, will do it explicitly once the patch is accepted. BTW, these checks use fprintf(stderr) -- it should be converted to something more appropriate, since stderr will most likely disappear somewhere. Cc: Marek Vasut CC: Serge Hallyn Cc: BALATON Zoltan Cc: Andrzej Zaborowski Signed-off-by: Michael Tokarev Reviewed-by: Marek Vasut Signed-off-by: Serge Hallyn --- hw/vmware_vga.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index 62771bb..cd15ee4 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -296,6 +296,15 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s, uint8_t *src; uint8_t *dst; + if (x < 0) { + fprintf(stderr, "%s: update x was < 0 (%d)\n", __func__, x); + w += x; + x = 0; + } + if (w < 0) { + fprintf(stderr, "%s: update w was < 0 (%d)\n", __func__, w); + w = 0; + } if (x + w > ds_get_width(s->vga.ds)) { fprintf(stderr, "%s: update width too large x: %d, w: %d\n", __func__, x, w); @@ -303,6 +312,15 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s, w = ds_get_width(s->vga.ds) - x; } + if (y < 0) { + fprintf(stderr, "%s: update y was < 0 (%d)\n", __func__, y); + h += y; + y = 0; + } + if (h < 0) { + fprintf(stderr, "%s: update h was < 0 (%d)\n", __func__, h); + h = 0; + } if (y + h > ds_get_height(s->vga.ds)) { fprintf(stderr, "%s: update height too large y: %d, h: %d\n", __func__, y, h);