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

[Paolo Bonzini]

On 10/02/2016 16:54, Laszlo Ersek wrote:
> 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.

No, I didn't.  Now I do, and you're right.

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

Gotcha.

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

It doesn't matter because width == 0 is handled properly later on; it
does not do anything, including not loading and not storing anything. So
the check is pointless anyway in that case.

But as I said: you're right and I will proceed to send v2.  Your reason
for preferring a > check makes sense.

An alternative possibility is to make max uint64_t, and ensure that all
the addends are properly sign extended.  I mention it just for the sake
of completeness. :)

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

I was interested in your reasoning, I just couldn't get it.

Paolo