From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36905) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTDeq-0004D9-5w for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:09:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTDel-0008Sh-36 for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:09:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTDek-0008Sd-Th for qemu-devel@nongnu.org; Tue, 09 Feb 2016 14:08:59 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 832EC5A70 for ; Tue, 9 Feb 2016 19:08:58 +0000 (UTC) References: <1455015557-15106-3-git-send-email-pbonzini@redhat.com> From: Laszlo Ersek Message-ID: <56BA3948.4080301@redhat.com> Date: Tue, 9 Feb 2016 20:08:56 +0100 MIME-Version: 1.0 In-Reply-To: <1455015557-15106-3-git-send-email-pbonzini@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/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 IIRC I spent hours reviewing the backport of d3532a0db022 (for CVE-2014-8106). Comparing exclusive with exclusive (rather than turning "max" into inclusive) was my suggestion back then. I'm not saying the way it is written above is incorrect, just that I can't make the effort this time to see if it is correct. With the relop replacement (and the commit message update), you could get my R-b at once! :) Thanks Laszlo