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 <[email protected]> >> Signed-off-by: Paolo Bonzini <[email protected]> >> --- >> 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: <[email protected]>, > 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
