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

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

[Paolo Bonzini]

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;
         }
-- 
2.5.0

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

[Laszlo Ersek]

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

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

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

[Paolo Bonzini]

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

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

[Laszlo Ersek]

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

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

[Paolo Bonzini]

On 10/02/2016 15:55, Laszlo Ersek wrote:
>> > 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.

Right, what my patch does is setting min and max both to pixels that are
drawn.

Paolo

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

[Laszlo Ersek]

On 02/10/16 16:29, Paolo Bonzini wrote:
> 
> 
> On 10/02/2016 15:55, Laszlo Ersek wrote:
>>>> 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.
> 
> Right, what my patch does is setting min and max both to pixels that are
> drawn.

Do you understand my concern with that? It's okay if you dismiss my
concern (or even better if you prove it is unfounded). But I hope you at
least understand it.

When you set "max" to the last pixel that is drawn, you are calculating
a new quantity in C that was not calculated before. By subtracting 1,
you could theoretically turn "max" into a negative number.

Have you checked and excluded this possibility, or proved why it doesn't
matter?

When I reviewed the underlying CVE fix (downstream), I spent hours on
tracking down all possibilities, with Gerd's help. With your patch, that
argument goes out the window, *for me*. I don't mind -- in particular
because it *could* be easy to prove your patch is safe --, but I can't
tell if it's going to be an easy proof without actually trying. And I'm
not going to try, now.

Changing the relop would be mathematically equivalent, and keep my
earlier argument intact.

Anyway, you don't need my personal R-b for this.

Thanks
Laszlo

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

[Paolo Bonzini]

On 10/02/2016 16:54, Laszlo Ersek wrote:
> On 02/10/16 16:29, Paolo Bonzini wrote:
>>
>>
>> On 10/02/2016 15:55, Laszlo Ersek wrote:
>>>>> 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.
>>
>> Right, what my patch does is setting min and max both to pixels that are
>> drawn.
> 
> Do you understand my concern with that? It's okay if you dismiss my
> concern (or even better if you prove it is unfounded). But I hope you at
> least understand it.

No, I didn't.  Now I do, and you're right.

> When you set "max" to the last pixel that is drawn, you are calculating
> a new quantity in C that was not calculated before. By subtracting 1,
> you could theoretically turn "max" into a negative number.

Gotcha.

> Have you checked and excluded this possibility, or proved why it doesn't
> matter?

It doesn't matter because width == 0 is handled properly later on; it
does not do anything, including not loading and not storing anything. So
the check is pointless anyway in that case.

But as I said: you're right and I will proceed to send v2.  Your reason
for preferring a > check makes sense.

An alternative possibility is to make max uint64_t, and ensure that all
the addends are properly sign extended.  I mention it just for the sake
of completeness. :)

> When I reviewed the underlying CVE fix (downstream), I spent hours on
> tracking down all possibilities, with Gerd's help. With your patch, that
> argument goes out the window, *for me*. I don't mind -- in particular
> because it *could* be easy to prove your patch is safe --, but I can't
> tell if it's going to be an easy proof without actually trying. And I'm
> not going to try, now.
> 
> Changing the relop would be mathematically equivalent, and keep my
> earlier argument intact.
> 
> Anyway, you don't need my personal R-b for this.

I was interested in your reasoning, I just couldn't get it.

Paolo