Re: [Qemu-devel] [RFC] qemu VGA endian swap low level drawing changes

[Benjamin Herrenschmidt]

On Sun, 2014-07-06 at 15:49 +1000, Benjamin Herrenschmidt wrote:
> We're basically tripping that test in cirrus_bitblt_rop_fwd_*
> 
>     if (dstpitch < 0 || srcpitch < 0) {
>         /* is 0 valid? srcpitch == 0 could be useful */
>         return;
>     }
> 
> Because when called from cirrus_bitblt_cputovideo_next() we
> always pass 0 for both pitch (we do one line at a time).

Ok, this is fun :-)

The above test was added supposedly as a fix for CVE2007-1320 in
2008, commit b2eb849d4b1fdb6f35d5c46958c7f703cf64cfef by "aurel32" (not
sure who that is).

However, looking at the description, it's a bit of a joke:

<<
    I have just noticed that patch for CVE-2007-1320 has never been applied
    to the QEMU CVS. Please find it below.
    
    | Multiple heap-based buffer overflows in the cirrus_invalidate_region
    | function in the Cirrus VGA extension in QEMU 0.8.2, as used in Xen and
    | possibly other products, might allow local users to execute arbitrary
    | code via unspecified vectors related to "attempting to mark
    | non-existent regions as dirty," aka the "bitblt" heap overflow.
>>

This is bogus for at least these reasons:

 - I don't see how adding that check inside one of the ROPs will have any
effect on the call to cirrus_invalidate_region()

 - Whoever wrote the patch obviously didn't "notice" the two substractions
to the pitches right before, since the comment about value "0" makes no sense
unless you ignore those. If you don't, then the comment is completely spurrious
as of course, the case of the pitch being 0 after susbtraction would be quite
common.

 - Whoever wrote the patch didn't notice that we do call it with 0 in the copy
from host path

 - Assuming doing the fix inside the ROP makes sense, then why fix only one
of them ? The exploit, if it exists, is still present in all the other ones...

 - Finally, assuming that bound-checking the pitch has some value to avoid
writing outside of the framebuffer allocated area, doing it inside the ROP
makes little sense, this should be done when setting up the blit.

At this point, I"m tempted to just revert that commit. What do you think Gerd ?

I've noticed a few reports over the last few years of breakage with Windows
NT that seem to stem from this.

Cheers,
Ben.