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 <kra...@redhat.com> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > --- > 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.5010...@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