- Devel - libvirt List Archives

[libvirt PATCH] remote: use SocketMode=0600 when polkit is not compiled
by Daniel P. Berrangé 03 Sep '20

03 Sep '20

The systemd .socket unit files we ship for libvirt daemons use SocketMode=0666 on the assumption that libvirt is built with polkit which provides access control. Some people, however, may have explicitly turned off polkit at build time and not realize that leaves them insecure unless they also change the SocketMode. This addresses that problem by making the SocketMode default to 0600 when polkit is disabled at compile time. Note we cannot automatically fix the case where the user compiles polkit, but then overrides the libvirtd.conf defaults to disable polkit. This is what lead to CVE-2020-15708 in Ubuntu 20.10. We can at least improve the inline comments in the config file to give a clearer warning though, which may have helped avoid the mistaken config. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/meson.build | 11 ++++++++++ src/remote/libvirtd.conf.in | 40 ++++++++++++++++++++++++++--------- src/remote/libvirtd.socket.in | 2 +- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/meson.build b/src/meson.build index 5d8deaf548..897b5ecbca 100644 --- a/src/meson.build +++ b/src/meson.build @@ -713,6 +713,12 @@ foreach data : virt_daemon_confs daemon_conf.set('DAEMON_NAME_UC', name_uc) # to silence meson warning about missing 'CONFIG' in the configuration_data daemon_conf.set('CONFIG', '@CONFIG@') + if conf.has('WITH_POLKIT') + daemon_conf.set('default_auth', 'polkit') + else + daemon_conf.set('default_auth', 'none') + endif + if data.get('with_ip', false) conf_in = libvirtd_conf_tmp @@ -792,6 +798,11 @@ if conf.has('WITH_LIBVIRTD') unit_conf.set('service', unit['service']) unit_conf.set('sockprefix', unit['sockprefix']) unit_conf.set('deps', unit.get('deps', '')) + if conf.has('WITH_POLKIT') + unit_conf.set('mode', '0666') + else + unit_conf.set('mode', '0600') + endif configure_file( input: unit['service_in'], diff --git a/src/remote/libvirtd.conf.in b/src/remote/libvirtd.conf.in index 2607fbad86..ae6207bf54 100644 --- a/src/remote/libvirtd.conf.in +++ b/src/remote/libvirtd.conf.in @@ -127,6 +127,8 @@ # # Authentication. # +# There are the following choices available: +# # - none: do not perform auth checks. If you can connect to the # socket you are allowed. This is suitable if there are # restrictions on connecting to the socket (eg, UNIX @@ -144,21 +146,39 @@ # full read/write access (aka sudo like), while anyone # is allowed read/only access. # + # Set an authentication scheme for UNIX read-only sockets +# # By default socket permissions allow anyone to connect # -# To restrict monitoring of domains you may wish to enable -# an authentication mechanism here -#auth_unix_ro = "none" +# If libvirt was compiled without support for 'polkit', then +# no access control checks are done, but libvirt still only +# allows execution of APIs which don't change state. +# +# If libvirt was compiled with support for 'polkit', then +# the libvirt socket will perform a check with polkit after +# connections. The default policy still allows any local +# user access. +# +# To restrict monitoring of domains you may wish to either +# enable 'sasl' here, or change the polkit policy definition. +#auth_unix_ro = "@default_auth@" -# Set an authentication scheme for UNIX read-write sockets -# By default socket permissions only allow root. If PolicyKit -# support was compiled into libvirt, the default will be to -# use 'polkit' auth. +# Set an authentication scheme for UNIX read-write sockets. +# +# If libvirt was compiled without support for 'polkit', then +# the systemd .socket files will use SocketMode=0600 by default +# thus only allowing root user to connect, and 'auth_unix_rw' +# will default to 'none'. +# +# If libvirt was compiled with support for 'polkit', then +# the systemd .socket files will use SocketMode=0666 which +# allows any user to connect and 'auth_iunix_rw' will default +# to 'polkit'. If you disable use of 'polkit' here, then it +# is essential to change the systemd SocketMode parameter +# back to 0600, to avoid an insecure configuration. # -# If the unix_sock_rw_perms are changed you may wish to enable -# an authentication mechanism here -#auth_unix_rw = "none" +#auth_unix_rw = "@default_auth@" @CUT_ENABLE_IP@ # Change the authentication scheme for TCP sockets. diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in index df36df2125..85b4aa800a 100644 --- a/src/remote/libvirtd.socket.in +++ b/src/remote/libvirtd.socket.in @@ -8,7 +8,7 @@ Before=@service@.service # when using systemd version < 227 ListenStream=@runstatedir@/libvirt/@sockprefix@-sock Service=@service@.service -SocketMode=0666 +SocketMode=@mode@ [Install] WantedBy=sockets.target -- 2.25.4

3 3

[PATCH] storage: only fallocate when allocation matches capacity
by Christian Ehrhardt 03 Sep '20

03 Sep '20

In c9ec7088 "storage: extend preallocation flags support for qemu-img" the option to fallocate was added and meant to be active when (quote): "the XML described storage <allocation> matches its <capacity>" Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation" the compared allocation size was an order of magnitude too small, but still it does use fallocate too often unless capacity>allocation. This change fixes the comparison to match the intended description of the feature. Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454 Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105 Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com> --- src/storage/storage_util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index cf82ea0a87..85bed76863 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -710,10 +710,10 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr encinfo, virQEMUBuildQemuImgKeySecretOpts(&buf, encinfo, info->secretAlias); if (info->preallocate) { - if (info->size_arg > info->allocation) - virBufferAddLit(&buf, "preallocation=metadata,"); - else + if (info->size_arg == info->allocation) virBufferAddLit(&buf, "preallocation=falloc,"); + else + virBufferAddLit(&buf, "preallocation=metadata,"); } if (info->nocow) -- 2.28.0

4 8

[libvirt PATCH v2] meson: add -Wall and -Wextra explicitly
by Pavel Hrdina 03 Sep '20

03 Sep '20

If someone runs `meson setup --buildtype plain` meson ignores warning_level=2 that is in our meson.build file. The implication is that Meson will not automatically add -Wall which enables -Wformat. This breaks building libvirt from git with the buildtype set to plain. There is an issue reported [1] to not ignore warning_level silently and the change to ignore it was done by upstream commit [2]. This change makes Meson print warnings about using warning_level which as described above doesn't work for all build types. [1] <https://github.com/mesonbuild/meson/issues/7399> [2] <https://github.com/mesonbuild/meson/commit/8ee1c9a07a3a35e3ed262fbc358fd86c…> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com> --- Chages in v2: - add -Wall and -Wextra explicitly regardless of the buildtype meson.build | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index 1aad385ad1..1eadea33bf 100644 --- a/meson.build +++ b/meson.build @@ -228,6 +228,7 @@ cc_flags += [ '-Waddress', '-Waddress-of-packed-member', '-Waggressive-loop-optimizations', + '-Wall', '-Wattribute-warning', '-Wattributes', '-Wbool-compare', @@ -257,6 +258,7 @@ cc_flags += [ '-Wempty-body', '-Wendif-labels', '-Wexpansion-to-defined', + '-Wextra', '-Wformat-contains-nul', '-Wformat-extra-args', '-Wformat-nonliteral', @@ -503,10 +505,6 @@ cc_flags_disabled = [ # In meson this is specified using 'c_std=gnu99' in project() function. '-std=gnu99', - # In meson this is specified using 'warning_level=2' in project() function. - '-Wall', - '-Wextra', - # don't care about C++ compiler compat '-Wc++-compat', '-Wabi', -- 2.26.2

2 1

[libvirt PATCH] tests: change confusing macro agument names
by Jonathon Jongsma 03 Sep '20

03 Sep '20

Rather than use the names "fial" and "kep", use "fail" and "keep". In the DO_TEST() macro, to prevent the preprocessor replacing the struct member names during assignment, use the names "fail_" and "keep_" instead. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com> --- tests/qemuhotplugtest.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1e18820a2b..b023c49a69 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -641,28 +641,28 @@ mymain(void) } -#define DO_TEST(file, ACTION, dev, fial, kep, ...) \ +#define DO_TEST(file, ACTION, dev, fail_, keep_, ...) \ do { \ const char *my_mon[] = { __VA_ARGS__, NULL}; \ const char *name = file " " #ACTION " " dev; \ data.action = ACTION; \ data.domain_filename = file; \ data.device_filename = dev; \ - data.fail = fial; \ + data.fail = fail_; \ data.mon = my_mon; \ - data.keep = kep; \ + data.keep = keep_; \ if (virTestRun(name, testQemuHotplug, &data) < 0) \ ret = -1; \ } while (0) -#define DO_TEST_ATTACH(file, dev, fial, kep, ...) \ - DO_TEST(file, ATTACH, dev, fial, kep, __VA_ARGS__) +#define DO_TEST_ATTACH(file, dev, fail, keep, ...) \ + DO_TEST(file, ATTACH, dev, fail, keep, __VA_ARGS__) -#define DO_TEST_DETACH(file, dev, fial, kep, ...) \ - DO_TEST(file, DETACH, dev, fial, kep, __VA_ARGS__) +#define DO_TEST_DETACH(file, dev, fail, keep, ...) \ + DO_TEST(file, DETACH, dev, fail, keep, __VA_ARGS__) -#define DO_TEST_UPDATE(file, dev, fial, kep, ...) \ - DO_TEST(file, UPDATE, dev, fial, kep, __VA_ARGS__) +#define DO_TEST_UPDATE(file, dev, fail, keep, ...) \ + DO_TEST(file, UPDATE, dev, fail, keep, __VA_ARGS__) #define QMP_OK "{\"return\": {}}" -- 2.26.2

2 1

[PATCH] vsh: Define HAVE_STDARG_H before including readline
by Michal Privoznik 03 Sep '20

03 Sep '20

As it turned out my previous commits which switched from HAVE_ to WITH_ and dropped stdarg.h detection were a bit too aggressive. Because of reasons described in 9ea3424a178 we need to define HAVE_STDARG_H before including readline otherwise macos build fails. Honestly, I still don't fully understand the problem so I am not going to bother you with "explanation". Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- tools/vsh.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/vsh.c b/tools/vsh.c index 11f493f969..d063a7f8cb 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -30,6 +30,10 @@ #include <signal.h> #if WITH_READLINE +/* In order to have proper rl_message declaration with older + * versions of readline, we have to declare this. See 9ea3424a178 + * for more info. */ +# define HAVE_STDARG_H # include <readline/readline.h> # include <readline/history.h> #endif -- 2.26.2

3 2

[PATCH] meson: Check for stdarg.h
by Michal Privoznik 03 Sep '20

03 Sep '20

As it turns out, one of my previous commits in which I removed checking for stdarg.h was too aggressive. Long story short, the readline public headers rely on stdarg.h and what is worse, they expect us to declare the autotools style of macro (HAVE_STDARG_H) if the header file exists. If we don't do it then compiling virsh on macos fails. See 9ea3424a178 for more info. Fixes: 85808b73846f93d656b4c81b6ebddd2dc3881bf6 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- meson.build | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/meson.build b/meson.build index 1aad385ad1..98f7545495 100644 --- a/meson.build +++ b/meson.build @@ -1333,8 +1333,12 @@ if readline_dep.found() endif endif - # We need this to avoid compilation issues with modern compilers. - # See 9ea3424a178 for a more detailed explanation + # We need both of these hacks to avoid compilation issues with modern + # compilers. See 9ea3424a178 for a more detailed explanation. + if cc.has_header('stdarg.h') + conf.set('HAVE_STDARG_H', 1) + endif + readline_dep = declare_dependency( compile_args: [ '-D_FUNCTION_DEF' ], dependencies: [ readline_dep ], -- 2.26.2

4 4

[PATCH] rpm: Fix conditional for defining %_vpath_builddir for RHEL <= 7
by Neal Gompa 02 Sep '20

02 Sep '20

The conditional was incorrectly overriding %_vpath_builddir when %rhel is not defined, which led to surprising behavior when the global %_vpath_builddir path is set on Fedora already. Signed-off-by: Neal Gompa <ngompa13(a)gmail.com> --- libvirt.spec.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index bb74443484..4b9e04ae61 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -32,7 +32,7 @@ %endif # On RHEL 7 and older macro _vpath_builddir is not defined. -%if 0%{?rhel} <= 7 +%if 0%{?rhel} && 0%{?rhel} <= 7 %define _vpath_builddir %{_target_platform} %endif -- 2.26.2

2 2

[PATCH v2] qemu_validate: Only allow none address for watchdog ib700
by Han Han 02 Sep '20

02 Sep '20

Since QEMU 1.5.3, the ib700 watchdog device has no options for address, and not address in device tree: $ /usr/libexec/qemu-kvm -version QEMU emulator version 1.5.3 (qemu-kvm-1.5.3-175.el7), Copyright (c) 2003-2008 Fabrice Bellard $ /usr/libexec/qemu-kvm -device ib700,\? $ virsh qemu-monitor-command seabios --hmp info qtree|grep ib700 -A 2 dev: ib700, id "watchdog0" dev: isa-serial, id "serial0" index = 0 So only allow it to use none address. Fixes: 8a54cc1d08a333283c9cfc3fd7788be2642ca71a Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1509908 Signed-off-by: Han Han <hhan(a)redhat.com> --- src/qemu/qemu_validate.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..402f81a3cd 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -1637,10 +1637,9 @@ qemuValidateDomainWatchdogDef(const virDomainWatchdogDef *dev, break; case VIR_DOMAIN_WATCHDOG_MODEL_IB700: - if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA) { + if (dev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s model of watchdog can go only on ISA bus"), + _("%s model of watchdog does not support configuring the address"), virDomainWatchdogModelTypeToString(dev->model)); return -1; } -- 2.28.0

3 2

[PATCH v2] qemu: Do not silently allow non-available timers on non-x86 systems
by Thomas Huth 02 Sep '20

02 Sep '20

libvirt currently silently allows <timer name="kvmclock"/> and some other timer tags in the guest XML definition for timers that do not exist on non-x86 systems. We should not silently ignore these tags since the users might not get what they expected otherwise. Note: The error is only generated if the timer is marked with present="yes" - otherwise we would suddenly refuse XML definitions that worked without problems before. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1754887 Signed-off-by: Thomas Huth <thuth(a)redhat.com> --- v2: Check also for timer->present == 1 src/qemu/qemu_validate.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 488f258d00..561e7b12c7 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -371,6 +371,18 @@ qemuValidateDomainDefClockTimers(const virDomainDef *def, case VIR_DOMAIN_TIMER_NAME_TSC: case VIR_DOMAIN_TIMER_NAME_KVMCLOCK: case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK: + if (!ARCH_IS_X86(def->os.arch) && timer->present == 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Configuring the '%s' timer is not supported " + "for virtType=%s arch=%s machine=%s guests"), + virDomainTimerNameTypeToString(timer->name), + virDomainVirtTypeToString(def->virtType), + virArchToString(def->os.arch), + def->os.machine); + return -1; + } + break; + case VIR_DOMAIN_TIMER_NAME_LAST: break; -- 2.18.1

5 5

[PATCH] qemu_driver.c: Fix domfsinfo for non-PCI device information from guest agent
by Thomas Huth 02 Sep '20

02 Sep '20

qemuAgentFSInfoToPublic() currently only sets the devAlias for PCI devices. However, the QEMU guest agent could also provide the device name in the "dev" field of the response for other devices instead (well, at least after fixing another problem in the current QEMU guest agent...). So if creating the devAlias from the PCI information failed, let's fall back to the name provided by the guest agent. This helps to fix the empty "Target" fields that occur when running "virsh domfsinfo" on s390x where CCW devices are used for the guest instead of PCI devices. Also add a proper debug message here in case we completely failed to set the device alias, since this problem here was very hard to debug: The only two error messages that I've seen were "Unable to get filesystem information" and "Unable to encode message payload" - which only indicates that something went wrong in the RPC call. No debug message indicated the real problem, so I had to learn the hard way why the RPC call failed (it apparently does not like devAlias left to be NULL) and where the real problem comes from. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075 Signed-off-by: Thomas Huth <thuth(a)redhat.com> --- src/qemu/qemu_driver.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d185666ed8..e45c61ee20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21935,14 +21935,17 @@ qemuAgentFSInfoToPublic(qemuAgentFSInfoPtr agent, qemuAgentDiskInfoPtr agentdisk = agent->disks[i]; virDomainDiskDefPtr diskDef; - if (!(diskDef = virDomainDiskByAddress(vmdef, - &agentdisk->pci_controller, - agentdisk->bus, - agentdisk->target, - agentdisk->unit))) - continue; - - ret->devAlias[i] = g_strdup(diskDef->dst); + diskDef = virDomainDiskByAddress(vmdef, + &agentdisk->pci_controller, + agentdisk->bus, + agentdisk->target, + agentdisk->unit); + if (diskDef != NULL) + ret->devAlias[i] = g_strdup(diskDef->dst); + else if (agentdisk->devnode != NULL) + ret->devAlias[i] = g_strdup(agentdisk->devnode); + else + VIR_DEBUG("Missing devnode name for '%s'.", ret->mountpoint); } return ret; -- 2.18.1

2 5