Re: [Qemu-devel] [PATCH] ui/vnc: fix potential memory corruption issues

[Peter Lieven <pl@kamp.de>]

On 30.06.2014 09:52, Gerd Hoffmann wrote:
> On Mo, 2014-06-30 at 09:24 +0200, Peter Lieven wrote:
>> this patch addresses 2 memory corruption issues.
>>
>> The first was actually discovered during playing
>> around with a Windows 7 vServer. During resolution
>> change in Windows 7 it happens sometimes that Windows
>> changes to an intermediate resolution where
>> server_stride % cmp_bytes != 0 (in vnc_refresh_server_surface).
>> This happens only if width % VNC_DIRTY_PIXELS_PER_BIT != 0.
>> The patch fixes the issue by clamping cmp_bytes in that case
>> and it finally makes those resolutions work correctly.
>> This can be easily tested by setting VNC_DIRTY_PIXELS_PER_BIT
>> to a bigger power of 2 value different than 16.
>>
>> The second is a theoretical issue, but is maybe exploitable
>> by the guest. If for some reason the surface size is bigger
>> than VNC_MAX_WIDTH x VNC_MAX_HEIGHT we end up in severe corruption.
>> This can be easily reproduced by playing around with VNC_MAX_WIDTH
>> and VNC_MAX_HEIGHT. This patch modifies the VNC server to only
>> track and copy the area up to the maximum possible size.
> So this basically makes vnc work correctly in case guest surface and
> server surface have different sizes, then fixes the two bugs on top of
> that.  And it obsoletes the other corruption patch send Friday.
> Correct?

Yes and yes.

Basically the server surface is adjusted to not exceed VNC_MAX_WIDTH x VNC_MAX_HEIGHT
and on top the width is rounded up to multiple of VNC_DIRTY_PIXELS_PER_BIT.

If you have a resolution whose width is not dividable by VNC_DIRTY_PIXELS_PER_BIT you get
a small black bar on the right of the screen. First I wanted to fix only that and then
I noticed that VNC_MAX_WIDTH and VNC_MAX_HEIGHT are nowhere enforced. I do not
know if this is actually exploitable by the guest.

If the surface is too big to fit the limits only the upper left area is shown.

I ran several sessions with altered values of VNC_MAX_WIDTH, VNC_MAX_HEIGHT and VNC_DIRTY_PIXELS_PER_BIT
in valgrind, but additional testing is welcome.

Peter