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>

Commit id '155ca616' added the 'refreshVol' API. In an NFS root-squash
environment it was possible that if the just created volume from XML wasn't
properly created with the right uid/gid and/or mode, then the followup
refreshVol will fail to open the volume in order to get the allocation/
capacity values. This would leave the volume still on the server and
cause a libvirtd crash because 'voldef' would be in the pool list, but
the cleanup code would free it.

In an NFS root-squashed environment the 'vol-delete' command will fail to
'unlink' the target volume since it was created under a different uid:gid.

This code continues the concepts introduced in virFileOpenForked and
virDirCreate[NoFork] with respect to running the unlink command under
the uid/gid of the child. Unlike the other two, don't retry on EACCES
(that's why we're here doing this now).

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)

So, recently I was testing the LXC driver. You know, startup some
domains. But to my surprise, I was not able to start a single one:

  virsh # start --console test
  error: Reconnected to the hypervisor
  error: Failed to start domain test
  error: internal error: guest failed to start: unexpected exit status 125

So I've start digging. It turns out, that in virExec(), when I printed
out the @cmd, I got strange values: *(cmd->outfdptr) was certainly not
valid FD number: it has random value of several millions. This
obviously made prepareStdFd(childout, STDOUT_FILENO) fail (line 611).
But outfdptr is set in virCommandSetOutputFD(). The only place within
LXC driver where the function is called is in
virLXCProcessBuildControllerCmd(). If you take a closer look at the
function it looks like this:

static virCommandPtr
virLXCProcessBuildControllerCmd(virLXCDriverPtr driver,
                                ..
                                int logfd,
                                const char *pidfile)
{
    ...
    virCommandSetOutputFD(cmd, &logfd);
    virCommandSetErrorFD(cmd, &logfd);
    ...
}

Yes, you guessed it. @logfd is passed into the function by value.
However, in the function we try to get its address (an address of a
local variable) which is no longer valid once function is finished and
stack is cleaned. Therefore when cmd->outfdptr is evaluated at any
point after this function, we may get a random number, depending on
what's currently on the stack. Of course, this may work sometimes too
- it depends on the compiler how it arranges the code, when the stack
is wiped out.

In order to fix this, lets pass a pointer to @logfd instead of
figuring out (wrong) its value in a function.

The bug was introduced in e1de5521.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>
(cherry picked from commit 302146b1)

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)

When the synchronous pivot option is selected, libvirt would not update
the backing chain until the job was exitted. Some applications then
received invalid data as their job serialized first.

This patch removes polling to wait for the ABORT/PIVOT job completion
and replaces it with a condition. If a synchronous operation is
requested the update of the XML is executed in the job of the caller of
the synchronous request. Otherwise the monitor event callback uses a
separate worker to update the backing chain with a new job.

This is a regression since 1a92c719

When the ABORT job is finished synchronously you get the following call
stack:
 #0  qemuBlockJobEventProcess
 #1  qemuDomainBlockJobImpl
 #2  qemuDomainBlockJobAbort
 #3  virDomainBlockJobAbort

While previously or while using the _ASYNC flag you'd get:
 #0  qemuBlockJobEventProcess
 #1  processBlockJobEvent
 #2  qemuProcessEventHandler
 #3  virThreadPoolWorker

Later on I'll be adding a condition that will allow to synchronise a
SYNC block job abort. The approach will require this code to be called
from two different places so it has to be extracted into a helper.

Commit 1a92c719 moved code to handle block job events to a different
function that is executed in a separate thread. The caller of
processBlockJob handles locking and unlocking of @vm, so the we should
not do it in the function itself.

The block copy API takes the speed in bytes/s rather than MiB/s that was
the prior approach in virDomainBlockRebase. We correctly converted the
speed to bytes/s in the old API but we still called the common helper
virDomainBlockCopyCommon with the unadjusted variable.

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

When getting info on NUMA parameters for domain,
virCgroupGetCpusetMems() may be called. However, as of 43b67f2e
the call is guarded by check if memory controller is present.
Even though it may be not obvious instantly, NUMA parameters are
stored under cpuset controller. Therefore the check needs to look
like this:

  if (!virCgroupHasController(priv->cgroup,
                              VIR_CGROUP_CONTROLLER_CPUSET) ||
      virCgroupGetCpusetMems(priv->cgroup, &nodeset) < 0) {
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Throughout our code, the virCgroupController enum is used in two ways.
First as an index to an array of cgroup controllers:

struct virCgroup {
    char *path;

    struct virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST];
};

Second way is that when calling virCgroupNew() a bitmask of the enum
items can be passed to selectively detect only some controllers. For
instance:

int
virCgroupNewVcpu(virCgroupPtr domain,
                 int vcpuid,
                 bool create,
                 virCgroupPtr *group)
{
    ...
    controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) |
                   (1 << VIR_CGROUP_CONTROLLER_CPUACCT) |
                   (1 << VIR_CGROUP_CONTROLLER_CPUSET));

    if (virCgroupNew(-1, name, domain, controllers, group) < 0)
        goto cleanup;
}

Even though it's highly unlikely that so many new controllers will be
invented so that we would overflow when constructing the bitmask, it
doesn't hurt to check at compile time either.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

When creating new internal representation of cgroups, all passed
arguments are logged. Well, except for two: pid and pointer for
return value. Lets log them too.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

The function has no argument named @name rather than @path
instead.  The comment is, however, referring to @name while it
should have been referring to @path really.
Signed-off-by: NMichal Privoznik <mprivozn@redhat.com>

Commit id '2dbfa716' exposed virCgroupDetectMountsFromFile, but did not
add the corresponding entry in the "#else /* !VIR_CGROUP_SUPPORTED */"
section of the module.

Commit id 'ba1dfc5b' added virCgroupSetCpusetMemoryMigrate and
virCgroupGetCpusetMemoryMigrate, but did not add the corresponding
entry points into the "#else /* !VIR_CGROUP_SUPPORTED */" section

Commint 0473b45c introduced new function virNetlinkDelLink, but in
it's counterpart for non-linux platform there should be ATTRIBUTE_UNUSED
instead of ATTRIBUTE_UNSUPPORTED.
Signed-off-by: NPavel Hrdina <phrdina@redhat.com>

Blockcopy to non-file destination is not supported according the code,
but a 'goto endjob' is missed after checking the destination.

This leads to calling drive-mirror with wrong parameters.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1206406Signed-off-by: NShanzhi Yu <shyu@redhat.com>
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Current libvirt can only handle up to 1023 bytes when it
reads Linux sysfs topology/thread_siblings. This isn't enough for
Linux distributions that support a large value. This patch fixes
the problem by using VIR_ALLOC()/VIR_FREE(), instead of using a
fixed-size (1024) local char array. In the meanwhile
SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX is increased to 8192 which
should be large enough for a foreseeable future.
Signed-off-by: NWei Huang <wei@redhat.com>

If we fail to create the thread we leak the shutdown_info
structure.
Signed-off-by: NKonrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Just as it is possible to delete a bridge device with the netlink
RTM_DELLINK message, one can be created with the RTM_NEWLINK
message. Because of differences in the format of the message, it's not
as straightforward as with virNetlinkDelLink() to create a single
utility function that can be used to create any type of interface, so
the new netlink version of virNetDevBridgeCreate() does its own
construction of the netlink message and calls virNetlinkCommand()
itself.

This doesn't provide any extra functionality, just provides symmetry
with the previous commit.

NB: We *could* alter the API of virNetDevBridgeCreate() to take a MAC
address, and directly program that mac address into the bridge (by
adding an IFLA_ADDRESS attribute, as is done in
virNetDevMacVLanCreate()) rather than separately creating the "dummy
tap" (e.g. virbr0-nic) to maintain a fixed mac address on the bridge,
but the commit history of virnetdevbridge.c shows that the presence of
this dummy tap is essential in some older versions of the kernel
(between 2.6.39 and 3.1 or 3.2, possibly?) to proper operation of IPv6
DAD, and I don't want to take the chance of breaking something that I
don't have the time/setup to test (my RHEL6 box is at kernel
2.6.32-544, and the next lowest kernel I have is 3.17)

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

reported that a stray bridge device was left on the system when a
libvirt network failed to start due to an illegal iptables rule caused
by bad config. Apparently the reason this was happening was that
NetworkManager was noticing immediately when the bridge device was
created and automatically setting it IFF_UP. libvirt would then try to
setup the iptables rules, get an error back, and since libvirt had
never IFF_UPed the bridge, it didn't expect that it needed to set it
~IFF_UP before deleting it during the cleanup process. But the
ioctl(SIOCBRDELBR) ioctl will fail to delete a bridge if it is IFF_UP.

Since that bug was reported, NetworkManager has gotten a bit more
polite in this respect, but just in case something similar happens in
the future, this patch switches to using the netlink RTM_DELLINK
message to delete the bridge - unlike SIOCBRDELBR, it will delete the
requested bridge no matter what the setting of IFF_UP.

These two functions are identical, so no sense in having the
duplication. I resisted the temptation to replace calls to
virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in
case some mythical future platform has macvtap devices that aren't
managed with netlink (or in case we some day need to do more than just
tell the kernel to delete the device).

libvirt has always used the netlink RTM_DELLINK message to delete
macvtap/macvlan devices, but it can actually be used to delete other
types of network devices, such as bonds and bridges. This patch makes
virNetDevMacVLanDelete() available as a generic function so it can
intelligibly be called to delete these other types of interfaces.

While it returns info about multiple threads, the version
without the plural is easier to read.

It only deals with a single thread.

This function only frees the info for one thread.

Starting a qemu VM with a memory module that has the base address
specified results in the following error:

 error: internal error: early end of file from monitor: possible problem:
 2015-03-26T03:45:52.338891Z qemu-kvm: -device pc-dimm,node=0,memdev=memdimm0,
 id=dimm0,slot=0,base=4294967296: Property '.base' not found

The correct property name for the base address is 'addr'.
Signed-off-by: NLuyao Huang <lhuang@redhat.com>

Because of the microcode update to Haswell/Broadwell CPUs, existing
domains using these CPUs may fail to start even though they used to run
just fine. To help users solve this issue we try to suggest switching to
-noTSX variant of the CPU model:

    virsh # start cd
    error: Failed to start domain cd
    error: unsupported configuration: guest and host CPU are not
    compatible: Host CPU does not provide required features: rtm, hle;
    try using 'Haswell-noTSX' CPU model
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

QEMU 2.3 adds these new models to cover Haswell and Broadwell CPUs with
updated microcode. Luckily, they also reverted former the machine type
specific changes to existing models. And since these changes were never
released, we don't need to hack around them in libvirt.
Signed-off-by: NJiri Denemark <jdenemar@redhat.com>

Use the virStorageSourceIsEmpty helper to determine whether the drive
source is empty rather than checking for src->path. This will fix start
of VM with empty network cdrom that would not report any error.

The function that formats the string for network drives would return
error code but did not set the error message when called on storage
source with VIR_STORAGE_NET_PROTOCOL_LAST or _NONE.

Report an error in this case if it would ever be called in that way.

In some circumstances where the build tree differs from the source,
libvirt's compile will try to create the symlink for cpu_map.xml before
creating the directory $(abs_builddir)/cpu:

'src/cpu/cpu_map.xml': No such file or directory'

Do not create the symlink, it is no longer needed after
commit e562e82f
    Load CPU map from builddir when run uninstalled
Signed-off-by: NAmy Fong <amy.fong@windriver.com>
Signed-off-by: NJán Tomko <jtomko@redhat.com>

Recently we've fixed a bug where the status XML could not be parsed as
the parser used absolute path XPath queries. This test enhancement tests
all XML files used in the qemu-xml-2-xml test as a part of a status XML
snippet to see whether they are parsed correctly. The status XML-2-XML is
currently tested in 223 cases with this patch.

The current auto-indentation buffer code applies indentation only on
complete strings. To allow adding a string containing newlines and
having it properly indented this patch adds virBufferAddStr.

While this thread is cleaning up the client and connection objects:
 #2  virFileReadAll (path=0x7f28780012b0 "/proc/1319/stat", maxlen=maxlen@entry=1024, buf=buf@entry=0x7f289c60fc40) at util/virfile.c:1287
 #3  0x00007f28adbb1539 in virProcessGetStartTime (pid=<optimized out>, timestamp=timestamp@entry=0x7f289c60fc98) at util/virprocess.c:838
 #4  0x00007f28adb91981 in virIdentityGetSystem () at util/viridentity.c:151
 #5  0x00007f28ae73f17c in remoteClientFreeFunc (data=<optimized out>) at remote.c:1131
 #6  0x00007f28adcb7f33 in virNetServerClientDispose (obj=0x7f28aecad180) at rpc/virnetserverclient.c:858
 #7  0x00007f28adba8eeb in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265
 #8  0x00007f28ae74ad05 in virNetServerHandleJob (jobOpaque=<optimized out>, opaque=0x7f28aec93ff0) at rpc/virnetserver.c:205
 #9  0x00007f28adbbef4e in virThreadPoolWorker (opaque=opaque@entry=0x7f28aec88030) at util/virthreadpool.c:145

In stack frame #6 the client->identity object got unref'd, but the code
that removes the event callbacks in frame #5 did not run yet as we are
trying to obtain the system identity (frames #4, #3, #2).

In other thead:
 #0  virObjectUnref (anyobj=anyobj@entry=0x7f288c162c60) at util/virobject.c:264
        klass = 0xdeadbeef
        obj = 0x7f288c162c60
 #1  0x00007f28ae71c709 in remoteRelayDomainEventCheckACL (client=<optimized out>, conn=<optimized out>, dom=dom@entry=0x7f28aecaafc0) at remote.c:164
 #2  0x00007f28ae71fc83 in remoteRelayDomainEventTrayChange (conn=<optimized out>, dom=0x7f28aecaafc0, ... ) at remote.c:717
 #3  0x00007f28adc04e53 in virDomainEventDispatchDefaultFunc (conn=0x7f287c0009a0, event=0x7f28aecab1a0, ...) at conf/domain_event.c:1455
 #4  0x00007f28adc03831 in virObjectEventStateDispatchCallbacks (callbacks=<optimized out>, ....) at conf/object_event.c:724
 #5  virObjectEventStateQueueDispatch (callbacks=0x7f288c083730, queue=0x7fff51f90030, state=0x7f288c18da20) at conf/object_event.c:738
 #6  virObjectEventStateFlush (state=0x7f288c18da20) at conf/object_event.c:816
 #7  virObjectEventTimer (timer=<optimized out>, opaque=0x7f288c18da20) at conf/object_event.c:562
 #8  0x00007f28adb859cd in virEventPollDispatchTimeouts () at util/vireventpoll.c:459

Frame #0 is unrefing an invalid identity object while frame #2 hints
that the client is still dispatching the event.

For untrimmed backtrace see the bugzilla attachment.

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

Don't unref the old identity unless we set the new one correctly and
unref the new one on failure to set it so that we don't leak any
references or use invalid pointers.

While adding tests for status XML parsing and formatting I've noticed
that the device alias list is leaked.

==763001== 81 (48 direct, 33 indirect) bytes in 1 blocks are definitely lost in loss record 414 of 514
==763001==    at 0x4C2B8F0: calloc (vg_replace_malloc.c:623)
==763001==    by 0x6ACF70F: virAllocN (viralloc.c:191)
==763001==    by 0x447B64: qemuDomainObjPrivateXMLParse (qemu_domain.c:727)
==763001==    by 0x6B848F9: virDomainObjParseXML (domain_conf.c:15491)
==763001==    by 0x6B84CAC: virDomainObjParseNode (domain_conf.c:15608)