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.
Conflicts:
src/qemu/qemu_driver.c
(cherry picked from commit db86da5ca2109e4006c286a09b6c75bfe10676ad)
Nearly every source file does something like
#define VIR_FROM_THIS VIR_FROM_FOO
#define virFooReportErorr(code, ...) \
virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
__FUNCTION__, __LINE__, \
__VA_ARGS__)
This creates needless duplication and inconsistent error
reporting function names in each file. It is trivial to
just have virterror_internal.h provide a virReportError
macro that is equivalent
* src/util/virterror_internal.h: Define virReportError(code, ...)
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 7e94acd4fc470ea46ab94f21f7fe718d512ca3b1)
The surest way to avoid regressions is to test documented behavior :)
* tests/virstringtest.c (testStrdup): New test case.
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 504b4a8dae06330ba1735a28c316c1c68a32c471)
While reviewing proposed VIR_STRDUP conversions, I've already noticed
several places that do:
if (str && VIR_STRDUP(dest, str) < 0)
which can be simplified by allowing str to be NULL (something that
strdup() doesn't allow). Meanwhile, code that wants to ensure a
non-NULL dest regardless of the source can check for <= 0.
Also, make it part of the VIR_STRDUP contract that macro arguments
are evaluated exactly once.
* src/util/virstring.h (VIR_STRDUP, VIR_STRDUP_QUIET, VIR_STRNDUP)
(VIR_STRNDUP_QUIET): Improve contract.
* src/util/virstring.c (virStrdup, virStrndup): Change return
conventions.
* docs/hacking.html.in: Document this.
* HACKING: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
HACKING
docs/hacking.html.in
(cherry picked from commit 6b74a9f5d98e066f8dfdf5d5ccda68230b516246)
The code adaptation is not done right now, but in subsequent patches.
Hence I am not implementing syntax-check rule as it would break
compilation. Developers are strongly advised to use these new macros.
They are similar to VIR_ALLOC() logic: VIR_STRDUP(dst, src) returns zero
on success, -1 otherwise. In case you don't want to report OOM error,
use the _QUIET variant of a macro.
Conflicts:
src/libvirt_private.syms
src/util/virstring.h
(cherry picked from commit c3abb5c45988a0d7583f059974513722d82e2c2b)
Introduced by 7b87a3
When I quit the process which only register VIR_DOMAIN_EVENT_ID_REBOOT,
I got error like:
"libvirt: XML-RPC error : internal error: domain event 0 not registered".
Then I add the following code, it fixed.
Signed-off-by: Zhou Yimin <zhouyimin@huawei.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 9712c2510ec87a87578576a407768380e250a6a4)
Conflicts:
src/remote/remote_driver.c - context
The 'stats' variable was not initialized to NULL, so if some
early validation of the RPC call fails, it is possible to jump
to the 'cleanup' label and VIR_FREE an uninitialized pointer.
This is a security flaw, since the API can be called from a
readonly connection which can trigger the validation checks.
This was introduced in release v0.9.1 onwards by
commit 158ba8730e44b7dd07a21ab90499996c5dec080a
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Wed Apr 13 16:21:35 2011 +0100
Merge all returns paths from dispatcher into single path
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit e7f400a110e2e3673b96518170bfea0855dd82c0)
Conflicts:
daemon/remote.c - context
With the existing pkcheck (pid, start time) tuple for identifying
the process, there is a race condition, where a process can make
a libvirt RPC call and in another thread exec a setuid application,
causing it to change to effective UID 0. This in turn causes polkit
to do its permission check based on the wrong UID.
To address this, libvirt must get the UID the caller had at time
of connect() (from SO_PEERCRED) and pass a (pid, start time, uid)
triple to the pkcheck program.
This fix requires that libvirt is re-built against a version of
polkit that has the fix for its CVE-2013-4288, so that libvirt
can see 'pkg-config --variable pkcheck_supports_uid polkit-gobject-1'
Signed-off-by: Colin Walters <walters@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 922b7fda77b094dbf022d625238262ea05335666)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
configure.ac - context
libvirt.spec.in - context of indentation
src/access/viraccessdriverpolkit.c - not present on this branch
Since PIDs can be reused, polkit prefers to be given
a (PID,start time) pair. If given a PID on its own,
it will attempt to lookup the start time in /proc/pid/stat,
though this is subject to races.
It is safer if the client app resolves the PID start
time itself, because as long as the app has the client
socket open, the client PID won't be reused.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 979e9c56a7aadf2dcfbddd1abfbad594b78b4468)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
src/libvirt_private.syms - not backported
src/locking/lock_daemon.c - not backported
src/rpc/virnetserverclient.c
src/rpc/virnetsocket.c
src/rpc/virnetsocket.h
src/util/viridentity.h - not backported
src/util/virprocess.c
src/util/virprocess.h
src/util/virstring.c
src/util/virstring.h
Most conflicts were contextual (this patch adds new functions,
but upstream intermediate patches not backported here also added
new features, and the resolution was picking out just the portions
needed by this commit). virnetsocket.c also had slightly
different locking semantics.
Continue consolidation of process functions by moving some
helpers out of command.{c,h} into virprocess.{c,h}
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 9467ab6074d02bd90248b5710b1c83856fefe9b4)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
src/lxc/lxc_controller.c
src/util/command.c
src/util/virprocess.c
tests/testutils.c
There are a number of process related functions spread
across multiple files. Start to consolidate them by
creating a virprocess.{c,h} file
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit e5e2b65cf86ea49eba76b3c274e3b9d2177485bc)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
src/qemu/qemu_monitor.c
src/util/util.h
The virCommand prefix was inappropriate because the API
does not use any virCommandPtr object instance. This
API closely related to waitpid/exit, so use virProcess
as the prefix
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 49ecf8b41fd1c606703d01792701ce46352b7669)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
src/util/command.c
Change "Pid" to "Process" to align with the virProcessKill
API naming prefix
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 0fb58ef5cd477cf9a0efdd966a22440ef087a2af)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
src/util/util.c
src/lxc/lxc_container.c
src/lxc/lxc_controller.c
Changing naming to follow the convention of "object" followed
by "action"
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit cf470068a114fc7aab5e5de37d3f4fe3545bdc81)
This introduces a few new APIs for dealing with strings.
One to split a char * into a char **, another to join a
char ** into a char *, and finally one to free a char **
There is a simple test suite to validate the edge cases
too. No more need to use the horrible strtok_r() API,
or hand-written code for splitting strings.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 76c1fd33c8093d6a7173a85486e1e6f51a832135)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
tests/Makefile.am - several intermediate tests not backported
When given a CA cert with basic constraints to set non-critical,
and key usage of 'key signing', this should be rejected. Version
of GNUTLS < 3 do not rejecte it though, so we never noticed the
test case was broken
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 0204d6d7a0519377b2e6bc296b00328cd748f55d)
Make has a builtin operator 'undefine', and coupled with latest
automake.git, this test name ended up confusing make into thinking
the file name was meant to be used as the make operator. Renaming
the file avoids the confusion.
* tests/undefine: Rename...
* tests/virsh-undefine: ...to this.
* tests/Makefile.am (test_scripts): Use new name.
Reported by Jim Meyering.
(cherry picked from commit a20f06d9d9b0353d7fb7a8e11a631253d5961b96)
The first two hunks fix "Unterminated I<...> sequence" error and the
last one fixes "’=item’ outside of any ’=over’" error.
(cherry picked from commit 61299a1c983a64c7e0337b94232fdd2d42c1f4f2)
Signed-off-by: Eric Blake <eblake@redhat.com>
Conflicts:
tools/virsh.pod - drop hunks not present this far back
This is a second attempt at fixing the problem first attempted
in commit 2df8d99; basically undoing the fact that it was
reverted in commit 43cee32f, plus fixing two more issues: the
code in configure.ac has to EXACTLY match virnetdevbridge.c
with regards to declaring in6 types before using if_bridge.h,
and the fact that RHEL 5 has even more conflicts:
In file included from util/virnetdevbridge.c:49:
/usr/include/linux/in6.h:47: error: conflicting types for 'in6addr_any'
/usr/include/netinet/in.h:206: error: previous declaration of 'in6addr_any' was here
/usr/include/linux/in6.h:49: error: conflicting types for 'in6addr_loopback'
/usr/include/netinet/in.h:207: error: previous declaration of 'in6addr_loopback' was here
The rest of this commit message borrows from the original try
of 2df8d99:
A fresh checkout on a RHEL 6 machine with these packages:
kernel-headers-2.6.32-405.el6.x86_64
glibc-2.12-1.128.el6.x86_64
failed to configure with this message:
checking for linux/if_bridge.h... no
configure: error: You must install kernel-headers in order to compile libvirt with QEMU or LXC support
Digging in config.log, we see that the problem is identical to
what we fixed earlier in commit d12c2811:
configure:98831: checking for linux/if_bridge.h
configure:98853: gcc -std=gnu99 -c -g -O2 conftest.c >&5
In file included from /usr/include/linux/if_bridge.h:17,
from conftest.c:559:
/usr/include/linux/in6.h:31: error: redefinition of 'struct in6_addr'
/usr/include/linux/in6.h:48: error: redefinition of 'struct sockaddr_in6'
/usr/include/linux/in6.h:56: error: redefinition of 'struct ipv6_mreq'
configure:98860: $? = 1
I had not hit it earlier because I was using incremental builds,
where config.cache had shielded me from the kernel-headers breakage.
* configure.ac (if_bridge.h): Avoid conflicting type definitions.
* src/util/virnetdevbridge.c (includes): Also sanitize for RHEL 5.
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 70024dc9192038575ab5217ac35080b038e5b13e)
Done in one patch rather than a series for bisectability reasons
(as intermediate patches suffer from various problems on various
platforms), and because the intermediate patches include a revert,
but it is pointless to backport two patches where one undoes the
other. Intermediate patches include: 9a2f36e, c308a9a, 1bf661c,
d12c281, 2df8d99, 43cee32 (phew, that's a mouthful).
Conflicts:
configure.ac - skip the churn of all intermediate patches
src/util/virnetdevbridge.c - ditto
glibc 2.15 (on Fedora 17) coupled with explicit disabling of
optimization during development dies a painful death:
In file included from /usr/include/limits.h:27:0,
from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/include/limits.h:169,
from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/include/syslimits.h:7,
from /usr/lib/gcc/x86_64-redhat-linux/4.7.0/include/limits.h:34,
from util/bitmap.c:26:
/usr/include/features.h:314:4: error: #warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Werror=cpp]
cc1: all warnings being treated as errors
Work around this by only conditionally defining _FORTIFY_SOURCE,
in the case where glibc can actually use it. The trick is using
AH_VERBATIM instead of AC_DEFINE.
* m4/virt-compile-warnings.m4 (LIBVIRT_COMPILE_WARNINGS): Squelch
_FORTIFY_SOURCE when needed to avoid glibc #warnings.
(cherry picked from commit 2af63b1c349114df98c163a8401fd9cf2facdabe)
When running virDomainDestroy, we need to make sure that no other
background thread cleans up the domain while we're doing our work.
This can happen if we release the domain object while in the
middle of work, because the monitor might detect EOF in this window.
For this reason we have a 'beingDestroyed' flag to stop the monitor
from doing its normal cleanup. Unfortunately this flag was only
being used to protect qemuDomainBeginJob, and not qemuProcessKill
This left open a race condition where either libvirtd could crash,
or alternatively report bogus error messages about the domain already
having been destroyed to the caller
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 81621f3e6e45e8681cc18ae49404736a0e772a11)
Conflicts:
src/qemu/qemu_driver.c
When calling qemuProcessKill from the virDomainDestroy impl
in QEMU, do not ignore the return value. This ensures that
if QEMU fails to respond to SIGKILL, the caller will know
about the failure.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit f1b4021b38f9485c50d386af6f682ecfc8025af5)
Commit ba226d334acbc49f6751b430e0c4e00f69eef6bf tried to fix crash of
the daemon when a domain with an open console was destroyed. The fix was
wrong as it tried to remove the callback also when the stream was
aborted, where at that point the fd stream driver was already freed and
removed.
This patch clears the callbacks with a helper right before the hash is
freed, so that it doesn't interfere with other codepaths where the
stream object is freed.
(cherry picked from commit 45edefc7a7bcbec988f54331ff37fc32e4bc2718)
When a domain has a active console connection and is destroyed the
callback is called on private data that no longer exist causing a
segfault.
(cherry picked from commit ba226d334acbc49f6751b430e0c4e00f69eef6bf)
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd
during regcomp; however, reg still needed to be VIR_FREE()'d. The call
to regfree() also didn't account for possible NULL value. Reformatted
the call to be closer to usage.
(cherry picked from commit 71da3b66a8455faf8019effe3cf504a31f91f54a)
Backported to 0.9.12 with afc4631b and its revert skipped.
Signed-off-by: Luca Tettamanti <ltettamanti acunu com>
Thanks to this new option we are now able to use modern CPU models (such
as Westmere) defined in external configuration file.
The qemu-1.1{,-device} data files for qemuhelptest are filled in with
qemu-1.1-rc2 output for now. I will update those files with real
qemu-1.1 output once it is released.
(cherry picked from commit 63b4243624b8fdabebaf5e6ec912095b2b5fdf5c)
This patch resolves CVE-2013-0170:
https://bugzilla.redhat.com/show_bug.cgi?id=893450
When reading and dispatching of a message failed the message was freed
but wasn't removed from the message queue.
After that when the connection was about to be closed the pointer for
the message was still present in the queue and it was passed to
virNetMessageFree which tried to call the callback function from an
uninitialized pointer.
This patch removes the message from the queue before it's freed.
* rpc/virnetserverclient.c: virNetServerClientDispatchRead:
- avoid use after free of RPC messages
(cherry picked from commit 46532e3e8ed5f5a736a02f67d6c805492f9ca720)
While unescaping the commands the commands passed through to the monitor
function qemuMonitorUnescapeArg() initialized lenght of the input string
to strlen()+1 which is fine for alloc but not for iteration of the
string.
This patch fixes the off-by-one error and drops the pointless check for
a single trailing slash that is automaticaly handled by the default
branch of switch.
(cherry picked from commit 0f4660c8787cc41fe67f869984c0ae11d680037e)
This reverts commit b1e374a7ac56927cfe62435179bf0bba1e08b372, which was
rather bad since I failed to consider all sides of the issue. The main
things I didn't consider properly are:
- a thread which sends a non-blocking call waits for the thread with
the buck to process the call
- the code doesn't expect non-blocking calls to remain in the queue
unless they were already partially sent
Thus, the reverted patch actually breaks more than what it fixes and
clients (which may even be libvirtd during p2p migrations) will likely
end up in a deadlock.
(cherry picked from commit 63643f67abcdeaa33a0f85ea8e54da75ea9908e4)
We were being lazy - virnetlink.c was getting uint32_t as a
side-effect from glibc 2.14's <unistd.h>, but older glibc 2.11
does not provide uint32_t from <unistd.h>. In fact, POSIX states
that <unistd.h> need only provide intptr_t, not all of <stdint.h>,
so the bug really is ours. Reported by Jonathan Alescio.
* src/util/virnetlink.h: Include <stdint.h>.
(cherry picked from commit e8314e78f9c5d5ad84cfda5c61000e50d91c4a1e)
Fix for CVE-2012-4423.
When generating RPC protocol messages, it's strictly needed to have a
continuous line of numbers or RPC messages. However in case anyone
tries backporting some functionality and will skip a number, there is
a possibility to make the daemon segfault with newer virsh (version of
the library, rpc call, etc.) even unintentionally.
The problem is that the skipped numbers will get func filled with
NULLs, but there is no check whether these are set before the daemon
tries to run them. This patch very simply enhances one check and fixes
that.
(cherry picked from commit b7ff9e696063189a715802d081d55a398663c15a)
Daemon uses the following pattern when dispatching APIs with typed
parameters:
VIR_ALLOC_N(params, nparams);
virDomain*(dom, params, &nparams, flags);
virTypedParameterArrayClear(params, nparams);
In case nparams was originally set to 0, virDomain* API would fill it
with the number of typed parameters it can provide and we would use this
number (rather than zero) to clear params. Because VIR_ALLOC* returns
non-NULL pointer even if size is 0, the code would end up walking
through random memory. If we were lucky enough and the memory contained
7 (VIR_TYPED_PARAM_STRING) at the right place, we would try to free a
random pointer and crash.
Let's make sure params stays NULL when nparams is 0.
(cherry picked from commit 6039a2cb49c8af4c68460d2faf365a7e1c686c7b)
Appending an item to a list transfers ownership of that item to the
list owner. But an error can occur in between item allocation and
appending it to the list. In this case the item has to be freed
explicitly. This was not done in some special cases resulting in
possible memory leaks.
Reported by Coverity.
This patch lifts the limit of calling thread detection code only on KVM
guests. With upstream qemu the thread mappings are reported also on
non-KVM machines.
QEMU adopted the thread_id information from the kvm branch.
To remain compatible with older upstream versions of qemu the check is
attempted but the failure to detect threads (or even run the monitor
command - on older versions without SMP support) is treated non-fatal
and the code reports one vCPU with pid of the hypervisor (in same
fashion this was done on non-KVM guests).
After a cpu hotplug the qemu driver did not refresh information about
virtual processors used by qemu and their corresponding threads. This
patch forces a re-detection as is done on start of QEMU.
This ensures that correct information is reported by the
virDomainGetVcpus API and "virsh vcpuinfo".
A failure to obtain the thread<->vcpu mapping is treated non-fatal and
the mapping is not updated in a case of failure as not all versions of
QEMU report this in the info cpus command.
This patch changes a switch statement into ifs when handling live vs.
configuration modifications getting rid of redundant code in case when
both live and persistent configuration gets changed.
Ever since commit c964b6a, make was trying to find the timestamp
of '""./apibuild.py".stamp"', but only touching 'apibuild.py.stamp',
and thus always rebuilding. Reported by Daniel P. Berrange.
* docs/Makefile.am (APIBUILD, APIBUILD_STAMP): Omit bogus quotes.
when failing to attach another usb device to a domain for some reason
which has one use device attached before, the libvirtd crashed.
The crash is caused by null-pointer dereference error in invoking
usbDeviceListSteal passed in NULL value usb variable.
commit 05abd1507d66aabb6cad12eeafeb4c4d1911c585 introduces the bug.
Based on a report by Seth Vidal. Just because _you_ can use virsh
to connect to both source and destinations does not mean that libvirtd
on the source (aka _root_) can likewise connect to the destination;
this matters when setting up a peer-to-peer migration instead of a
native one.
* docs/migration.html.in: Mention that in peer-to-peer, the owner
of the source libvirtd (usually root) must be able to connect to
the destination.
Detected by valgrind. Leaks are introduced in commit 122fa379.
src/conf/storage_conf.c: fix memory leaks.
How to reproduce?
$ make && make -C tests check TESTS=storagepoolxml2xmltest
$ cd tests && valgrind -v --leak-check=full ./storagepoolxml2xmltest
actual result:
==28571== LEAK SUMMARY:
==28571== definitely lost: 40 bytes in 5 blocks
==28571== indirectly lost: 0 bytes in 0 blocks
==28571== possibly lost: 0 bytes in 0 blocks
==28571== still reachable: 1,054 bytes in 21 blocks
==28571== suppressed: 0 bytes in 0 blocks
Signed-off-by: Alex Jia <ajia@redhat.com>
No useful error was being reported when an invalid character device
target type is specified in the domainXML. E.g.
...
<console type="pty">
<source path="/dev/pts/2"/>
<target type="kvm" port="0"/>
</console>
...
resulted in
error: Failed to define domain from x.xml
error: An error occurred, but the cause is unknown
With this small patch, the error is more helpful
error: Failed to define domain from x.xml
error: XML error: unknown target type 'kvm' specified for character device
Commit 642973135c54b93242c4548ef27d591b52b0994c added three direct
references to nl_handle_* instead of using our aliases which hide
differences between libnl-3 and libnl-1.
