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 <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
>
> 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
