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 <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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: <address@hidden>,
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