Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_unsafe

[Paolo Bonzini <pbonzini@redhat.com>]

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 <kraxel@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@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.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