From 58ef246dd1849e6b18b22c52ccca0a30e6325d1d Mon Sep 17 00:00:00 2001 From: Michael Tokarev Date: Fri, 25 Jan 2013 21:23:24 +0400 Subject: [PATCH] vmware_vga: fix out of bounds and invalid rects updating 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 Signed-off-by: Blue Swirl (cherry picked from commit 8cb6bfb54e91b1a31a6ae704def595c2099efde1) Conflicts: hw/vmware_vga.c --- hw/vmware_vga.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index f5e4f44..4c54946 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -298,6 +298,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 > s->width) { fprintf(stderr, "%s: update width too large x: %d, w: %d\n", __FUNCTION__, x, w); @@ -305,6 +314,15 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s, w = s->width - 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 > s->height) { fprintf(stderr, "%s: update height too large y: %d, h: %d\n", __FUNCTION__, y, h);