Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_unsafe

[Laszlo Ersek]

On 02/10/16 16:29, Paolo Bonzini wrote:
> 
> 
> On 10/02/2016 15:55, Laszlo Ersek wrote:
>>>> 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.
> 
> Right, what my patch does is setting min and max both to pixels that are
> drawn.

Do you understand my concern with that? It's okay if you dismiss my
concern (or even better if you prove it is unfounded). But I hope you at
least understand it.

When you set "max" to the last pixel that is drawn, you are calculating
a new quantity in C that was not calculated before. By subtracting 1,
you could theoretically turn "max" into a negative number.

Have you checked and excluded this possibility, or proved why it doesn't
matter?

When I reviewed the underlying CVE fix (downstream), I spent hours on
tracking down all possibilities, with Gerd's help. With your patch, that
argument goes out the window, *for me*. I don't mind -- in particular
because it *could* be easy to prove your patch is safe --, but I can't
tell if it's going to be an easy proof without actually trying. And I'm
not going to try, now.

Changing the relop would be mathematically equivalent, and keep my
earlier argument intact.

Anyway, you don't need my personal R-b for this.

Thanks
Laszlo