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