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

[Laszlo Ersek]

On 01/24/17 10:34, Li Qiang wrote:
> From: Li Qiang <address@hidden>
> 
> When doing bitblt copy in backward mode, minus the blt width first
> to avoid an oob access issue.
> 
> Signed-off-by: Li Qiang <address@hidden>
> ---
>  hw/display/cirrus_vga.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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). This is why "max" is calculated the way it is -- to get the
max address, just move to the right side of the same bottom line.

Which then also means, in order to get the top left corner, you just
need to subtract an integral multiple of the stride (you are already on
the left side). Since the pitch is negative here, that means adding an
integral multiple of the pitch.

Finally, for a single-line blt, the bottom line is the only line, in
which case we pitch multiplier should be (1 - 1) == 0.

I think the code is correct as-is.

Thanks
Laszlo