From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1WX6-0000c1-Ha for qemu-devel@nongnu.org; Mon, 30 Jun 2014 04:01:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1WWx-0006do-CV for qemu-devel@nongnu.org; Mon, 30 Jun 2014 04:01:48 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:49796 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1WWx-0006dU-27 for qemu-devel@nongnu.org; Mon, 30 Jun 2014 04:01:39 -0400 Message-ID: <53B1195E.9060708@kamp.de> Date: Mon, 30 Jun 2014 10:01:34 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1404113089-7154-1-git-send-email-pl@kamp.de> <1404114779.24066.2.camel@nilsson.home.kraxel.org> In-Reply-To: <1404114779.24066.2.camel@nilsson.home.kraxel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ui/vnc: fix potential memory corruption issues List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: qemu-devel@nongnu.org, anthony@codemonkey.ws, xiawenc@linux.vnet.ibm.com 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