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

[Gerd Hoffmann]

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

That is (with the patch applied) inconsistent indeed.  We must either
subtract s->cirrus_blt_width from min (addr == right-most), or add it to
max (addr == left-most), but certainly not both.

> ... Really as I remember it from the downstream review, the pitch is
> negative (bottom-up), but the horizontal direction remains left to right.

Looking at cirrus_vga_rop.h I see:
 - cirrus_bitblt_rop_fwd_*() increment src and dst while walking the
   scanline, and
 - cirrus_bitblt_rop_bkwd_*() decrement src and dst ...

I still think x axis goes backwards too and therefore addr is the
right-most pixel.

cheers,
  Gerd