From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTX6J-0006p7-VF for qemu-devel@nongnu.org; Wed, 10 Feb 2016 10:54:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTX6H-0001LX-K3 for qemu-devel@nongnu.org; Wed, 10 Feb 2016 10:54:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTX6H-0001Kd-86 for qemu-devel@nongnu.org; Wed, 10 Feb 2016 10:54:41 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id C2C1C344EB4 for ; Wed, 10 Feb 2016 15:54:40 +0000 (UTC) References: <1455015557-15106-3-git-send-email-pbonzini@redhat.com> <56BA3948.4080301@redhat.com> <56BB2DD6.6050201@redhat.com> <56BB4F59.7090606@redhat.com> <56BB5768.7040006@redhat.com> From: Laszlo Ersek Message-ID: <56BB5D3E.8000206@redhat.com> Date: Wed, 10 Feb 2016 16:54:38 +0100 MIME-Version: 1.0 In-Reply-To: <56BB5768.7040006@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_unsafe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, Gerd Hoffmann On 02/10/16 16:29, Paolo Bonzini wrote: > > > On 10/02/2016 15:55, Laszlo Ersek wrote: >>>> Hmm, not sure why. We're comparing against the inclusive-exclusive >>>> range [0,s->vga.vram_size). The right way to check if something is >>>> within the range is >= min && < max; the right way to check if something >>>> is outside the range is < min || >= max. >> Absolutely: if the thing you are verifying against the interval is >> itself an inclusive thing, that is, a pixel or byte *drawn*. However, >> exactly as you stated in the commit message, for the maximum check, what >> we are comparing is the first offset *not* processed. > > Right, what my patch does is setting min and max both to pixels that are > drawn. Do you understand my concern with that? It's okay if you dismiss my concern (or even better if you prove it is unfounded). But I hope you at least understand it. When you set "max" to the last pixel that is drawn, you are calculating a new quantity in C that was not calculated before. By subtracting 1, you could theoretically turn "max" into a negative number. Have you checked and excluded this possibility, or proved why it doesn't matter? When I reviewed the underlying CVE fix (downstream), I spent hours on tracking down all possibilities, with Gerd's help. With your patch, that argument goes out the window, *for me*. I don't mind -- in particular because it *could* be easy to prove your patch is safe --, but I can't tell if it's going to be an easy proof without actually trying. And I'm not going to try, now. Changing the relop would be mathematically equivalent, and keep my earlier argument intact. Anyway, you don't need my personal R-b for this. Thanks Laszlo