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

[Laszlo Ersek <lersek@redhat.com>]

On 02/10/16 13:32, Paolo Bonzini wrote:
> 
> 
> 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.

Absolutely: if the thing you are verifying against the interval is
itself an inclusive thing, that is, a pixel or byte *drawn*. However,
exactly as you stated in the commit message, for the maximum check, what
we are comparing is the first offset *not* processed.

As I said, my suggestion doesn't change the meaning of your patch at
all, it only reformulates it.

Consider, pre-patch we have the following condition for rejection (max
check only):

  T := addr + s->cirrus_blt_width

  reject if (T >= s->vga.vram_size)        [0]

This is overprotective, because (T == s->vga.vram_size) should be
accepted. (Because, as you wrote, both T and s->vga.vram_size are
exclusive.) Therefore the right condition is:

  reject if (T > s->vga.vram_size)         [1]

This is equivalent to:

  reject if (T >= s->vga.vram_size + 1)    [2]

Which is further equivalent to

  reject if (T - 1 >= s->vga.vram_size)    [3]

Your patch encodes [3], by setting the "max" variable to (T-1), and
keeping the >= relop.

My suggestion is to preserve "max" set to T, and encode [1]: replace the
>= relop with >.

Mathematically they are equivalent, but in C, I feel [1] is safer
(without putting in the work to see that [3] is safe as well.)

Thanks
Laszlo