From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57322) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTTwc-0008Te-DA for qemu-devel@nongnu.org; Wed, 10 Feb 2016 07:32:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTTwZ-0000wn-6o for qemu-devel@nongnu.org; Wed, 10 Feb 2016 07:32:30 -0500 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]:37066) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTTwZ-0000wj-07 for qemu-devel@nongnu.org; Wed, 10 Feb 2016 07:32:27 -0500 Received: by mail-wm0-x235.google.com with SMTP id g62so24666318wme.0 for ; Wed, 10 Feb 2016 04:32:26 -0800 (PST) Sender: Paolo Bonzini References: <1455015557-15106-3-git-send-email-pbonzini@redhat.com> <56BA3948.4080301@redhat.com> From: Paolo Bonzini Message-ID: <56BB2DD6.6050201@redhat.com> Date: Wed, 10 Feb 2016 13:32:22 +0100 MIME-Version: 1.0 In-Reply-To: <56BA3948.4080301@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: Laszlo Ersek Cc: qemu-devel@nongnu.org, Gerd Hoffmann On 09/02/2016 20:08, Laszlo Ersek wrote: > On 02/09/16 11:59, Paolo Bonzini wrote: >> The "max" value is being compared with >=, but addr + width points to >> the first byte that will _not_ be copied. Subtract one like it is >> already done above for the height. >> >> Cc: Gerd Hoffmann >> Signed-off-by: Paolo Bonzini >> --- >> hw/display/cirrus_vga.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c >> index b6ce1c8..e7939d2 100644 >> --- a/hw/display/cirrus_vga.c >> +++ b/hw/display/cirrus_vga.c >> @@ -275,14 +275,14 @@ static bool blit_region_is_unsafe(struct CirrusVGAState *s, >> int64_t min = addr >> + ((int64_t)s->cirrus_blt_height-1) * pitch; >> int32_t max = addr >> - + s->cirrus_blt_width; >> + + s->cirrus_blt_width-1; >> if (min < 0 || max >= s->vga.vram_size) { >> return true; >> } >> } else { >> int64_t max = addr >> + ((int64_t)s->cirrus_blt_height-1) * pitch >> - + s->cirrus_blt_width; >> + + s->cirrus_blt_width-1; >> if (max >= s->vga.vram_size) { >> return true; >> } >> > > (a) I reported this issue in an internal discussion @RH, more than a > year ago. Please refer to Message-Id: <54B7A2D7.5010404@redhat.com>, > points (2) and (5). > > (b) I think the commit message should be clearer about the fact that > this is not a security problem -- the off-by-one errs on the side of > safety (i.e., the behavior is too strict, not too lax). > > (c) The patch is mathematically correct, but I'd feel safer if, rather > than decrementing max, it would replace the > > max >= s->vga.vram_size > > comparisons with > > max > s->vga.vram_size 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. Paolo