[libvirt] [PATCH 0/2] qemu: Add support for host-model pseries machine option
This series adds support for the -machine,host-model= QEMU option for for pseries guests. Pseries guests used to have a node (/proc/device-tree/host-model) in device-tree that exposed the host's model string so that guest userspace tools could determine the host machine they were running on. QEMU used to provide the node by default, but this has been disabled due to security concerns. There is now a machine option (host-model) that allows the user to set an arbitrary string to be used as the host model. Userspace tools will then be broken unless the user explicitly edits the domain XML with the desired host model to be exposed to the guest. However, having an arbitrary string in the domain XML that needs to be the same for every guest in that machine but different across host machines would generate issues with XML portability and more importantly, migration. So this series implements a generic way for an administrator to enable the old behavior for a specific guest without allowing arbitrary strings. Implementation note: The 'host-serial' property, which reads /proc/device-tree/system-id in the host and was also addressed by the QEMU change could be implemented similarly in the future. However I see that we're currently populating smbios structures (virSysinfoDef) with info gathered from /proc/cpuinfo for architectures that do not use smbios and I think that should be addressed first. My idea is to perhaps create new sysinfo types like the existing 'smbios', such as 'cpuinfo' and 'device-tree'. So to keep the two discussions separated I took advantage of the fact that the host model happens to also be present at /proc/cpuinfo and used the smbios structures for now. Fabiano Rosas (2): qemu: Add capability for pseries machine 'host-model' parameter qemu: Add support for pseries 'host-model' machine parameter docs/formatdomain.html.in | 9 ++++++ docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 4 +++ src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 25 ++++++++++++++-- src/qemu/qemu_domain.c | 1 + .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + tests/qemuxml2argvdata/pseries-features.args | 3 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 30 ++++++++++++++++++- tests/qemuxml2xmloutdata/pseries-features.xml | 1 + 13 files changed, 79 insertions(+), 5 deletions(-) -- 2.20.1
As of QEMU v4.0.0, the pseries machine has a new parameter 'host-model' which receives an arbitrary string to be exposed inside the guest at /proc/device-tree/host-model. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 + 3 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 46a056340b..3933f4a01b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -545,6 +545,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "incremental-backup", "query-cpu-model-baseline", "query-cpu-model-comparison", + "machine.pseries.host-model", ); @@ -1440,6 +1441,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsPSeries[] = { { "cap-hpt-max-page-size", QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE }, { "cap-htm", QEMU_CAPS_MACHINE_PSERIES_CAP_HTM }, { "cap-nested-hv", QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV }, + { "host-model", QEMU_CAPS_MACHINE_PSERIES_HOST_MODEL }, }; static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 72da3691f2..3adc2d8b65 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -526,6 +526,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_INCREMENTAL_BACKUP, /* incremental backup is supported */ QEMU_CAPS_QUERY_CPU_MODEL_BASELINE, /* qmp query-cpu-model-baseline */ QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON, /* qmp query-cpu-model-comparison */ + QEMU_CAPS_MACHINE_PSERIES_HOST_MODEL, /* -machine pseries,host-model= */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml index 9ea6f4d046..37c76225a3 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml @@ -170,6 +170,7 @@ <flag name='nbd-bitmap'/> <flag name='bochs-display'/> <flag name='migration-file-drop-cache'/> + <flag name='machine.pseries.host-model'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>42900758</microcodeVersion> -- 2.20.1
Since QEMU v4.0.0, the host's model is no longer automatically exposed to the guest via /proc/device-tree/host-model. There is now a machine option 'host-model' that allows an arbitrary string to be used as the host model [1]. This patch adds support for exposing the real host model string from /proc/cpuinfo (also found on /proc/device-tree/model) to the guest via -machine pseries,host-model=<model> by setting in the domain XML file: <features> <host_model_passthrough state='on'/> <features/> The functionality is disabled by default to avoid leaking host information to the guest. 1 - https://git.qemu.org/?p=qemu.git;a=commitdiff;h=0a794529bd11 Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- docs/formatdomain.html.in | 9 ++++++ docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 4 +++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 25 ++++++++++++++-- src/qemu/qemu_domain.c | 1 + tests/qemuxml2argvdata/pseries-features.args | 3 +- tests/qemuxml2argvdata/pseries-features.xml | 1 + tests/qemuxml2argvtest.c | 30 ++++++++++++++++++- tests/qemuxml2xmloutdata/pseries-features.xml | 1 + 10 files changed, 75 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 647f96f651..b613f61f23 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2357,6 +2357,15 @@ will not be ignored. <span class="since">Since 5.1.0</span> (bhyve only) </dd> + <dt><code>host-model-passthrough</code></dt> + <dd>Configure the passthrough of the host's model string to + pSeries guests. The host model will be present in the guest at + /proc/device-tree/host-model. + Possible values for the <code>state</code> attribute are + <code>on</code> and <code>off</code>. If the attribute is + not defined, the default value (<code>off</code>) is assumed. + <span class="since">Since 5.8.0</span> (QEMU/KVM only) + </dd> </dl> <h3><a id="elementsTime">Time keeping</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 40eb4a2d75..0d505bb38a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5117,6 +5117,11 @@ <ref name="featurestate"/> </element> </optional> + <optional> + <element name="host-model-passthrough"> + <ref name="featurestate"/> + </element> + </optional> <optional> <ref name="msrs"/> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a53cd6a725..9803f18389 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -171,6 +171,7 @@ VIR_ENUM_IMPL(virDomainFeature, "htm", "nested-hv", "msrs", + "host-model-passthrough", ); VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, @@ -20396,6 +20397,7 @@ virDomainDefParseXML(xmlDocPtr xml, case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: + case VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH: if (!(tmp = virXMLPropString(nodes[i], "state"))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("missing state attribute '%s' of feature '%s'"), @@ -22621,6 +22623,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: + case VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " @@ -28187,6 +28190,7 @@ virDomainDefFormatFeatures(virBufferPtr buf, case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: + case VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH: switch ((virTristateSwitch) def->features[i]) { case VIR_TRISTATE_SWITCH_LAST: case VIR_TRISTATE_SWITCH_ABSENT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2884af49d8..ba32116e90 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1755,6 +1755,7 @@ typedef enum { VIR_DOMAIN_FEATURE_HTM, VIR_DOMAIN_FEATURE_NESTED_HV, VIR_DOMAIN_FEATURE_MSRS, + VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH, VIR_DOMAIN_FEATURE_LAST } virDomainFeature; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index cbf25d5f07..bdb4461282 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7110,7 +7110,6 @@ qemuAppendLoadparmMachineParm(virBuffer *buf, } } - static int qemuBuildNameCommandLine(virCommandPtr cmd, virQEMUDriverConfigPtr cfg, @@ -7140,14 +7139,16 @@ qemuBuildNameCommandLine(virCommandPtr cmd, static int qemuBuildMachineCommandLine(virCommandPtr cmd, - virQEMUDriverConfigPtr cfg, + virQEMUDriverPtr driver, const virDomainDef *def, virQEMUCapsPtr qemuCaps) { virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT]; virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM]; virCPUDefPtr cpu = def->cpu; + virSysinfoDefPtr hostinfo = driver->hostsysinfo; VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); size_t i; /* This should *never* be NULL, since we always provide @@ -7409,6 +7410,24 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, virBufferAsprintf(&buf, ",cap-nested-hv=%s", str); } + if (def->features[VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH] == VIR_TRISTATE_SWITCH_ON) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_HOST_MODEL)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Host model passthrough is not supported by " + "this QEMU binary")); + return -1; + } + + if (!hostinfo || !hostinfo->system) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Host model information is not available")); + return -1; + } + + virBufferAddLit(&buf, ",host-model="); + virQEMUBuildBufferEscapeComma(&buf, hostinfo->system->serial); + } + if (cpu && cpu->model && cpu->mode == VIR_CPU_MODE_HOST_MODEL && qemuDomainIsPSeries(def) && @@ -10309,7 +10328,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (enableFips) virCommandAddArg(cmd, "-enable-fips"); - if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps) < 0) + if (qemuBuildMachineCommandLine(cmd, driver, def, qemuCaps) < 0) return NULL; qemuBuildTSEGCommandLine(cmd, def); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b4175a846e..a310ab828e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4730,6 +4730,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def, case VIR_DOMAIN_FEATURE_HPT: case VIR_DOMAIN_FEATURE_HTM: case VIR_DOMAIN_FEATURE_NESTED_HV: + case VIR_DOMAIN_FEATURE_HOST_MODEL_PASSTHROUGH: if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT && !qemuDomainIsPSeries(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, diff --git a/tests/qemuxml2argvdata/pseries-features.args b/tests/qemuxml2argvdata/pseries-features.args index 9fde54b37a..931645cba6 100644 --- a/tests/qemuxml2argvdata/pseries-features.args +++ b/tests/qemuxml2argvdata/pseries-features.args @@ -11,7 +11,8 @@ QEMU_AUDIO_DRV=none \ -name guest \ -S \ -machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\ -cap-hpt-max-page-size=1048576k,cap-htm=on,cap-nested-hv=off \ +cap-hpt-max-page-size=1048576k,cap-htm=on,cap-nested-hv=off,\ +host-model=fake_model \ -m 512 \ -realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ diff --git a/tests/qemuxml2argvdata/pseries-features.xml b/tests/qemuxml2argvdata/pseries-features.xml index 6f7d32b065..8dd222c562 100644 --- a/tests/qemuxml2argvdata/pseries-features.xml +++ b/tests/qemuxml2argvdata/pseries-features.xml @@ -12,6 +12,7 @@ </hpt> <htm state='on'/> <nested-hv state='off'/> + <host-model-passthrough state='on'/> </features> <devices> <emulator>/usr/bin/qemu-system-ppc64</emulator> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index db79301f0e..1d00068d2b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -95,6 +95,30 @@ static virSecretDriver fakeSecretDriver = { .secretUndefine = NULL, }; +static void +testSetHostSysinfo(virSysinfoDefPtr sysinfo) +{ + if (!sysinfo) { + virSysinfoDefFree(driver.hostsysinfo); + return; + } + + driver.hostsysinfo = sysinfo; +} + +static virSysinfoDefPtr +fakeVirSysinfoReadPPC(void) +{ + virSysinfoDefPtr sysinfo = NULL; + + if (VIR_ALLOC(sysinfo) < 0 || VIR_ALLOC(sysinfo->system) < 0) { + virSysinfoDefFree(sysinfo); + return NULL; + } + + ignore_value(VIR_STRDUP(sysinfo->system->serial, "fake_model")); + return sysinfo; +} # define STORAGE_POOL_XML_PATH "storagepoolxml2xmlout/" static const unsigned char fakeUUID[VIR_UUID_BUFLEN] = "fakeuuid"; @@ -1955,12 +1979,16 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VFIO_PCI); + testSetHostSysinfo(fakeVirSysinfoReadPPC()); DO_TEST("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM, QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV, - QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); + QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT, + QEMU_CAPS_MACHINE_PSERIES_HOST_MODEL); + testSetHostSysinfo(NULL); + DO_TEST_FAILURE("pseries-features", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE); DO_TEST_PARSE_ERROR("pseries-features-invalid-machine", NONE); diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml b/tests/qemuxml2xmloutdata/pseries-features.xml index 7e12bc9c03..0d0277280d 100644 --- a/tests/qemuxml2xmloutdata/pseries-features.xml +++ b/tests/qemuxml2xmloutdata/pseries-features.xml @@ -14,6 +14,7 @@ </hpt> <htm state='on'/> <nested-hv state='off'/> + <host-model-passthrough state='on'/> </features> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> -- 2.20.1
On Tue, Oct 08, 2019 at 01:46:57PM -0300, Fabiano Rosas wrote:
Since QEMU v4.0.0, the host's model is no longer automatically exposed to the guest via /proc/device-tree/host-model. There is now a machine option 'host-model' that allows an arbitrary string to be used as the host model [1].
This patch adds support for exposing the real host model string from /proc/cpuinfo (also found on /proc/device-tree/model) to the guest via -machine pseries,host-model=<model> by setting in the domain XML file:
<features> <host_model_passthrough state='on'/> <features/>
I definitely don't want to see that. A mgmt app which used this would be re-exposing the security risk that the QEMU change was intended to fix. That might be ok in some cases, but in the general case we need the ability for the mgmt app to supply the actual data to expose. This is how we deal with SMBIOS - we allow <smbios mode="emulate|host|sysinfo"/>, where 'sysinfo' option lets the mgmt app have full control over what's exposed. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
On Tue, Oct 08, 2019 at 01:46:57PM -0300, Fabiano Rosas wrote:
Since QEMU v4.0.0, the host's model is no longer automatically exposed to the guest via /proc/device-tree/host-model. There is now a machine option 'host-model' that allows an arbitrary string to be used as the host model [1].
This patch adds support for exposing the real host model string from /proc/cpuinfo (also found on /proc/device-tree/model) to the guest via -machine pseries,host-model=<model> by setting in the domain XML file:
<features> <host_model_passthrough state='on'/> <features/>
I definitely don't want to see that. A mgmt app which used this would be re-exposing the security risk that the QEMU change was intended to fix. That might be ok in some cases, but in the general case we need the ability for the mgmt app to supply the actual data to expose. This is how we deal with SMBIOS - we allow <smbios mode="emulate|host|sysinfo"/>, where 'sysinfo' option lets the mgmt app have full control over what's exposed.
Ok, fair enough My ideia of only allowing either the vulnerable behavior or none at all (the default) was to encourage users to find another solution, instead of having them alter the management applications and continue relying on (some sort of) host information exposure. So do you suggest something like <host-model mode="none|sysinfo"/> ? Because having "host" as well (like smbios has) would have the same effect as this patch. Thanks
On Tue, Oct 08, 2019 at 01:46:55PM -0300, Fabiano Rosas wrote:
This series adds support for the -machine,host-model= QEMU option for for pseries guests.
Pseries guests used to have a node (/proc/device-tree/host-model) in device-tree that exposed the host's model string so that guest userspace tools could determine the host machine they were running on.
QEMU used to provide the node by default, but this has been disabled due to security concerns. There is now a machine option (host-model) that allows the user to set an arbitrary string to be used as the host model.
Userspace tools will then be broken unless the user explicitly edits the domain XML with the desired host model to be exposed to the guest. However, having an arbitrary string in the domain XML that needs to be the same for every guest in that machine but different across host machines would generate issues with XML portability and more importantly, migration.
What userspace tool is broken, and in what way ? Re-introducing the host passthrough to satisfy a broken tool is not very attractive because it reintroduces the security flaw that the QEMU change was fixing. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes:
What userspace tool is broken, and in what way ?
The major use case, as far as I know, is from software license managers which use this to determine how much to charge for software. I would have to ask around to know exactly which ones and how they operate.
Re-introducing the host passthrough to satisfy a broken tool is not very attractive because it reintroduces the security flaw that the QEMU change was fixing.
Sure, this is reasonable. I'm just trying to make it less painful for those that depend on the old behavior for some reason. =) Determined folks will probably just use <qemu:commandline> anyway.
On Wed, Oct 09, 2019 at 02:29:37PM -0300, Fabiano Rosas wrote:
Daniel P. Berrangé <berrange@redhat.com> writes:
What userspace tool is broken, and in what way ?
The major use case, as far as I know, is from software license managers which use this to determine how much to charge for software. I would have to ask around to know exactly which ones and how they operate.
Mostly such code should not need to know the real host model / serial information. It should be sufficient to pass through fake data, as long it is consistently fake for all VMs on the same host. Still license managers like this are doomed in a virtualized world as it is trivial to fake the information they're relying on.
Re-introducing the host passthrough to satisfy a broken tool is not very attractive because it reintroduces the security flaw that the QEMU change was fixing.
Sure, this is reasonable. I'm just trying to make it less painful for those that depend on the old behavior for some reason. =)
Determined folks will probably just use <qemu:commandline> anyway.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Fabiano Rosas