Instead pass around the address (aka offset into vga memory).  Calculate
the pointer in the rop_* functions, after applying the mask to the
address, to make sure the address stays within the valid range.
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1489574872-8679-1-git-send-email-kraxel@redhat.com

off_cur_end is exclusive, so off_cur_end == cirrus_addr_mask is valid.
Fix calculation to make sure to allow that, otherwise the assert added
by commit f153b563 can trigger for valid
blits.

Test case: boot windows nt 4.0
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1489579606-26020-1-git-send-email-kraxel@redhat.com

Ok, we have this beast in the cirrus code which is not used at all by
modern guests, except when you try to find security holes in qemu.  So,
add an option to disable blitter altogether.  Guests released within
the last ten years should not show any rendering issues if you turn off
blitter support.

There are no known bugs in the cirrus blitter code.  But in the past we
hoped a few times already that we've finally nailed the last issue.  So
having some easy way to mitigate in case yet another blitter issue shows
up certainly makes me sleep a bit better at night.

For completeness:  The by far better way to mitigate is to switch away
from cirrus and use stdvga instead.  Or something more modern like
virtio-vga in case your guest has support for it.
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494540-15745-1-git-send-email-kraxel@redhat.com

Quoting cirrus source code:
   Follow real hardware, cirrus card emulated has 4 MB video memory.
   Also accept 8 MB/16 MB for backward compatibility.

So just use 4MB by default.  We decided to leave that at 8MB by default
a while ago, for live migration compatibility reasons.  But we have
compat properties to handle that, so that isn't a compeling reason.

This also removes some sanity check inconsistencies in the cirrus code.
Some places check against the allocated video memory, some places check
against the 4MB physical hardware has.  Guest code can trigger asserts
because of that.
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494514-15606-1-git-send-email-kraxel@redhat.com

There is a special code path (dpy_gfx_copy) to allow graphic emulation
notify user interface code about bitblit operations carryed out by
guests.  It is supported by cirrus and vnc server.  The intended purpose
is to optimize display scrolls and just send over the scroll op instead
of a full display update.

This is rarely used these days though because modern guests simply don't
use the cirrus blitter any more.  Any linux guest using the cirrus drm
driver doesn't.  Any windows guest newer than winxp doesn't ship with a
cirrus driver any more and thus uses the cirrus as simple framebuffer.

So this code tends to bitrot and bugs can go unnoticed for a long time.
See for example commit "3e10c3ec vnc: fix qemu crash because of SIGSEGV"
which fixes a bug lingering in the code for almost a year, added by
commit "c7628bff vnc: only alloc server surface with clients connected".

Also the vnc server will throttle the frame rate in case it figures the
network can't keep up (send buffers are full).  This doesn't work with
dpy_gfx_copy, for any copy operation sent to the vnc client we have to
send all outstanding updates beforehand, otherwise the vnc client might
run the client side blit on outdated data and thereby corrupt the
display.  So this dpy_gfx_copy "optimization" might even make things
worse on slow network links.

Lets kill it once for all.

Oh, and one more reason: Turns out (after writing the patch) we have a
security bug in that code path ...

Fixes: CVE-2016-9603
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1489494419-14340-1-git-send-email-kraxel@redhat.com

check the validity of parameters in cirrus_bitblt_rop_fwd_transp_xxx
and cirrus_bitblt_rop_fwd_xxx to avoid the OOB read which causes qemu Segmentation fault.

After the fix, we will touch the assert in
cirrus_invalidate_region:
assert(off_cur_end >= off_cur);
Signed-off-by: Nfangying <fangying1@huawei.com>
Signed-off-by: Nhangaohuai <hangaohuai@huawei.com>
Message-id: 20170314063919.16200-1-hangaohuai@huawei.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

Make sure we don't leave guest_cursor pointing into nowhere.  This might
lead to (rare) live migration failures, due to target trying to restore
the cursor from the stale pointer.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1421788Reported-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1488789111-27340-1-git-send-email-kraxel@redhat.com

Don't truncate the multiplication and do a 64 bit one instead
because the result is stored in a 64 bit variable.

This fixes a similar coverity warning to commits 237a8650 and
4382fa65, in a similar way, and is the final third of the fix for
coverity CID 1167561 (hopefully!).
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Reviewed-by: NPhilippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: NMichael Walle <michael@walle.cc>
Signed-off-by: NMichael Tokarev <mjt@tls.msk.ru>

Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1487669841-13668-4-git-send-email-kraxel@redhat.com

We'll add a variant which accepts dmabufs soon.  Change
the name so we can easily disturgish the two variants.
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1487669841-13668-2-git-send-email-kraxel@redhat.com

CIRRUS_BLTMODE_MEMSYSSRC blits do NOT check blit destination
and blit width, at all.  Oops.  Fix it.

Security impact: high.

The missing blit destination check allows to write to host memory.
Basically same as CVE-2014-8106 for the other blit variants.

Cc: qemu-stable@nongnu.org
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

The member VMStateField.start is used for two things, partial data
migration for VBUFFER data (basically provide migration for a
sub-buffer) and for locating next in QTAILQ.

The implementation of the VBUFFER feature is broken when VMSTATE_ALLOC
is used. This however goes unnoticed because actually partial migration
for VBUFFER is not used at all.

Let's consolidate the usage of VMStateField.start by removing support
for partial migration for VBUFFER.
Signed-off-by: NHalil Pasic <pasic@linux.vnet.ibm.com>

Message-Id: <20170203175217.45562-1-pasic@linux.vnet.ibm.com>
Reviewed-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: NDr. David Alan Gilbert <dgilbert@redhat.com>

This reverts commit 5858dd18.

Conflicts:
	hw/display/cirrus_vga.c

Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: NLaurent Vivier <lvivier@redhat.com>
Message-id: 1486645341-5010-2-git-send-email-kraxel@redhat.com

The blit_region_is_unsafe checks don't work correctly for the
patterncopy source.  It's a fixed-sized region, which doesn't
depend on cirrus_blt_{width,height}.  So go do the check in
cirrus_bitblt_common_patterncopy instead, then tell blit_is_unsafe that
it doesn't need to verify the source.  Also handle the case where we
blit from cirrus_bitbuf correctly.

This patch replaces 5858dd18.

Security impact:  I think for the most part error on the safe side this
time, refusing blits which should have been allowed.

Only exception is placing the blit source at the end of the video ram,
so cirrus_blt_srcaddr + 256 goes beyond the end of video memory.  But
even in that case I'm not fully sure this actually allows read access to
host memory.  To trick the commit 5858dd18 security checks one has to
pick very small cirrus_blt_{width,height} values, which in turn implies
only a fraction of the blit source will actually be used.

Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: NWolfgang Bumiller <w.bumiller@proxmox.com>
Reviewed-by: NLaurent Vivier <lvivier@redhat.com>
Message-id: 1486645341-5010-1-git-send-email-kraxel@redhat.com

Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NLaurent Vivier <lvivier@redhat.com>
Reviewed-by: NPhilippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 1486561893-26470-2-git-send-email-kraxel@redhat.com

Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NLaurent Vivier <lvivier@redhat.com>
Reviewed-by: NPhilippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 1486561893-26470-1-git-send-email-kraxel@redhat.com

When the guest sends VIRTIO_GPU_CMD_RESOURCE_UNREF without detaching the
backing storage beforehand (VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING)
we'll leak memory.

This patch fixes it for 3d mode, simliar to the 2d mode fix in commit
"b8e23926 virtio-gpu: call cleanup mapping function in resource destroy".
Reported-by: N李强 <liqiang6-s@360.cn>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1485167210-4757-1-git-send-email-kraxel@redhat.com

In virtio_gpu_set_scanout function, when creating the 'rect'
its refcount is set to 2, by pixman_image_create_bits and
qemu_create_displaysurface_pixman function. This can lead
a memory leak issues. This patch avoid this issue.
Signed-off-by: NLi Qiang <liqiang6-s@360.cn>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 5884626f.5b2f6b0a.1bfff.3037@mx.google.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

When doing bitblt copy in backward mode, we should minus the
blt width first just like the adding in the forward mode. This
can avoid the oob access of the front of vga's vram.
Signed-off-by: NLi Qiang <liqiang6-s@360.cn>

{ kraxel: with backward blits (negative pitch) addr is the topmost
          address, so check it as-is against vram size ]

Cc: qemu-stable@nongnu.org
Cc: P J P <ppandit@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Fixes: d3532a0d (CVE-2014-8106)
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1485938101-26602-1-git-send-email-kraxel@redhat.com
Reviewed-by: NLaszlo Ersek <lersek@redhat.com>

Apply the cirrus_addr_mask to cirrus_blt_dstaddr and cirrus_blt_srcaddr
right after assigning them, in cirrus_bitblt_start(), instead of having
this all over the place in the cirrus code, and missing a few places.
Reported-by: NWolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1485338996-17095-1-git-send-email-kraxel@redhat.com

The rops used by cirrus_bitblt_common_patterncopy only use
the destination pitch, so the source pitch shoul allowed to
be zero and the blit with used for the range check around the
source address.
Signed-off-by: NWolfgang Bumiller <w.bumiller@proxmox.com>
Message-id: 1485272138-23249-1-git-send-email-w.bumiller@proxmox.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

cirrus_invalidate_region() calls memory_region_set_dirty()
on a per-line basis, always ranging from off_begin to
off_begin+bytesperline. With a negative pitch off_begin
marks the top most used address and thus we need to do an
initial shift backwards by a line for negative pitches of
backward blits, otherwise the first iteration covers the
line going from the start offset forwards instead of
backwards.
Additionally since the start address is inclusive, if we
shift by a full `bytesperline` we move to the first address
*not* included in the blit, so we only shift by one less
than bytesperline.
Signed-off-by: NWolfgang Bumiller <w.bumiller@proxmox.com>
Message-id: 1485352137-29367-1-git-send-email-w.bumiller@proxmox.com

[ kraxel: codestyle fixes ]
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

There are a number of unused trace events that
scripts/cleanup-trace-events.pl finds.  The "hw/vfio/pci-quirks.c"
filename was typoed and "qapi/qapi-visit-core.c" was missing the qapi/
directory prefix.
Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Message-id: 20170126171613.1399-3-stefanha@redhat.com
Signed-off-by: NStefan Hajnoczi <stefanha@redhat.com>

We are switching BUILD_BUG_ON to verify that it's parameter is a
compile-time constant, and it turns out that some gcc versions
(specifically gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609) are
not smart enough to figure it out for expressions involving local
variables. This is harmless but means that the check is ineffective for
these platforms.  To fix, replace variables with macros.
Reported-by: NPeter Maydell <peter.maydell@linaro.org>
Signed-off-by: NMichael S. Tsirkin <mst@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>

Pick a uniform chardev type name.
Signed-off-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NPaolo Bonzini <pbonzini@redhat.com>

Coverity points out that calculating src_len by multiplying
src_width by rows could overflow. This can only happen in
the implausible case of a framebuffer larger than 4GB, but
we may as well fix it, placating Coverity. (CID1005515)
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Signed-off-by: NMichael Tokarev <mjt@tls.msk.ru>

Don't truncate the multiplication and do a 64 bit one instead
because the result is stored in a 64 bit variable.

This fixes a similar coverity warning to commit 237a8650,
in a similar way, and is the other half of the fix for
coverity CID 1167561.
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Acked-by: NMichael Walle <michael@walle.cc>
Signed-off-by: NMichael Tokarev <mjt@tls.msk.ru>

Signed-off-by: NStefan Weil <sw@weilnetz.de>
Acked-by: NAlistair Francis <alistair.francis@xilinx.com>
Signed-off-by: NMichael Tokarev <mjt@tls.msk.ru>

If a migration is already in progress and somebody attempts
to add a migration blocker, this should rightly fail.

Add an errp parameter and a retcode return value to migrate_add_blocker.
Signed-off-by: NJohn Snow <jsnow@redhat.com>
Signed-off-by: NAshijeet Acharya <ashijeetacharya@gmail.com>
Message-Id: <1484566314-3987-5-git-send-email-ashijeetacharya@gmail.com>
Reviewed-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: NGreg Kurz <groug@kaod.org>
Signed-off-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
  Merged with recent 'Allow invtsc migration' change

Current migration code cannot handle some data structures such as
QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
in VMStateInfo so that customized handling is supported. put now
will return int type.
Reviewed-by: NDr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: NJianjun Duan <duanj@linux.vnet.ibm.com>
Message-Id: <1484852453-12728-2-git-send-email-duanj@linux.vnet.ibm.com>
Signed-off-by: NDr. David Alan Gilbert <dgilbert@redhat.com>

qemu can't hotplug display devices.
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Reviewed-by: NMichael S. Tsirkin <mst@redhat.com>
Message-id: 1483970138-20360-1-git-send-email-kraxel@redhat.com

Coverity points out that if we fail in the "creating resources"
loop in virtio_gpu_load() we will leak various resources (CID 1356431).
Failing a VM load is going to leave the simulation in a complete mess,
but we can tidy up to the point that a full system reset should
get us back to sanity.
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Message-id: 1483969123-14839-3-git-send-email-peter.maydell@linaro.org
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

The 'hostmem' field in VirtIOGPU is used to track the total memory
used in pixmaps so that we can impose a maximum limit on it.
However this field is neither migrated nor recalculated on
VM load, which means that after a migration it will be incorrectly
too low, which can allow the guest to use more pixmap memory
than it should. The per-resource hostmem fields are not filled
in either as we reallocate them in the load function.

Recalculate the memory used for each pixmap and the total memory
used as we reallocate the pixmaps in virtio_gpu_load().
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
Message-id: 1483969123-14839-2-git-send-email-peter.maydell@linaro.org
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

Commit 4299b90e added a check which is too broad, given that the source
pitch value is not required to be initialized for solid fill operations.
This patch refines the blit_is_unsafe() check to ignore source pitch in
that case. After applying the above commit as a security patch, we
noticed the SLES 11 SP4 guest gui failed to initialize properly.
Signed-off-by: NBruce Rogers <brogers@suse.com>
Message-id: 20170109203520.5619-1-brogers@suse.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

In virgl_cmd_get_capset function, it uses g_malloc to allocate
a response struct to the guest. As the 'resp'struct hasn't been full
initialized it will lead the 'resp->padding' field to the guest.
Use g_malloc0 to avoid this.
Signed-off-by: NLi Qiang <liqiang6-s@360.cn>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 58188cae.4a6ec20a.3d2d1.aff2@mx.google.com

[ kraxel: resolved conflict ]
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

Add a return value to the event handler.  Some I2C devices will
NAK if they have no data, so allow them to do this.  This required
the following changes:

Go through all the event handlers and change them to return int
and return 0.

Modify i2c_start_transfer to terminate the transaction on a NAK.

Modify smbus handing to not assert if a NAK occurs on a second
operation, and terminate the transaction and return -1 instead.

Add some information on semantics to I2CSlaveClass.
Signed-off-by: NCorey Minyard <cminyard@mvista.com>
Reviewed-by: NPeter Maydell <peter.maydell@linaro.org>
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>

In the resource attach backing function, everytime it will
allocate 'res->iov' thus can leading a memory leak. This
patch avoid this.
Signed-off-by: NLi Qiang <liq3ea@gmail.com>
Message-id: 1483003721-65360-1-git-send-email-liq3ea@gmail.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

If the virgl_renderer_resource_attach_iov function fails the
'res_iovs' will be leaked. Add check of the return value to
free the 'res_iovs' when failing.
Signed-off-by: NLi Qiang <liq3ea@gmail.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1482999086-59795-1-git-send-email-liq3ea@gmail.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

If the guest destroy the resource before detach banking, the 'iov'
and 'addrs' field in resource is not freed thus leading memory
leak issue. This patch avoid this.
Signed-off-by: NLi Qiang <liq3ea@gmail.com>
Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 1480386565-10077-1-git-send-email-liq3ea@gmail.com
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>

This patch makes virtio-gpu track host memory allocations for ressources
and applies a limit (configurable 256M by default).  When exceeding the
limit virtio-gpu throws VIRTIO_GPU_RESP_ERR_OUT_OF_MEMORY errors (like
it already does today when pixman image allocations fail).

This patch covers 2d mode only.  For 3d mode we have to figure how we
are going to handle this best.  qemu doesn't track resources in case
virglrenderer is used, so I guess we should extend virglrenderer to
allow setting a limit, then let qemu set the limit and catch
virgl_renderer_resource_create failures.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: 李强 <liqiang6-s@360.cn>
Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
Message-id: 1480423356-22255-1-git-send-email-kraxel@redhat.com