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
