Jiri Denemark [Tue, 28 Jun 2016 12:39:58 +0000 (14:39 +0200)]
qemu: Let empty default VNC password work as documented
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".
Eric Blake [Wed, 9 Dec 2015 00:46:31 +0000 (17:46 -0700)]
CVE-2015-5313: storage: don't allow '/' in filesystem volume names
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.
Since commit 8eb55d782a2b9afacc7938694891cc6fad7b42a5 libxml2 removes
two slashes from the URI when there is no server part. This is fixed
with beb7281055dbf0ed4d041022a67c6c5cfd126f25, but only if the calling
application calls xmlSaveUri() on URI that xmlURIParse() parsed. And
that is not the case in virURIFormat(). virURIFormat() accepts
virURIPtr that can be created without parsing it and we do that when we
format network storage paths for gluster for example. Even though
virStorageSourceParseBackingURI() uses virURIParse(), it throws that data
structure right away.
Since we want to format URIs as URIs and not absolute URIs or opaque
URIs (see RFC 3986), we can specify that with a special hack thanks to
commit beb7281055dbf0ed4d041022a67c6c5cfd126f25, by setting port to -1.
This fixes qemuxml2argvtest test where the disk-drive-network-gluster
case was failing.
In systemd >= 218, the udev_set_log_fn method has been marked
deprecated and turned into a no-op. Nothing in the udev client
library will print to stderr by default anymore, so we can
just stop installing a logging hook for new enough udev.
Dario Faggioli [Mon, 30 Jun 2014 17:19:01 +0000 (19:19 +0200)]
libxl: don't break the build on Xen>=4.5 because of libxl_vcpu_setaffinity()
libxl interface for vcpu pinning is changing in Xen 4.5. Basically,
libxl_set_vcpuaffinity() now wants one more parameter. That is
representative of 'VCPU soft affinity', which libvirt does not use.
To mark such change, the macro LIBXL_HAVE_VCPUINFO_SOFT_AFFINITY is
defined. Use it as a gate and, if present, re-#define the calls from
the old to the new interface, to avoid breaking the build.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Cc: Jim Fehlig <jfehlig@suse.com> Cc: Ian Campbell <Ian.Campbell@citrix.com> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
(cherry picked from commit bfc72e99920215c9b004a5380ca61fe6ff81ea6b)
The only thing that will force the qemu driver to refresh it's cached
capablities info is an explict API call to GetCapabilities.
However in the case when the initial caps lookup at driver connect didn't
find a single qemu emulator to poll, the driver is effectively useless
and really can't do anything until it's populated some qemu capabilities
info.
With the above steps, the user would have to either know about the
magic refresh capabilities call, or restart libvirtd to pick up the
changes.
Instead, this patch changes things so that every time a part of th
driver requests access to capabilities info, check to see if
we've previously seen any emulators. If not, force a refresh.
In the case of 'still no emulators found', this is still very quick, so
I can't think of a downside.
When starting up a domain, the SELinux labeling is done depending on
current configuration. If the labeling fails we check for possible
causes, as not all labeling failures are fatal. For example, if the
labeled file is on NFS which lacks SELinux support, the file can still
be readable to qemu process. These cases are distinguished by the errno
code: NFS without SELinux support returns EOPNOTSUPP. However, we were
missing one scenario. In case there's a read-only disk on a read-only
NFS (and possibly any FS) and the labeling is just optional (not
explicitly requested in the XML) there's no need to make the labeling
error fatal. In other words, read-only file on read-only NFS can fail to
be labeled, but be readable at the same time.
Peter Krempa [Tue, 20 Jan 2015 16:01:01 +0000 (17:01 +0100)]
CVE-2015-0236: qemu: Check ACLs when dumping security info from snapshots
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.
Eric Blake [Sat, 1 Nov 2014 04:14:07 +0000 (22:14 -0600)]
CVE-2014-7823: dumpxml: security hole with migratable flag
Commit 28f8dfd (v1.0.0) introduced a security hole: in at least
the qemu implementation of virDomainGetXMLDesc, the use of the
flag VIR_DOMAIN_XML_MIGRATABLE (which is usable from a read-only
connection) triggers the implicit use of VIR_DOMAIN_XML_SECURE
prior to calling qemuDomainFormatXML. However, the use of
VIR_DOMAIN_XML_SECURE is supposed to be restricted to read-write
clients only. This patch treats the migratable flag as requiring
the same permissions, rather than analyzing what might break if
migratable xml no longer includes secret information.
Fortunately, the information leak is low-risk: all that is gated
by the VIR_DOMAIN_XML_SECURE flag is the VNC connection password;
but VNC passwords are already weak (FIPS forbids their use, and
on a non-FIPS machine, anyone stupid enough to trust a max-8-byte
password sent in plaintext over the network deserves what they
get). SPICE offers better security than VNC, and all other
secrets are properly protected by use of virSecret associations
rather than direct output in domain XML.
* src/remote/remote_protocol.x (REMOTE_PROC_DOMAIN_GET_XML_DESC):
Tighten rules on use of migratable flag.
* src/libvirt-domain.c (virDomainGetXMLDesc): Likewise.
node_device_udev: Try harder to get human readable vendor:product
The manufacurer and product from USB device itself are usually not particularly
useful -- they tend to be missing, or ugly (all-uppercase, padded with spaces,
etc.). Prefer what's in the usb id database and fall back to descriptors only
if the device is too new to be in database.
gnutls-3.3.0 and newer leaves 2 FDs open in order to be backwards
compatible when it comes to chrooted binaries [1]. Linking
commandhelper with gnutls then leaves these two FDs open and
commandtest fails thanks to that. This patch does not link
commandhelper with libvirt.la, but rather only the utilities making
the test pass.
Cédric Bosdonnat [Wed, 18 Dec 2013 17:33:44 +0000 (17:33 +0000)]
Fix crash in virsystemdtest with dbus 1.7.6
D-bus introduced some changes in its locking code. Overriding the init
function skips the new locking init and thus crashes later in libvirt
test. Removing the function makes the test pass again.
Pavel Hrdina [Mon, 22 Sep 2014 16:19:07 +0000 (18:19 +0200)]
domain_conf: fix domain deadlock
If you use public api virConnectListAllDomains() with second parameter
set to NULL to get only the number of domains you will lock out all
other operations with domains.
Peter Krempa [Thu, 11 Sep 2014 14:35:53 +0000 (16:35 +0200)]
CVE-2014-3633: qemu: blkiotune: Use correct definition when looking up disk
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.
in recently xen commit: 7051d5c8, there is a api changes in
libxl_domain_create_restore.
Author: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Thu Oct 10 12:23:10 2013 +0100
tools/migrate: Fix regression when migrating from older version of Xen
use the macro LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS in libxl.h
in order to make libvirt could compile with old and new xen.
the params checkpointed_stream is useful if libvirt libxl driver
support migration. for new, set it as zero.
This patch changes the macro introduced in 292d3f2d to either be
empty in the case of newer libselinux, or contain 'const' in the
case of older libselinux. The macro is then used directly in
tests/securityselinuxhelper.c.
Cédric Bosdonnat [Wed, 28 May 2014 12:44:08 +0000 (14:44 +0200)]
build: fix build with libselinux 2.3
Several function signatures changed in libselinux 2.3, now taking
a 'const char *' instead of 'security_context_t'. The latter is
defined in selinux/selinux.h as
Currently VolOpen notifies the user of a potentially non-fatal failure by
returning -2 and logging a VIR_WARN or VIR_INFO. Unfortunately most
callers treat -2 as fatal but don't actually report any message with
the error APIs.
Rename the VOL_OPEN_ERROR flag to VOL_OPEN_NOERROR. If NOERROR is specified,
we preserve the current behavior of returning -2 (there's only one caller
that wants this).
However in the default case, only return -1, and actually use the error
APIs. Fix up a couple callers as a result.
unprivileged user has no rights to move the mounts that
inherited from parent mountns. we use this feature to move
the /stateDir/domain-name.{dev, devpts} to the /dev/ and
/dev/pts directroy of container. this commit breaks libvirt lxc.
this patch changes the behavior to bind these mounts when
user namespace is enabled and move these mounts when user
namespace is disabled.
Recent discussions around naming of 'pci' vs 'pci.0' for PPC
made me go back and look at the PPC emulator in every historical
version of QEMU since 1.0. The results were worse than I imagined.
This patch adds the logic required to make libvirt work with PPC
correctly with naming variations across all versions & machine
types.
Stefan Bader [Wed, 12 Mar 2014 12:03:26 +0000 (13:03 +0100)]
libxl: Check for control_d string to decide about dom0
As soon as any guest mounts xenfs to /proc/xen, there is a capabilities
file in that directory. However it returns nothing when reading from it.
Change the test to actually check the contents of the file.
Peter Krempa [Tue, 1 Jul 2014 11:52:51 +0000 (13:52 +0200)]
qemu: copy: Accept 'format' parameter when copying to a non-existing img
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 35c7701c64508f975dfeb8
but since it was introduced a few commits before that it was never
released as working.
Eric Blake [Mon, 6 Jan 2014 21:33:08 +0000 (14:33 -0700)]
build: fix 'make check' with newer git
Newer git doesn't like the maint.mk rule 'public-submodule-commit'
run during 'make check', as inherited from our checkout of gnulib.
I tracked down that libvirt commit 8531301 picked up a gnulib fix
that makes git happy. Rather than try and do a full .gnulib
submodule update to gnulib.git d18d1b802 (as used in that libvirt
commit), it was easier to just backport the fixed maint.mk from
gnulib on top of our existing submodule level. I did it as follows,
where these steps will have to be repeated when cherry-picking this
commit to any other maintenance branch:
mkdir -p gnulib/local/top
cd .gnulib
git checkout d18d1b802 top/maint.mk
git diff HEAD > ../gnulib/local/top/maint.mk.diff
git reset --hard
cd ..
git add gnulib/local/top
Eric Blake [Wed, 25 Jun 2014 20:54:36 +0000 (14:54 -0600)]
docs: publish correct enum values
We publish libvirt-api.xml for others to use, and in fact, the
libvirt-python bindings use it to generate python constants that
correspond to our enum values. However, we had an off-by-one bug
that any enum that relied on C's rules for implicit initialization
of the first enum member to 0 got listed in the xml as having a
value of 1 (and all later members of the enum were equally
botched).
The fix is simple - since we add one to the previous value when
encountering an enum without an initializer, the previous value
must start at -1 so that the first enum member is assigned 0.
The python generator code has had the off-by-one ever since DV
first wrote it years ago, but most of our public enums were immune
because they had an explicit = 0 initializer. The only affected
enums are:
- virDomainEventGraphicsAddressType (such as
VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_IPV4), since commit 987e31e
(libvirt v0.8.0)
- virDomainCoreDumpFormat (such as VIR_DOMAIN_CORE_DUMP_FORMAT_RAW),
since commit 9fbaff0 (libvirt v1.2.3)
- virIPAddrType (such as VIR_IP_ADDR_TYPE_IPV4), since commit 03e0e79 (not yet released)
Thanks to Nehal J Wani for reporting the problem on IRC, and
for helping me zero in on the culprit function.
Peter Krempa [Wed, 25 Jun 2014 16:11:17 +0000 (18:11 +0200)]
qemu: blockcopy: Don't remove existing disk mirror info
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.
LSN-2014-0003: Don't expand entities when parsing XML
If the XML_PARSE_NOENT flag is passed to libxml2, then any
entities in the input document will be fully expanded. This
allows the user to read arbitrary files on the host machine
by creating an entity pointing to a local file. Removing
the XML_PARSE_NOENT flag means that any entities are left
unchanged by the parser, or expanded to "" by the XPath
APIs.
Jim Fehlig [Mon, 17 Mar 2014 20:22:44 +0000 (14:22 -0600)]
libxl: fix framebuffer port setting for HVM domains
libxl uses the libxl_vnc_info and libxl_sdl_info fields from the
hvm union in libxl_domain_build_info struct when generating QEMU
args for VNC or SDL. These fields were left unset by the libxl
driver, causing libxl to ignore any user settings. E.g. with
<graphics type='vnc' port='5950'/>
port would be ignored and QEMU would instead be invoked with
-vnc 127.0.0.1:0,to=99
Unlike the libxl_domain_config struct, the libxl_domain_build_info
contains only a single libxl_vnc_info and libxl_sdl_info, so
populate these fields from the first vfb in
libxl_domain_config->vfbs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
(cherry picked from commit b55cc5f4e31b488c4f9c3c8470c992c1f8f5d09c)
Whenever we check for ABI stability, we have new xml (e.g. provided by
user, or obtained from snapshot, whatever) which we compare to old xml
and see if ABI won't break. However, if the new xml was produced via
virDomainGetXMLDesc(..., VIR_DOMAIN_XML_MIGRATABLE) it lacks some
devices, e.g. 'pci-root' controller. Hence, the ABI stability check
fails even though it is stable. Moreover, we can't simply fix
virDomainDefCheckABIStability because removing the correct devices is
task for the driver. For instance, qemu driver wants to remove the usb
controller too, while LXC driver doesn't. That's why we need special
qemu wrapper over virDomainDefCheckABIStability which removes the
correct devices from domain XML, produces MIGRATABLE xml and calls the
check ABI stability function.
Laine Stump [Mon, 7 Apr 2014 12:32:00 +0000 (15:32 +0300)]
interface: dump inactive xml when interface isn't active
Other drivers in libvirt (e.g. network, qemu) will automatically
return the "inactive" (persistent configuration) XML of an object when
that object is inactive. The netcf backend of the interface driver
would always try to return the live status XML of the interface, even
when it was down. Although netcf does return valid XML in that case,
for bond interfaces it is missing almost all of its content, including
the <bond> subelement itself, leading to this error message from
"virsh iface-dumpxml" of a bond interface that is inactive:
error: XML error: bond interface misses the bond element
(this is because libvirt's validation of the XML returned by netcf
always requires a <bond> element be present).
This patch modifies the interface driver netcf backend to check if the
interface is inactive, and in that case always return the inactive XML
(which will always have a <bond> element, thus eliminating the error
message, as well as making operation more in line with other drivers.
Ján Tomko [Fri, 21 Feb 2014 09:22:22 +0000 (10:22 +0100)]
Ignore additional fields in iscsiadm output
There has been a new field introduced in iscsiadm --mode session
output [1], but our regex only expects four fields. This breaks
startup of iscsi pools:
error: Failed to start pool iscsi
error: internal error: cannot find session
Fix this by ignoring anything after the fourth field.
Laine Stump [Thu, 1 May 2014 08:40:41 +0000 (11:40 +0300)]
qemu: fix crash when removing <filterref> from interface with update-device
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
The systemd journal expects log record PRIORITY values to
be encoded using the syslog compatible numbering scheme,
not libvirt's own native numbering scheme. We must therefore
apply a conversion.
qemu: make sure agent returns error when required data are missing
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.
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:
Ján Tomko [Thu, 20 Mar 2014 15:42:52 +0000 (16:42 +0100)]
Ignore char devices in storage pools by default
Without this, using /dev/mapper as a directory pool
fails in virStorageBackendUpdateVolTargetInfoFD:
cannot seek to end of file '/dev/mapper/control': Illegal seek
Ján Tomko [Thu, 20 Mar 2014 15:35:00 +0000 (16:35 +0100)]
Ignore missing files on pool refresh
If we cannot stat/open a file on pool refresh, returning -1 aborts
the refresh and the pool is undefined.
Only treat missing files as fatal unless VolOpenCheckMode is called
with the VIR_STORAGE_VOL_OPEN_ERROR flag. If this flag is missing
(when it's called from virStorageBackendProbeTarget in
virStorageBackendFileSystemRefresh), only emit a warning and return
-2 to let the caller skip over the file.
Eric Blake [Tue, 5 Nov 2013 15:30:01 +0000 (08:30 -0700)]
storage: reduce number of stat calls
We are calling fstat() at least twice per storage volume in
a directory storage pool; this is rather wasteful. Refactoring
this is also a step towards making code reusable for gluster,
where gluster can provide struct stat but cannot use fstat().
Ján Tomko [Thu, 19 Dec 2013 14:00:43 +0000 (15:00 +0100)]
Fix explicit usage of default video PCI slots
Do not leave the PCI address of the primary video card set
to the legacy default (0000:00:02.0) if we're doing two-pass
allocation.
Since QEMU 1.6 (QEMU_CAPS_VIDEO_PRIMARY) we allow the primary
video card to be on other slots than 0000:00:02.0 (as we use
-device instead of -vga).
However we fail to assign it an address if:
* another device explicitly uses 0000:00:02.0 and
* the primary video device has no address specified
On the first pass, we have set the address to default, then checked
if it's available, leaving it set even if it wasn't. This address
got picked up by the second pass, resulting in a conflict:
XML error: Attempted double use of PCI slot 0000:00:02.0
(may need "multifunction='on'" for device on function 0)
Also fix the test that was supposed to catch this.
Michal Privoznik [Wed, 19 Mar 2014 17:10:34 +0000 (18:10 +0100)]
virNetClientSetTLSSession: Restore original signal mask
Currently, we use pthread_sigmask(SIG_BLOCK, ...) prior to calling
poll(). This is okay, as we don't want poll() to be interrupted.
However, then - immediately as we fall out from the poll() - we try to
restore the original sigmask - again using SIG_BLOCK. But as the man
page says, SIG_BLOCK adds signals to the signal mask:
SIG_BLOCK
The set of blocked signals is the union of the current set and the set argument.
Therefore, when restoring the original mask, we need to completely
overwrite the one we set earlier and hence we should be using:
SIG_SETMASK
The set of blocked signals is set to the argument set.
Oops. That's not valid XML. And when we fix the XML
generation, it fails RelaxNG validation.
I'm also tired of seeing <key>(null)</key> in the example
output for volume xml; while we used NULLSTR() to avoid
a NULL deref rather than relying on glibc's printf
extension behavior, it's even better if we avoid the issue
in the first place. But this requires being careful that
we don't invalidate any storage backends that were relying
on key being unassigned during virStoragVolCreateXML[From].
I would have split this into two patches (one for escaping,
one for avoiding <key>(null)</key>), but since they both
end up touching a lot of the same test files, I ended up
merging it into one.
Note that this patch allows pretty much any volume name
that can appear in a directory (excluding . and .. because
those are special), but does nothing to change the current
(unenforced) RelaxNG claim that pool names will consist
only of letters, numbers, _, -, and +. Tightening the C
code to match RelaxNG patterns and/or relaxing the grammar
to match the C code for pool names is a task for another
day (but remember, we DID recently tighten C code for
domain names to exclude a leading '.').
* src/conf/storage_conf.c (virStoragePoolSourceFormat)
(virStoragePoolDefFormat, virStorageVolTargetDefFormat)
(virStorageVolDefFormat): Escape user-controlled strings.
(virStorageVolDefParseXML): Parse key, for use in unit tests.
* src/storage/storage_driver.c (storageVolCreateXML)
(storageVolCreateXMLFrom): Ensure parsed key doesn't confuse
volume creation.
* docs/schemas/basictypes.rng (volName): Relax definition.
* tests/storagepoolxml2xmltest.c (mymain): Test it.
* tests/storagevolxml2xmltest.c (mymain): Likewise.
* tests/storagepoolxml2xmlin/pool-dir-naming.xml: New file.
* tests/storagepoolxml2xmlout/pool-dir-naming.xml: Likewise.
* tests/storagevolxml2xmlin/vol-file-naming.xml: Likewise.
* tests/storagevolxml2xmlout/vol-file-naming.xml: Likewise.
* tests/storagevolxml2xmlout/vol-*.xml: Fix fallout.
Michal Privoznik [Fri, 21 Feb 2014 12:06:42 +0000 (13:06 +0100)]
virNetServerRun: Notify systemd that we're accepting clients
Systemd does not forget about the cases, where client service needs to
wait for daemon service to initialize and start accepting new clients.
Setting a dependency in client is not enough as systemd doesn't know
when the daemon has initialized itself and started accepting new
clients. However, it offers a mechanism to solve this. The daemon needs
to call a special systemd function by which the daemon tells "I'm ready
to accept new clients". This is exactly what we need with
libvirtd-guests (client) and libvirtd (daemon). So now, with this
change, libvirt-guests.service is invoked not any sooner than
libvirtd.service calls the systemd notify function.
Michal Privoznik [Fri, 21 Feb 2014 11:46:08 +0000 (12:46 +0100)]
libvirt-guests: Wait for libvirtd to initialize
I've noticed that in some cases systemd was quick enough and even
if libvirt-guests.service is marked to be started after the
libvirtd.service my guests were not resumed as
libvirt-guests.sh failed to connect. This is because of a
simple fact: systemd correctly starts libvirt-guests after it
execs libvirtd. However, the daemon is not able to accept
connections right from the start. It's doing some
initialization which may take ages. This problem is not limited
to systemd only, indeed. Any init system that is able to startup
services in parallel (e.g. OpenRC) may run into this situation.
The fix is to try connecting not only once, but continuously a few
times with a small sleep in between tries.
When creating a new domain, we let systemd know about it by calling
CreateMachine() function via dbus. Systemd then creates a scope and
places domain into it. However, later when the host is shutting
down, systemd computes the shutdown order to see what processes can
be shut down in parallel. And since we were not setting
dependencies at all, the slices (and thus domains) were most likely
killed before libvirt-guests.service. So user domains that had to
be saved, shut off, whatever were in fact killed. This problem can
be solved by letting systemd know that scopes we're creating must
not be killed before libvirt-guests.service.
The nwfilter conf update mutex previously serialized
updates to the internal data structures for firewall
rules, and updates to the firewall itself. The latter
was recently turned into a read/write lock, and filter
instantiation allowed to proceed in parallel. It was
believed that this was ok, since each filter is created
on a separate iptables/ebtables chain.
It turns out that there is a subtle lock ordering problem
on virNWFilterObjPtr instances. __virNWFilterInstantiateFilter
will hold a lock on the virNWFilterObjPtr it is instantiating.
This in turn invokes virNWFilterInstantiate which then invokes
virNWFilterDetermineMissingVarsRec which then invokes
virNWFilterObjFindByName. This iterates over every single
virNWFilterObjPtr in the list, locking them and checking their
name. So if 2 or more threads try to instantiate a filter in
parallel, they'll all hold 1 lock at the top level in the
__virNWFilterInstantiateFilter method which will cause the
other thread to deadlock in virNWFilterObjFindByName.
The fix is to add an exclusive mutex to serialize the
execution of __virNWFilterInstantiateFilter.
Guido Günther [Tue, 5 Nov 2013 08:04:43 +0000 (09:04 +0100)]
virt-login-shell: also build virAtomic.h
Needed for architectures that don't use gcc atomic ops but pthread. This
fixes the armel build that otherwise breaks like:
CCLD virt-login-shell
../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-virobject.o): In function `virClassNew':
/«PKGBUILDDIR»/debian/build/src/../../../src/util/virobject.c:150: undefined reference to `virAtomicLock'
../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-virobject.o): In function `virObjectNew':
/«PKGBUILDDIR»/debian/build/src/../../../src/util/virobject.c:202: undefined reference to `virAtomicLock'
../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-virobject.o): In function `virObjectUnref':
/«PKGBUILDDIR»/debian/build/src/../../../src/util/virobject.c:274: undefined reference to `virAtomicLock'
../src/.libs/libvirt-setuid-rpc-client.a(libvirt_setuid_rpc_client_la-virobject.o): In function `virObjectRef':
/«PKGBUILDDIR»/debian/build/src/../../../src/util/virobject.c:295: undefined reference to `virAtomicLock'
collect2: error: ld returned 1 exit status
See https://buildd.debian.org/status/fetch.php?pkg=libvirt&arch=armel&ver=1.1.4-1&stamp=1383588268
Ján Tomko [Tue, 18 Feb 2014 14:01:32 +0000 (15:01 +0100)]
Fix conflicting types of virInitctlSetRunLevel
aebbcdd didn't change the non-linux definition of the function,
breaking the build on FreeBSD:
../../src/util/virinitctl.c:164: error: conflicting types for
'virInitctlSetRunLevel'
../../src/util/virinitctl.h:40: error: previous declaration of
'virInitctlSetRunLevel' was here
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC hotunplug code
Rewrite multiple hotunplug functions to to use the
virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with an absolute
symlink, tricking the driver into changing the host OS
filesystem.
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC chardev hostdev hotplug
Rewrite lxcDomainAttachDeviceHostdevMiscLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC block hostdev hotplug
Rewrite lxcDomainAttachDeviceHostdevStorageLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC USB hotplug
Rewrite lxcDomainAttachDeviceHostdevSubsysUSBLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC disk hotplug
Rewrite lxcDomainAttachDeviceDiskLive function to use the
virProcessRunInMountNamespace helper. This avoids risk of
a malicious guest replacing /dev with a absolute symlink,
tricking the driver into changing the host OS filesystem.
Eric Blake [Tue, 24 Dec 2013 05:55:51 +0000 (22:55 -0700)]
CVE-2013-6456: Avoid unsafe use of /proc/$PID/root in LXC shutdown/reboot code
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and
lxcDomainReboot. Otherwise, a malicious guest could use symlinks
to force the host to manipulate the wrong file in the host's namespace.
Idea by Dan Berrange, based on an initial report by Reco
<recoverym4n@gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Add helper for running code in separate namespaces
Implement virProcessRunInMountNamespace, which runs callback of type
virProcessNamespaceCallback in a container namespace. This uses a
child process to run the callback, since you can't change the mount
namespace of a thread. This implies that callbacks have to be careful
about what code they run due to async safety rules.
Idea by Dan Berrange, based on an initial report by Reco
<recoverym4n@gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Daniel Berrange <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 7c72ef6f555f1f9844d51be2f38f078bc908652c)
Add a helper function which takes a file path and ensures
that all directory components leading up to the file exist.
IOW, it strips the filename part of the path and passes
the result to virFileMakePath.
Move check for cgroup devices ACL upfront in LXC hotplug
The check for whether the cgroup devices ACL is available is
done quite late during LXC hotplug - in fact after the device
node is already created in the container in some cases. Better
to do it upfront so we fail immediately.
Fix reset of cgroup when detaching USB device from LXC guests
When detaching a USB device from an LXC guest we must remove
the device from the cgroup ACL. Unfortunately we were telling
the cgroup code to use the guest /dev path, not the host /dev
path, and the guest device node had already been unlinked.
This was, however, fortunate since the code passed &priv->cgroup
instead of priv->cgroup, so would have crash if the device node
were accessible.
The LXC code missed the 'usb' component out of the path
/dev/bus/usb/$BUSNUM/$DEVNUM, so it failed to actually
setup cgroups for the device. This was in fact lucky
because the call to virLXCSetupHostUsbDeviceCgroup
was also mistakenly passing '&priv->cgroup' instead of
just 'priv->cgroup'. So once the path is fixed, libvirtd
would then crash trying to access the bogus virCgroupPtr
pointer. This would have been a security issue, were it
not for the bogus path preventing the pointer reference
being reached.
virDomainDefCompatibleDevice blocks use of USB if no USB
controller is present. This is not correct for containers
since devices can be assigned directly regardless of any
controllers.
Eric Blake [Tue, 5 Nov 2013 17:30:56 +0000 (10:30 -0700)]
storage: avoid short reads while chasing backing chain
Our backing file chain code was not very robust to an ill-timed
EINTR, which could lead to a short read causing us to randomly
treat metadata differently than usual. But the existing
virFileReadLimFD forces an error if we don't read the entire
file, even though we only care about the header of the file.
So add a new virFile function that does what we want.
* src/util/virfile.h (virFileReadHeaderFD): New prototype.
* src/util/virfile.c (virFileReadHeaderFD): New function.
* src/libvirt_private.syms (virfile.h): Export it.
* src/util/virstoragefile.c (virStorageFileGetMetadataInternal)
(virStorageFileProbeFormatFromFD): Use it.
Commit f9f56340 for CVE-2014-0028 almost had the right idea - we
need to check the ACL rules to filter which events to send. But
it overlooked one thing: the event dispatch queue is running in
the main loop thread, and therefore does not normally have a
current virIdentityPtr. But filter checks can be based on current
identity, so when libvirtd.conf contains access_drivers=["polkit"],
we ended up rejecting access for EVERY event due to failure to
look up the current identity, even if it should have been allowed.
Furthermore, even for events that are triggered by API calls, it
is important to remember that the point of events is that they can
be copied across multiple connections, which may have separate
identities and permissions. So even if events were dispatched
from a context where we have an identity, we must change to the
correct identity of the connection that will be receiving the
event, rather than basing a decision on the context that triggered
the event, when deciding whether to filter an event to a
particular connection.
If there were an easy way to get from virConnectPtr to the
appropriate virIdentityPtr, then object_event.c could adjust the
identity prior to checking whether to dispatch an event. But
setting up that back-reference is a bit invasive. Instead, it
is easier to delay the filtering check until lower down the
stack, at the point where we have direct access to the RPC
client object that owns an identity. As such, this patch ends
up reverting a large portion of the framework of commit f9f56340.
We also have to teach 'make check' to special-case the fact that
the event registration filtering is done at the point of dispatch,
rather than the point of registration. Note that even though we
don't actually use virConnectDomainEventRegisterCheckACL (because
the RegisterAny variant is sufficient), we still generate the
function for the purposes of documenting that the filtering
takes place.
Also note that I did not entirely delete the notion of a filter
from object_event.c; I still plan on using that for my upcoming
patch series for qemu monitor events in libvirt-qemu.so. In
other words, while this patch changes ACL filtering to live in
remote.c and therefore we have no current client of the filtering
in object_event.c, the notion of filtering in object_event.c is
still useful down the road.
* src/check-aclrules.pl: Exempt event registration from having to
pass checkACL filter down call stack.
* daemon/remote.c (remoteRelayDomainEventCheckACL)
(remoteRelayNetworkEventCheckACL): New functions.
(remoteRelay*Event*): Use new functions.
* src/conf/domain_event.h (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Drop unused parameter.
* src/conf/network_event.h (virNetworkEventStateRegisterID):
Likewise.
* src/conf/domain_event.c (virDomainEventFilter): Delete unused
function.
* src/conf/network_event.c (virNetworkEventFilter): Likewise.
* src/libxl/libxl_driver.c: Adjust caller.
* src/lxc/lxc_driver.c: Likewise.
* src/network/bridge_driver.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
* src/remote/remote_driver.c: Likewise.
* src/test/test_driver.c: Likewise.
* src/uml/uml_driver.c: Likewise.
* src/vbox/vbox_tmpl.c: Likewise.
* src/xen/xen_driver.c: Likewise.
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
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.
When all source CPU XMLs contain just a single CPU model (with a
possibly varying set of additional feature elements),
virConnectBaselineCPU will try to use this CPU model in the computed
guest CPU. Thus, when used on just a single CPU (useful with
VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES), the result will not use a
different CPU model.
If the computed CPU uses the source model, set fallback mode to 'forbid'
to make sure the guest CPU will always be as close as possible to the
source CPUs.
VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES flag for virConnectBaselineCPU
did not work if the resulting guest CPU would disable some features
present in its base model. This patch makes sure we won't try to add
such features twice.
There is a number of reported issues when we fail starting a domain.
Turns out that, in some scenarios like high load, 3 second timeout is
not enough for qemu to start up to the phase where the socket is
created. Since there is no downside of waiting longer, raise the
timeout right to 30 seconds.
Eric Blake [Mon, 23 Dec 2013 17:15:48 +0000 (10:15 -0700)]
virt-login-shell: fix regressions in behavior
Our fixes for CVE-2013-4400 were so effective at "fixing" bugs
in virt-login-shell that we ended up fixing it into a useless
do-nothing program.
Commit 3e2f27e1 picked the name LIBVIRT_SETUID_RPC_CLIENT for
the witness macro when we are doing secure compilation. But
commit 9cd6a57d checked whether the name IN_VIRT_LOGIN_SHELL,
from an earlier version of the patch series, was defined; with
the net result that virt-login-shell invariably detected that
it was setuid and failed virInitialize.
Commit b7fcc799 closed all fds larger than stderr, but in the
wrong place. Looking at the larger context, we mistakenly did
the close in between obtaining the set of namespace fds, then
actually using those fds to switch namespace, which means that
virt-login-shell will ALWAYS fail.
This is the minimal patch to fix the regressions, although
further patches are also worth having to clean up poor
semantics of the resulting program (for example, it is rude to
not pass on the exit status of the wrapped program back to the
invoking shell).
* tools/virt-login-shell.c (main): Don't close fds until after
namespace swap.
* src/libvirt.c (virGlobalInit): Use correct macro.
Unfortunately this is racy - since the event loop is in a
different thread, the virDBusWatchCallback method may be
run before we get to calling dbus_watch_set_data. We must
reverse the order of these calls
See https://bugzilla.redhat.com/show_bug.cgi?id=885445
Eric Blake [Tue, 14 Jan 2014 17:29:34 +0000 (10:29 -0700)]
event: filter global events by domain:getattr ACL [CVE-2014-0028]
Ever since ACL filtering was added in commit 7639736 (v1.1.1), a
user could still use event registration to obtain access to a
domain that they could not normally access via virDomainLookup*
or virConnectListAllDomains and friends. We already have the
framework in the RPC generator for creating the filter, and
previous cleanup patches got us to the point that we can now
wire the filter through the entire object event stack.
Furthermore, whether or not domain:getattr is honored, use of
global events is a form of obtaining a list of networks, which
is covered by connect:search_domains added in a93cd08 (v1.1.0).
Ideally, we'd have a way to enforce connect:search_domains when
doing global registrations while omitting that check on a
per-domain registration. But this patch just unconditionally
requires connect:search_domains, even when no list could be
obtained, based on the following observations:
1. Administrators are unlikely to grant domain:getattr for one
or all domains while still denying connect:search_domains - a
user that is able to manage domains will want to be able to
manage them efficiently, but efficient management includes being
able to list the domains they can access. The idea of denying
connect:search_domains while still granting access to individual
domains is therefore not adding any real security, but just
serves as a layer of obscurity to annoy the end user.
2. In the current implementation, domain events are filtered
on the client; the server has no idea if a domain filter was
requested, and must therefore assume that all domain event
requests are global. Even if we fix the RPC protocol to
allow for server-side filtering for newer client/server combos,
making the connect:serach_domains ACL check conditional on
whether the domain argument was NULL won't benefit older clients.
Therefore, we choose to document that connect:search_domains
is a pre-requisite to any domain event management.
Network events need the same treatment, with the obvious
change of using connect:search_networks and network:getattr.
Eric Blake [Tue, 14 Jan 2014 00:30:59 +0000 (17:30 -0700)]
Fix memory leak in virObjectEventCallbackListRemoveID()
While running objecteventtest, it was found that valgrind pointed out the
following memory leak:
==13464== 5 bytes in 1 blocks are definitely lost in loss record 7 of 134
==13464== at 0x4A0887C: malloc (vg_replace_malloc.c:270)
==13464== by 0x341F485E21: strdup (strdup.c:42)
==13464== by 0x4CAE28F: virStrdup (virstring.c:554)
==13464== by 0x4CF3CBE: virObjectEventCallbackListAddID (object_event.c:286)
==13464== by 0x4CF49CA: virObjectEventStateRegisterID (object_event.c:729)
==13464== by 0x4CF73FE: virDomainEventStateRegisterID (domain_event.c:1424)
==13464== by 0x4D7358F: testConnectDomainEventRegisterAny (test_driver.c:6032)
==13464== by 0x4D600C8: virConnectDomainEventRegisterAny (libvirt.c:19128)
==13464== by 0x402409: testDomainStartStopEvent (objecteventtest.c:232)
==13464== by 0x403451: virtTestRun (testutils.c:138)
==13464== by 0x402012: mymain (objecteventtest.c:395)
==13464== by 0x403AF2: virtTestMain (testutils.c:593)
==13464==
The @list->callbacks is an array that is inflated whenever a new event
is added, e.g. via virDomainEventCallbackListAddID(). However, when we
are freeing the array, we free the items within it but forgot to
actually free it.
When writing commit 173c291, I missed the fact virNetServerClientClose
unlocks the client object before actually clearing client->sock and thus
it is possible to hit a window when client->keepalive is NULL while
client->sock is not NULL. I was thinking client->sock == NULL was a
better check for a closed connection but apparently we have to go with
client->keepalive == NULL to actually fix the crash.
projects / thirdparty / libvirt.git / log
