Re: [Qemu-devel] [PATCH] cirrus: fix oob access issue

[Laszlo Ersek]

On 01/24/17 12:17, Laszlo Ersek wrote:
> On 01/24/17 11:48, Gerd Hoffmann wrote:
>>>> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
>>>> index 379910d..7ddd289 100644
>>>> --- a/hw/display/cirrus_vga.c
>>>> +++ b/hw/display/cirrus_vga.c
>>>> @@ -277,7 +277,8 @@ static bool blit_region_is_unsafe(struct 
>>>> CirrusVGAState *s,
>>>>      }
>>>>      if (pitch < 0) {
>>>>          int64_t min = addr
>>>> -            + ((int64_t)s->cirrus_blt_height-1) * pitch;
>>>> +            + ((int64_t)s->cirrus_blt_height-1) * pitch
>>>> +            - s->cirrus_blt_width;
>>>>          int32_t max = addr
>>>>              + s->cirrus_blt_width;
>>>>          if (min < 0 || max > s->vga.vram_size) {
>>>>
>>>
>>> I believe this is incorrect. In this case (AFAIR), "addr" points to the
>>> left-most pixel (= lowest address) of the bottom line (= highest
>>> address).
>>
>> If I read the code correctly it is backwards *both* x and y axis, so
>> addr is the right-most pixel of the bottom line.
> 
> What is "max" then? If "addr" is the right-most pixel of the bottom
> line, then "max" has the highest address just past the rectangle, and
> then adding anything non-negative to it makes no sense.

sorry, over-editing caused a bug. I meant

  If "addr" is the right-most pixel of the bottom line (or just one
  past that), then *it* has the highest address, and then adding
  anything non-negative to it (i.e., how we end up with "max") makes no
  sense.

I should have started with the diagram, not with the text.

Thanks
Laszlo

> 
> Adding a negative value to it (i.e., if s->cirrus_blt_width were
> negative) might make sense, for getting the leftmost pixel on the bottom
> line, but then that "max" value shouldn't be used for the final boundary
> check (against RAM size).
> 
> ... Really as I remember it from the downstream review, the pitch is
> negative (bottom-up), but the horizontal direction remains left to right.
> 
> 0-----------------------------------------------------------------> x
> |
> |   addr + (height - 1) * pitch, pitch < 0, height > 0
> |   |
> |   v
> |   *--------------------* <-- addr + (height - 1) * pitch + width
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   |                    |
> |   *--------------------*
> |   ^                    ^
> |   |                    |
> |   addr                 max = addr + width, width > 0
> |
> v
> y
> 
> 
> The rightmost column represented here is exclusive, the rest is inclusive.
> 
> Thanks
> Laszlo
>