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

[Peter Lieven]

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