[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_un
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] cirrus_vga: fix off-by-one in blit_region_is_unsafe |
Date: |
Wed, 10 Feb 2016 17:15:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 |
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