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)

The libvirt file system storage driver determines what file to
act on by concatenating the pool location with the volume name.
If a user is able to pick names like "../../../etc/passwd", then
they can escape the bounds of the pool.  For that matter,
virStoragePoolListVolumes() doesn't descend into subdirectories,
so a user really shouldn't use a name with a slash.

Normally, only privileged users can coerce libvirt into creating
or opening existing files using the virStorageVol APIs; and such
users already have full privilege to create any domain XML (so it
is not an escalation of privilege).  But in the case of
fine-grained ACLs, it is feasible that a user can be granted
storage_vol:create but not domain:write, and it violates
assumptions if such a user can abuse libvirt to access files
outside of the storage pool.

Therefore, prevent all use of volume names that contain "/",
whether or not such a name is actually attempting to escape the
pool.

This changes things from:

$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
Vol ../../../../../../etc/haha created
$ rm /etc/haha

to:

$ virsh vol-create-as default ../../../../../../etc/haha --capacity 128
error: Failed to create vol ../../../../../../etc/haha
error: Requested operation is not valid: volume name '../../../../../../etc/haha' cannot contain '/'
Signed-off-by: NEric Blake <eblake@redhat.com>
(cherry picked from commit 034e47c3)

Well, in 8ad126e6 we tried to fix a memory corruption problem.
However, the fix was not as good as it could be. I mean, the
commit has one line more than it should. I've noticed this output
just recently:

  # ./run valgrind --leak-check=full --show-reachable=yes ./tools/virsh domblklist gentoo
  ==17019== Memcheck, a memory error detector
  ==17019== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
  ==17019== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
  ==17019== Command: /home/zippy/work/libvirt/libvirt.git/tools/.libs/virsh domblklist gentoo
  ==17019==
  Target     Source
  ------------------------------------------------
  fda        /var/lib/libvirt/images/fd.img
  vda        /var/lib/libvirt/images/gentoo.qcow2
  hdc        /home/zippy/tmp/install-amd64-minimal-20150402.iso

  ==17019== Thread 2:
  ==17019== Invalid read of size 4
  ==17019==    at 0x4EFF5B4: virObjectUnref (virobject.c:258)
  ==17019==    by 0x5038CFF: remoteClientCloseFunc (remote_driver.c:552)
  ==17019==    by 0x5069D57: virNetClientCloseLocked (virnetclient.c:685)
  ==17019==    by 0x506C848: virNetClientIncomingEvent (virnetclient.c:1852)
  ==17019==    by 0x5082136: virNetSocketEventHandle (virnetsocket.c:1913)
  ==17019==    by 0x4ECD64E: virEventPollDispatchHandles (vireventpoll.c:509)
  ==17019==    by 0x4ECDE02: virEventPollRunOnce (vireventpoll.c:658)
  ==17019==    by 0x4ECBF00: virEventRunDefaultImpl (virevent.c:308)
  ==17019==    by 0x130386: vshEventLoop (vsh.c:1864)
  ==17019==    by 0x4F1EB07: virThreadHelper (virthread.c:206)
  ==17019==    by 0xA8462D3: start_thread (in /lib64/libpthread-2.20.so)
  ==17019==    by 0xAB441FC: clone (in /lib64/libc-2.20.so)
  ==17019==  Address 0x139023f4 is 4 bytes inside a block of size 240 free'd
  ==17019==    at 0x4C2B1F0: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==17019==    by 0x4EA8949: virFree (viralloc.c:582)
  ==17019==    by 0x4EFF6D0: virObjectUnref (virobject.c:273)
  ==17019==    by 0x4FE74D6: virConnectClose (libvirt.c:1390)
  ==17019==    by 0x13342A: virshDeinit (virsh.c:406)
  ==17019==    by 0x134A37: main (virsh.c:950)

The problem is, when registering remoteClientCloseFunc(), it's
conn->closeCallback which is ref'd. But in the function itself
it's conn->closeCallback->conn what is unref'd. This is causing
imbalance in reference counting. Moreover, there's no need for
the remote driver to increase/decrease conn refcount since it's
not used anywhere. It's just merely passed to client registered
callback. And for that purpose it's correctly ref'd in
virConnectRegisterCloseCallback() and then unref'd in
virConnectUnregisterCloseCallback().
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
(cherry picked from commit e6893007)
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

This reverts commit 1ce7c1d2,
which introduced a significant semantic change to the
virDomainGetInfo() API. Additionally, the change was only
made to 2 of the 15 virt drivers.

Conflicts:
	src/qemu/qemu_driver.c
Signed-off-by: NJim Fehlig <jfehlig@suse.com>
(cherry picked from commit 60acb38a)

Future kernels will mandate the use of nosuid+nodev+noexec
flags when mounting the /proc/sys filesystem. Unconditionally
add them now since they don't harm things regardless and could
mitigate future security attacks.

(cherry picked from commit 24710414)

The cpu xml copy is incorrect: the memAccess field is not copied.
The lack of copy of this memAccess field can cause unexpected behaviour for live
migration when vhost user is used.

For example if guest has the following configuration:
....
<cpu>
<model>Westmere</model>
<topology sockets="1" cores="4" threads="1"/>
<numa>
<cell id='0' cpus='0-3' memory='2097152' memAccess='shared'/>
</numa>
</cpu>
....

The used configuration on the remote host in case of live migration is:
....
  <cpu mode='custom' match='exact'>
    <model fallback='allow'>Westmere</model>
    <topology sockets='1' cores='4' threads='1'/>
    <numa>
      <cell id='0' cpus='0-3' memory='2097152' unit='KiB'/>
    </numa>
  </cpu>
....

On the remote host the lack of memAccess info can cause unexpected error on the
qemu backend vhost user driver.

Fixes: def6b359 ("docs, conf, schema: add support for shared memory mapping")

This issue is present only for libvirt1.2.9 to libvirt1.2.12
With patch 181742d4 ("conf: Move all NUMA configuration to virDomainNuma")
present since libvirt1.2.13 the problem does not exist anymore as NUMA
information are no more in the CPU configuration.
Signed-off-by: NThibaut Collet <thibaut.collet@6wind.com>

Commit 4f25146b (v1.2.8) managed to silence Coverity, but at the
cost of a memory leak detected by valgrind:
==24129== 40 bytes in 5 blocks are definitely lost in loss record 355 of 637
==24129==    at 0x4A08B1C: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24129==    by 0x5084B8E: virReallocN (viralloc.c:245)
==24129==    by 0x514D5AA: virDomainObjListExport (domain_conf.c:22200)
==24129==    by 0x201227DB: qemuConnectListAllDomains (qemu_driver.c:18042)
==24129==    by 0x51CC1B6: virConnectListAllDomains (libvirt-domain.c:6797)
==24129==    by 0x14173D: remoteDispatchConnectListAllDomains (remote.c:1580)
==24129==    by 0x121BE1: remoteDispatchConnectListAllDomainsHelper (remote_dispatch.h:1072)

In short, every time a client calls a ListAll variant and asks
for the resulting list, but there are 0 elements to return, we
end up leaking the 1-entry array that holds the NULL terminator.

What's worse, a read-only client can access these functions in a
tight loop to cause libvirtd to eventually run out of memory; and
this can be considered a denial of service attack against more
privileged clients.  Thankfully, the leak is so small (8 bytes per
call) that you would already have some other denial of service with
any guest calling the API that frequently, so an out-of-memory
crash is unlikely enough that this did not warrant a CVE.

* daemon/remote.c (remoteDispatchConnectListAllDomains)
(remoteDispatchDomainListAllSnapshots)
(remoteDispatchDomainSnapshotListAllChildren)
(remoteDispatchConnectListAllStoragePools)
(remoteDispatchStoragePoolListAllVolumes)
(remoteDispatchConnectListAllNetworks)
(remoteDispatchConnectListAllInterfaces)
(remoteDispatchConnectListAllNodeDevices)
(remoteDispatchConnectListAllNWFilters)
(remoteDispatchConnectListAllSecrets)
(remoteDispatchNetworkGetDHCPLeases): Plug leak.
Signed-off-by: NEric Blake <eblake@redhat.com>
(cherry picked from commit 3c2ff502)

Commit e105dc98 moved some code but
didn't adjust the jump labels so that the job would be terminated.

(cherry picked from commit 0df2f040)

If 'virNumaGetHostNodeset()' fails then the error path will try to free
uninitialized pointer mem_mask. Introduced by commit af2a1f05.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>
(cherry picked from commit 77a9dc0b)
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

After virBitmapEqual is able to compare NULL bitmaps few bits of code
can be cleaned up.

(cherry picked from commit 20448c2a)
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Partial backport without the cleanup.

The qemuDomainHelperGetVcpus attempted to report an error when the
vcpupids info was NULL. Unfortunately earlier code would clamp the
value of 'maxinfo' to 0 when nvcpupids was 0, so the error reporting
would end up being skipped.

This lead to 'virsh vcpuinfo <dom>' just returning an empty list
instead of giving the user a clear error.

(cherry picked from commit 9358b63a)

If a previous commit I fixed the incorrect handling of vcpu pids
for TCG mode QEMU:

  commit b07f3d82
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Thu Dec 18 16:34:39 2014 +0000

    Don't setup fake CPU pids for old QEMU

    The code assumes that def->vcpus == nvcpupids, so when we setup
    fake CPU pids for old QEMU with nvcpupids == 1, we cause the
    later code to read off the end of the array. This has fun results
    like sche_setaffinity(0, ...) which changes libvirtd's own CPU
    affinity, or even better sched_setaffinity($RANDOM, ...) which
    changes the affinity of a random OS process.

The intent was that this would merely disable the ability to set
per-vCPU affinity. It should still have been possible to set VM
level host CPU affinity.

Unfortunately, when you set  <vcpu cpuset='0-1'>4</vcpu>, the XML
parser will internally take this & initialize an entry in the
def->cputune.vcpupin array for every VCPU. IOW this is implicitly
being treated as

  <cputune>
    <vcpupin cpuset='0-1' vcpu='0'/>
    <vcpupin cpuset='0-1' vcpu='1'/>
    <vcpupin cpuset='0-1' vcpu='2'/>
    <vcpupin cpuset='0-1' vcpu='3'/>
  </cputune>

Even more fun, the faked cputune elements are hidden from view when
querying the live XML, because their cpuset mask is the same as the
VM default cpumask.

The upshot was that it was impossible to set VM level CPU affinity.

To fix this we must update qemuProcessSetVcpuAffinities so that it
only reports a fatal error if the per-VCPU cpu mask is different
from the VM level cpu mask.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
(cherry picked from commit a103bb10)

* docs/news.html.in libvirt.spec.in: updated for the release
* po/*.po*: regenerated the po

The function may return NULL if something went wrong. In some places
in the tests we are not checking the return value rather than
accessing the pointer directly resulting in SIGSEGV.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Coverity reports that my commit af1c98e4 introduced
two memory leaks:
the cpumap if ncpus == 0 in virCgroupGetPercpuStats
and the params array in the test of the function.

After the 8-byte size header, there are two one-byte headers
and two bytes of padding before the crypt_header field.

Our QCOW1_HDR_CRYPT constant did not skip the padding.
http://git.qemu.org/?p=qemu.git;a=blob;f=block/qcow.c;h=ece22697#l41

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

Add a virDBusErrorIsUnknownMethod helper so that callers
don't need todo string comparisons themselves to detect
standard error names.

The virDBusMethodCall method has a DBusError as one of its
parameters. If the caller wants to pass a non-NULL value
for this, it immediately makes the calling code require
DBus at build time. This has led to breakage of non-DBus
builds several times. It is desirable that only the virdbus.c
file should need WITH_DBUS conditionals, so we must ideally
remove the DBusError parameter from the method.

We can't simply raise a libvirt error, since the whole point
of this parameter is to give the callers a way to check if
the error is one they want to ignore, without having the logs
polluted with an error message. So, we add a virErrorPtr
parameter which the caller can then either ignore or raise
using the new virReportErrorObject method.

This new method is distinct from virSetError in that it
ensures the logging hooks are run.
Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>

For distros that want to add versioned machine types, they will add
(downstream) machine types like "virt-foo-1.2.3".  Detect these as
MMIO too.
Signed-off-by: NRichard W.M. Jones <rjones@redhat.com>

Previous patch of this series fixed the issue with adding a new PCI bridge
when all the slots were reserved by devices with user specified addresses.
In case there are still some PCI devices waiting to get a slot reserved
by qemuAssignDevicePCISlots, this means a new bus needs to be
created along with a corresponding bridge controller. By adding an
additional check, this scenario now results in a reasonable error
instead of generating wrong qemu command line.

Commit 93c8ca tried to fix the issue with auto-adding of a PCI bridge
controller, but didn't work properly in all scenarios.

This patch provides a better fix of the issue when all slots on a PCI bus
are reserved by devices with user specified addresses and no additional
bridges need to be created.

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

In order to be able to test for fully reserved PCI buses, assignment of
PCI slots for integrated devices needs to be moved to a separate function.
This also might be a good preparation if we decide to add support for
other chipsets as well.

Move qemuDomainAssignPCIAddresses after the definition
of the static function qemuDomainValidateDevicePCISlotsQ35.

This lets us define a new static function using
qemuDomainValidateDevicePCISlots* and use it in
qemuDomainAssignPCIAddresses without a forward declaration.
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Allow reuse of the type.

The function doesn't allow to modify the array in any way, thus the
argument can be const.

@def is modified in the function indirectly although it's marked as
const.

After refactor to use the virTristateSwitch enum the comment in the
struct was not adjusted.

Apparmor must not prevent access to required helper programs. The following
helpers should be allowed to run in unconfined execution mode:

 - libvirt_parthelper
 - libvirt_iohelper

The network and nwfilter tests contained in the libvirt-TCK testkit can fail
unless access to raw network packets is granted. Without this access, the
following apparmor error can be seen while running the tests:

  apparmor="DENIED" operation="create" parent=1 profile="/usr/sbin/libvirtd"
  pid=94731 comm="libvirtd" family="packet" sock_type="raw" protocol=768

In order for apparmor to work properly in Xen environments, the following
access rights need to be allowed:

 - Allow CAP_SYS_PACCT, which is required when resetting some multi-port
   Broadcom cards by writting to the PCI config space

 - Allow CAP_IPC_LOCK, which is required to lock/unlock memory. Without
   this setting, an error 'Resource temporarily unavailable' can be seen
   while attempting to mmap memory. At the same time, the following
   apparmor message is seen:

   apparmor="DENIED" operation="capable" parent=1 profile="/usr/sbin/libvirtd"
   pid=2097 comm="libvirtd" pid=2097 comm="libvirtd" capability=14
   capname="ipc_lock"

 - Allow access to distribution specific directories:
     /usr/{lib,lib64}/xen/bin

Previously the function returned either -1 in case of an error or 0 on
success. However, we should also distinguish between a case we
successfully added a controller and a case there wasn't a need to add any
controller

As it turned out, fix of dead code 419a22 changed the affected condition
from "never true" to "always true", so better fix would be to change the
return code of virDomainMaybeAddController from 0 to 1 if
a new bridge has been added, thus distinguishing case when we didn't need to
add any controller and case we successfully added one.

The return code is changed in the next commit

My commit af1c98e4 broke the build on RHEL-6:
vircgrouptest.c: In function 'testCgroupGetPercpuStats':
vircgrouptest.c:566: error: nested extern declaration of
'_gl_verify_function2' [-Wnested-externs]

The only thing that needs checking is that the array size
is at least EXPECTED_NCPUS, to prevent access beyond the array.

We can ensure the minimum size also by specifying the array
size upfront.

Clang found possible dereference of NULL pointer which is right.
Function 'esxVI_LookupTaskInfoByTask' should find a task info. The issue
is that we could return 0 and leave 'taksInfo' pointer NULL because if
there is no match we simply end the search loop end set 'result' to 0.
Every caller count on the fact that if the return value is 0 than it's
safe to dereference 'taskInfo'. We should return 0 only in case we found
something and the '*taskInfo' is not NULL.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Clang found that we are passing variable with wrong enum type to
'xenapiCrashExitEnum2virDomainLifecycle' function. This is probably
copy-paste typo as the correct variable exists in the code, but it isn't
used.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Per-cpu stats are only shown for present CPUs in the cgroups,
but we were only parsing the largest CPU number from
/sys/devices/system/cpu/present and looking for stats even for
non-present CPUs.
This resulted in:
internal error: cpuacct parse error

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.

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

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

When using 'virsh attach-device' to hotplug an unsupported console type
into a qemu guest the attachment would succeed as the command line
formatter didn't report error in such case.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>

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

The listen address is not mandatory for <interface type='server'>
but when it's not specified, we've been formatting it as:
-netdev socket,listen=(null):5558,id=hostnet0
which failed with:
Device 'socket' could not be initialized

Omit the address completely and only format the port in the listen
attribute.

Also fix the schema to allow specifying a model.