From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47827) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXQR-0000X5-4l for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:15:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTXQN-0007TS-TN for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:15:31 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:33880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTXQN-0007TM-M9 for qemu-devel@nongnu.org; Wed, 10 Feb 2016 11:15:27 -0500 Received: by mail-wm0-x231.google.com with SMTP id 128so33517488wmz.1 for ; Wed, 10 Feb 2016 08:15:27 -0800 (PST) Sender: Paolo Bonzini 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> <56BB5D3E.8000206@redhat.com> From: Paolo Bonzini Message-ID: <56BB621C.20801@redhat.com> Date: Wed, 10 Feb 2016 17:15:24 +0100 MIME-Version: 1.0 In-Reply-To: <56BB5D3E.8000206@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit 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: Laszlo Ersek Cc: qemu-devel@nongnu.org, Gerd Hoffmann On 10/02/2016 16:54, Laszlo Ersek wrote: > 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. No, I didn't. Now I do, and you're right. > 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. Gotcha. > Have you checked and excluded this possibility, or proved why it doesn't > matter? It doesn't matter because width == 0 is handled properly later on; it does not do anything, including not loading and not storing anything. So the check is pointless anyway in that case. But as I said: you're right and I will proceed to send v2. Your reason for preferring a > check makes sense. An alternative possibility is to make max uint64_t, and ensure that all the addends are properly sign extended. I mention it just for the sake of completeness. :) > 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. I was interested in your reasoning, I just couldn't get it. Paolo