CVE-2016-5008

Setting an empty graphics password is documented as a way to disable
VNC/SPICE access, but QEMU does not always behaves like that. VNC would
happily accept the empty password. Let's enforce the behavior by setting
password expiration to "now".

https://bugzilla.redhat.com/show_bug.cgi?id=1180092Signed-off-by: NJiri Denemark <jdenemar@redhat.com>
(cherry picked from commit bb848fee)
(cherry picked from commit d933f68e)

It returns NULL on failure. Checking if the negation of it
is less than zero makes no sense. (Found by coverity after moving
the code)

In another case, the return value wasn't checked at all.

(cherry picked from commit 3fe9d75a)

Conflicts:
	src/conf/domain_addr.c - no code movement from commit b2626755

Osier Yang pointed out that ever since commit 31cb030a, the
signature of qemuDomainObjEndJob was changed to return a bool.
While comparison against 0 or > 0 still gives the right results,
it looks fishy; we also had one place that was comparing < 0
which is effectively dead code.

* src/qemu/qemu_migration.c (qemuMigrationPrepareAny): Fix dead
code bug.
(qemuMigrationBegin): Use more canonical form of bool check.
* src/qemu/qemu_driver.c (qemuAutostartDomain)
(qemuDomainCreateXML, qemuDomainSuspend, qemuDomainResume)
(qemuDomainShutdownFlags, qemuDomainReboot, qemuDomainReset)
(qemuDomainDestroyFlags, qemuDomainSetMemoryFlags)
(qemuDomainSetMemoryStatsPeriod, qemuDomainInjectNMI)
(qemuDomainSendKey, qemuDomainGetInfo, qemuDomainScreenshot)
(qemuDomainSetVcpusFlags, qemuDomainGetVcpusFlags)
(qemuDomainRestoreFlags, qemuDomainGetXMLDesc)
(qemuDomainCreateWithFlags, qemuDomainAttachDeviceFlags)
(qemuDomainUpdateDeviceFlags, qemuDomainDetachDeviceFlags)
(qemuDomainBlockResize, qemuDomainBlockStats)
(qemuDomainBlockStatsFlags, qemuDomainMemoryStats)
(qemuDomainMemoryPeek, qemuDomainGetBlockInfo)
(qemuDomainAbortJob, qemuDomainMigrateSetMaxDowntime)
(qemuDomainMigrateGetCompressionCache)
(qemuDomainMigrateSetCompressionCache)
(qemuDomainMigrateSetMaxSpeed)
(qemuDomainSnapshotCreateActiveInternal)
(qemuDomainRevertToSnapshot, qemuDomainSnapshotDelete)
(qemuDomainQemuMonitorCommand, qemuDomainQemuAttach)
(qemuDomainBlockJobImpl, qemuDomainBlockCopy)
(qemuDomainBlockCommit, qemuDomainOpenGraphics)
(qemuDomainGetBlockIoTune, qemuDomainGetDiskErrors)
(qemuDomainPMSuspendForDuration, qemuDomainPMWakeup)
(qemuDomainQemuAgentCommand, qemuDomainFSTrim): Likewise.
Signed-off-by: NEric Blake <eblake@redhat.com>
(cherry picked from commit 6cd15482)

Conflicts:
	src/qemu/qemu_driver.c - some intermediate patches not backported

The ACL check didn't check the VIR_DOMAIN_XML_SECURE flag and the
appropriate permission for it. Found via code inspection while fixing
permissions for save images.

(cherry picked from commit b347c0c2)

The ACL check didn't check the VIR_DOMAIN_XML_SECURE flag and the
appropriate permission for it.

(cherry picked from commit 03c3c0c8)

Avoid leaving the domain locked on a failed ACL check in
qemuDomainMigratePerform() and qemuDomainMigrateFinish2().

Introduced in commit abf75aea (Add ACL checks into the QEMU driver).

(cherry picked from commit 2bdcd29c)

Live definition was used to look up the disk index while persistent one
was indexed leading to a crash in qemuDomainGetBlockIoTune. Use the
correct def and report a nice error.

Unfortunately it's accessible via read-only connection, though it can
only crash libvirtd in the cases where the guest is hot-plugging disks
without reflecting those changes to the persistent definition.  So
avoiding hotplug, or doing hotplug where persistent is always modified
alongside live definition, will avoid the out-of-bounds access.

Introduced in: eca96694a (v0.9.8)
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140724Reported-by: NLuyao Huang <lhuang@redhat.com>
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>

(cherry picked from commit 3e745e8f)

We have the following matrix of possible arguments handled by the logic
statement touched by this patch:
       | flags & _REUSE_EXT | !(flags & _REUSE_EXT)
-------+--------------------+----------------------
 format| (1)                | (2)
-------+--------------------+----------------------
!format| (3)                | (4)
-------+--------------------+----------------------

In cases 1 and 2 the user provided a format, in cases 3 and 4 not. The
user requests to use a pre-existing image in 1 and 3 and libvirt will
create a new image in 2 and 4.

The difference between cases 3 and 4 is that for 3 the format is probed
from the user-provided image, whereas in 4 we just use the existing disk
format.

The current code would treat cases 1,3 and 4 correctly but in case 2 the
format provided by the user would be ignored.

The particular piece of code was broken in commit 35c7701c
but since it was introduced a few commits before that it was never
released as working.

(cherry picked from commit 42619ed0)
Signed-off-by: NEric Blake <eblake@redhat.com>

Conflicts:
	src/qemu/qemu_driver.c - no refactoring of commits 7b7bf001, 4f202266

When creating a new disk mirror the new struct is stored in a separate
variable until everything went well. The removed hunk would actually
remove existing mirror information for example when the api would be run
if a mirror still exists.

(cherry picked from commit 02b364e1)

This fixes a regression introduced in commit ff5f30b6.
Signed-off-by: NEric Blake <eblake@redhat.com>

Conflicts:
	src/qemu/qemu_driver.c - no refactoring of commits 7b7bf001, 4f202266

If a domain network interface that contains a <filterref> is modified
"live" using "virsh update-device --live", libvirtd would crash. This
was because the code supporting live update of an interface's
filterref was assuming that a filterref might be added or modified,
but didn't account for removing the filterref, resulting in a null
dereference of the filter name.

Introduced with commit 258fb278, which was first in libvirt v1.0.1.

This addresses https://bugzilla.redhat.com/show_bug.cgi?id=1093301

(cherry picked from commit 0eac9d1e)

Commit 5b3492fa aimed to fix this and caught one error but exposed
another one.  When agent command is being executed and the thread
waiting for the reply is woken up by an event (e.g. EOF in case of
shutdown), the command finishes with no data (rxObject == NULL), but
no error is reported, since this might be desired by the caller
(e.g. suspend through agent).  However, in other situations, when the
data are required (e.g. getting vCPUs), we proceed to getting desired
data out of the reply, but none of the virJSON*() functions works well
with NULLs.  I chose the way of a new parameter for qemuAgentCommand()
function that specifies whether reply is required and behaves
according to that.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>
(cherry picked from commit 736e017e)

by moving qemuAgentCommand() after qemuAgentCheckError().
Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>
(cherry picked from commit e9d09fe1)

Conflicts:
        src/qemu/qemu_agent.c -- label indentation (5922d05a)
                                 comment removal (56874f01)
                                 VIR_ALLOC refactor (e987a30d)

On all the places where qemuAgentComand() was called, we did a check
for errors in the reply.  Unfortunately, some of the places called
qemuAgentCheckError() without checking for non-null reply which might
have resulted in a crash.

So this patch makes the error-checking part of qemuAgentCommand()
itself, which:

 a) makes it look better,

 b) makes the check mandatory and, most importantly,

 c) checks for the errors if and only if it is appropriate.

This actually fixes a potential crashers when qemuAgentComand()
returned 0, but reply was NULL.  Having said that, it *should* fix the
following bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1058149Signed-off-by: NMartin Kletzander <mkletzan@redhat.com>
(cherry picked from commit 5b3492fa)

The NWFilter code has as a deadlock race condition between
the virNWFilter{Define,Undefine} APIs and starting of guest
VMs due to mis-matched lock ordering.

In the virNWFilter{Define,Undefine} codepaths the lock ordering
is

  1. nwfilter driver lock
  2. virt driver lock
  3. nwfilter update lock
  4. domain object lock

In the VM guest startup paths the lock ordering is

  1. virt driver lock
  2. domain object lock
  3. nwfilter update lock

As can be seen the domain object and nwfilter update locks are
not acquired in a consistent order.

The fix used is to push the nwfilter update lock upto the top
level resulting in a lock ordering for virNWFilter{Define,Undefine}
of

  1. nwfilter driver lock
  2. nwfilter update lock
  3. virt driver lock
  4. domain object lock

and VM start using

  1. nwfilter update lock
  2. virt driver lock
  3. domain object lock

This has the effect of serializing VM startup once again, even if
no nwfilters are applied to the guest. There is also the possibility
of deadlock due to a call graph loop via virNWFilterInstantiate
and virNWFilterInstantiateFilterLate.

These two problems mean the lock must be turned into a read/write
lock instead of a plain mutex at the same time. The lock is used to
serialize changes to the "driver->nwfilters" hash, so the write lock
only needs to be held by the define/undefine methods. All other
methods can rely on a read lock which allows good concurrency.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 6e5c79a1)

Conflicts:
	src/conf/nwfilter_conf.c
          - virReportOOMError() in context of one hunk.
	src/lxc/lxc_driver.c
          - functions renamed, and lxc object locking changed, creating
            a conflict in the context.

The virConnectPtr is passed around loads of nwfilter code in
order to provide it as a parameter to the callback registered
by the virt drivers. None of the virt drivers use this param
though, so it serves no purpose.

Avoiding the need to pass a virConnectPtr means that the
nwfilterStateReload method no longer needs to open a bogus
QEMU driver connection. This addresses a race condition that
can lead to a crash on startup.

The nwfilter driver starts before the QEMU driver and registers
some callbacks with DBus to detect firewalld reload. If the
firewalld reload happens while the QEMU driver is still starting
up though, the nwfilterStateReload method will open a connection
to the partially initialized QEMU driver and cause a crash.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 999d72fb)

CVE-2013-6458

Every API that is going to begin a job should do that before fetching
data from vm->def.

(cherry picked from commit 3b564259)

Every API that is going to begin a job should do that before fetching
data from vm->def.

(cherry picked from commit ff5f30b6)

CVE-2013-6458

Every API that is going to begin a job should do that before fetching
data from vm->def.

(cherry picked from commit f93d2caa)

CVE-2013-6458

Generally, every API that is going to begin a job should do that before
fetching data from vm->def. However, qemuDomainGetBlockInfo does not
know whether it will have to start a job or not before checking vm->def.
To avoid using disk alias that might have been freed while we were
waiting for a job, we use its copy. In case the disk was removed in the
meantime, we will fail with "cannot find statistics for device '...'"
error message.

(cherry picked from commit b7992595)

CVE-2013-6458
https://bugzilla.redhat.com/show_bug.cgi?id=1043069

When virDomainDetachDeviceFlags is called concurrently to
virDomainBlockStats: libvirtd may crash because qemuDomainBlockStats
finds a disk in vm->def before getting a job on a domain and uses the
disk pointer after getting the job. However, the domain in unlocked
while waiting on a job condition and thus data behind the disk pointer
may disappear. This happens when thread 1 runs
virDomainDetachDeviceFlags and enters monitor to actually remove the
disk. Then another thread starts running virDomainBlockStats, finds the
disk in vm->def, and while it's waiting on the job condition (owned by
the first thread), the first thread finishes the disk removal. When the
second thread gets the job, the memory pointed to be the disk pointer is
already gone.

That said, every API that is going to begin a job should do that before
fetching data from vm->def.

(cherry picked from commit db86da5c)

After successful @cmd construction the memory where @keys points to is
part of @cmd. Avoid double freeing it.
(cherry picked from commit 3e8343e1)

Since the wait is done during migration (still inside
QEMU_ASYNC_JOB_MIGRATION_OUT), the code should enter the monitor as such
in order to prohibit all other jobs from interfering in the meantime.
This patch fixes bug #1009886 in which qemuDomainGetBlockInfo was
waiting on the monitor condition and after GetSpiceMigrationStatus
mangled its internal data, the daemon crashed.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1009886
(cherry picked from commit 484cc321)

https://bugzilla.redhat.com/show_bug.cgi?id=981094
The commit 0ad9025e introduce qemu flag QEMU_CAPS_DEVICE_VIDEO_PRIMARY
for using -device VGA, -device cirrus-vga, -device vmware-svga and
-device qxl-vga. In use, for -device qxl-vga, mouse doesn't display
in guest window like the desciption in above bug.
This patch try to use -device for primary video when qemu >=1.6 which
contains the bug fix patch

(cherry picked from commit e3f2686b)

https://bugzilla.redhat.com/show_bug.cgi?id=964358

A future patch wants the DAC security manager to be able to safely
get the supplemental group list for a given uid, but at the time
of a fork rather than during initialization so as to pick up on
live changes to the system's group database.  This patch adds the
framework, including the possibility of a pre-fork callback
failing.

For now, any driver that implements a prefork callback must be
robust against the possibility of being part of a security stack
where a later element in the chain fails prefork.  This means
that drivers cannot do any action that requires a call to postfork
for proper cleanup (no grabbing a mutex, for example).  If this
is too prohibitive in the future, we would have to switch to a
transactioning sequence, where each driver has (up to) 3 callbacks:
PreForkPrepare, PreForkCommit, and PreForkAbort, to either clean
up or commit changes made during prepare.

* src/security/security_driver.h (virSecurityDriverPreFork): New
callback.
* src/security/security_manager.h (virSecurityManagerPreFork):
Change signature.
* src/security/security_manager.c (virSecurityManagerPreFork):
Optionally call into driver, and allow returning failure.
* src/security/security_stack.c (virSecurityDriverStack):
Wrap the handler for the stack driver.
* src/qemu/qemu_process.c (qemuProcessStart): Adjust caller.
Signed-off-by: NEric Blake <eblake@redhat.com>
(cherry picked from commit fdb3bde3)

CVE-2013-4153

A part of the returned monitor response was freed twice and caused
crashes of the daemon when using guest agent cpu count retrieval.

 # virsh vcpucount dom --guest

Introduced in v1.0.6-48-gc6afcb05

(cherry picked from commit dfc69235)

CVE-2013-4154

If users haven't configured guest agent then qemuAgentCommand() will
dereference a NULL 'mon' pointer, which causes crash of libvirtd when
using agent based cpu (un)plug.

With the patch, when the qemu-ga service isn't running in the guest,
a expected error "error: Guest agent is not responding: Guest agent
not available for now" will be raised, and the error "error: argument
unsupported: QEMU guest agent is not configured" is raised when the
guest hasn't configured guest agent.

GDB backtrace:

 (gdb) bt
 #0  virNetServerFatalSignal (sig=11, siginfo=<value optimized out>, context=<value optimized out>) at rpc/virnetserver.c:326
 #1  <signal handler called>
 #2  qemuAgentCommand (mon=0x0, cmd=0x7f39300017b0, reply=0x7f394b090910, seconds=-2) at qemu/qemu_agent.c:975
 #3  0x00007f39429507f6 in qemuAgentGetVCPUs (mon=0x0, info=0x7f394b0909b8) at qemu/qemu_agent.c:1475
 #4  0x00007f39429d9857 in qemuDomainGetVcpusFlags (dom=<value optimized out>, flags=9) at qemu/qemu_driver.c:4849
 #5  0x00007f3957dffd8d in virDomainGetVcpusFlags (domain=0x7f39300009c0, flags=8) at libvirt.c:9843

How to reproduce?

 # To start a guest without guest agent configuration
 # then run the following cmdline

 # virsh vcpucount foobar --guest
 error: End of file while reading data: Input/output error
 error: One or more references were leaked after disconnect from the hypervisor
 error: Failed to reconnect to the hypervisor

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=984821Signed-off-by: NAlex Jia <ajia@redhat.com>
Signed-off-by: NPeter Krempa <pkrempa@redhat.com>
(cherry picked from commit 96518d43)

CVE-2013-2230

Don't overwrite the callback ID returned by
virDomainEventStateRegisterID in ret by 0.

Introduced by abf75aea.
(cherry picked from commit f38c8185)

Remove assignment of the string freed by virURIFree
to hostname, since it's not used anywhere.

Double free introduced by ddf8ad82, useless code
introduced by f03dcc5d.

https://bugzilla.redhat.com/show_bug.cgi?id=977961
(cherry picked from commit 5744d96f)

After abf75aea the compiler screams:

qemu/qemu_driver.c: In function 'qemuNodeDeviceDetachFlags':
qemu/qemu_driver.c:10693:9: error: 'domain' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     pci = virPCIDeviceNew(domain, bus, slot, function);
         ^
qemu/qemu_driver.c:10693:9: error: 'bus' may be used uninitialized in this function [-Werror=maybe-uninitialized]
qemu/qemu_driver.c:10693:9: error: 'slot' may be used uninitialized in this function [-Werror=maybe-uninitialized]
qemu/qemu_driver.c:10693:9: error: 'function' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Since the other functions qemuNodeDeviceReAttach and qemuNodeDeviceReset
looks exactly the same, I've initialized the variables there as well.
However, I am still wondering why those functions don't matter to gcc
while the first one does.
(cherry picked from commit bc09c5d3)

If qemuMonitorBlockJob returned 0, qemuDomainBlockPivot
might return 0 even if an error occured.

https://bugzilla.redhat.com/show_bug.cgi?id=977678
(cherry picked from commit c34107df)

A loop in qemuPrepareHostdevPCIDevices() intended to cycle through all
the objects on the list pcidevs was doing "while (listcount > 0)", but
nothing in the body of the loop was reducing the size of the list - it
was instead removing items from a *different* list. It has now been
safely changed to a for() loop.

(This isn't as bad as it sounds - it's only a problem in case of an
OOM error.)

qemuGetActivePciHostDeviceList() had been creating a list that
contained pointers to objects that were also on the activePciHostdevs
list. In case of an OOM error, this newly created list would be
virObjectUnref'ed, which would cause everything on the list to be
freed. But all of those objects would still be on the
activePciHostdevs list, which could have very bad consequences if that
list was ever again accessed.

The solution used here is to populate the new list with *copies* of
the objects from the original list. It turns out that on return from
qemuGetActivePciHostDeviceList(), the caller would almost immediately
go through all the device objects and "steal" them (i.e. remove the
pointer from the list but not delete it) all from either one list or
the other; we now instead just *delete* (remove from the list and
free) each device from one list or the other, so in the end we have
the same state.

I realized after the fact that it's probably better in the long run to
give this function a name that matches the name of the link used in
sysfs to hold the group (iommu_group).

I'm changing it now because I'm about to add several more functions
that deal with iommu groups.

The driver arg to virPCIDeviceDetach is no longer used (the name of the stub driver is now set in the virPCIDevice object, and virPCIDeviceDetach retrieves it from there). Remove it.

I just learned that VFIO resets PCI devices when they are assigned to
guests / returned to the host, so it is redundant for libvirt to reset
the devices. This patch inhibits calling virPCIDeviceReset to devices
that will be/were assigned using VFIO.