This series fixes the following BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1612009 TL;DR: We don't format SEV platform data (PDH, certificate chain,...) into our qemu caps cache which poses a problem after libvirtd restart when we restore from the cache and get a segfault upon issuing virNodeGetSEVInfo. I performed some tests on an AMD machine, but CC'ing Brijesh, he might give it a test too. Erik Skultety (4): tests: sev: Test launch-security with specific QEMU version qemu: Define and use a auto cleanup function with virSEVCapability qemu: Fix probing of AMD SEV support qemu: caps: Format SEV platform data into qemuCaps cache src/conf/domain_capabilities.h | 4 + src/qemu/qemu_capabilities.c | 112 +++++++++++++++++++-- src/qemu/qemu_monitor_json.c | 20 ++-- tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 5 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 6 ++ tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - ...args => launch-security-sev.x86_64-2.12.0.args} | 19 ++-- tests/qemuxml2argvtest.c | 4 +- 8 files changed, 142 insertions(+), 29 deletions(-) rename tests/qemuxml2argvdata/{launch-security-sev.args => launch-security-sev.x86_64-2.12.0.args} (54%) -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list In order to test SEV we need real QEMU capabilities. Ideally, this would be tested with -latest capabilities, however, our capabilities are currently tied to Intel HW, even the 2.12.0 containing SEV were edited by hand, so we can only use that one for now, as splitting the capabilities according to the vendor is a refactor for another day. The need for real capabilities comes from the extended SEV platform data (PDH, cbitpos, etc.) we'll need to cache/parse. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ...ev.args => launch-security-sev.x86_64-2.12.0.args} | 19 ++++++++++++------- tests/qemuxml2argvtest.c | 4 +--- 2 files changed, 13 insertions(+), 10 deletions(-) rename tests/qemuxml2argvdata/{launch-security-sev.args => launch-security-sev.x86_64-2.12.0.args} (54%) diff --git a/tests/qemuxml2argvdata/launch-security-sev.args b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args similarity index 54% rename from tests/qemuxml2argvdata/launch-security-sev.args rename to tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args index XXXXXXX..XXXXXXX 100644 --- a/tests/qemuxml2argvdata/launch-security-sev.args +++ b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args @@ -XXX,XX +XXX,XX @@ USER=test \ LOGNAME=test \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine pc-1.0,accel=kvm,usb=off,dump-guest-core=off,memory-encryption=sev0 \ -m 214 \ +-realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --usb \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\ -bootindex=1 \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\ dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\ -session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 +session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -XXX,XX +XXX,XX @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw", "s390x"); DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x"); - DO_TEST("launch-security-sev", - QEMU_CAPS_KVM, - QEMU_CAPS_SEV_GUEST); + DO_TEST_CAPS_VER("launch-security-sev", "2.12.0"); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Keep with the recent effort of replacing as many explicit *Free functions with their automatic equivalents. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_capabilities.h | 4 ++++ src/qemu/qemu_capabilities.c | 12 ++++-------- src/qemu/qemu_monitor_json.c | 11 ++++++----- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index XXXXXXX..XXXXXXX 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -XXX,XX +XXX,XX @@ # include "internal.h" # include "domain_conf.h" +# include "viralloc.h" typedef const char * (*virDomainCapsValToStr)(int value); @@ -XXX,XX +XXX,XX @@ char * virDomainCapsFormat(virDomainCapsPtr const caps); void virSEVCapabilitiesFree(virSEVCapability *capabilities); + +VIR_DEFINE_AUTOPTR_FUNC(virSEVCapability, virSEVCapabilitiesFree); + #endif /* __DOMAIN_CAPABILITIES_H__ */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -XXX,XX +XXX,XX @@ static int virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, virDomainCapsPtr domCaps) { - virSEVCapability *sev; virSEVCapability *cap = qemuCaps->sevCapabilities; - int ret = -1; + VIR_AUTOPTR(virSEVCapability) sev = NULL; if (!cap) return 0; @@ -XXX,XX +XXX,XX @@ virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, return -1; if (VIR_STRDUP(sev->pdh, cap->pdh) < 0) - goto cleanup; + return -1; if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0) - goto cleanup; + return -1; sev->cbitpos = cap->cbitpos; sev->reduced_phys_bits = cap->reduced_phys_bits; VIR_STEAL_PTR(domCaps->sev, sev); - ret = 0; - cleanup: - virSEVCapabilitiesFree(sev); - return ret; + return 0; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr caps; - virSEVCapability *capability = NULL; - const char *pdh = NULL, *cert_chain = NULL; - unsigned int cbitpos, reduced_phys_bits; + const char *pdh = NULL; + const char *cert_chain = NULL; + unsigned int cbitpos; + unsigned int reduced_phys_bits; + VIR_AUTOPTR(virSEVCapability) capability = NULL; *capabilities = NULL; @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, } if (virJSONValueObjectGetNumberUint(caps, "reduced-phys-bits", - &reduced_phys_bits) < 0) { + &reduced_phys_bits) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-sev-capabilities reply was missing" " 'reduced-phys-bits' field")); @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, ret = 0; cleanup: - virSEVCapabilitiesFree(capability); virJSONValueFree(cmd); virJSONValueFree(reply); -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list So the procedure to detect SEV support works like this: 1) we detect that sev-guest is among the QOM types and set the cap flag 2) we probe the monitor for SEV support - this is tricky, because QEMU with compiled SEV support will always report -object sev-guest and query-sev-capabilities command, that however doesn't mean SEV is supported 3) depending on what the monitor returned, we either keep or clear the capability flag for SEV Commit a349c6c21c6 added an explicit check for "GenericError" in the monitor reply to prevent libvirtd to spam logs about missing 'query-sev-capabilities' command. At the same time though, it returned success in this case which means that we didn't clear the capability flag afterwards and happily formatted SEV into qemuCaps. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_monitor_json.c | 9 +++++---- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, goto cleanup; /* Both -object sev-guest and query-sev-capabilities can be present - * even if SEV is not available */ - if (qemuMonitorJSONHasError(reply, "GenericError")) { - ret = 0; + * even if SEV is not available. We have to check for "GenericError" first, + * in order not to spam libvirtd logs. + * NOTE: We return failure here too so that the capability gets cleared + * later */ + if (qemuMonitorJSONHasError(reply, "GenericError")) goto cleanup; - } if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <flag name='tpm-emulator'/> <flag name='mch'/> <flag name='mch.extended-tseg-mbytes'/> - <flag name='sev-guest'/> <flag name='usb-storage.werror'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Since we're not saving the platform-specific data into a cache, we're not going to populate the structure, which in turn will cause a crash upon calling virNodeGetSEVInfo because of a NULL pointer dereference. Ultimately, we should start caching this data along with host-specific capabilities like NUMA and SELinux stuff into a separate cache, but for the time being, this is a semi-proper fix for a potential crash. Backtrace (requires libvirtd restart to load qemu caps from cache): https://bugzilla.redhat.com/show_bug.cgi?id=1612009 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 100 ++++++++++++++++++++++ tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 5 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 6 ++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -XXX,XX +XXX,XX @@ virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData) } +static int +virQEMUCapsSEVInfoCopy(virSEVCapabilityPtr *dst, + virSEVCapabilityPtr src) +{ + VIR_AUTOPTR(virSEVCapability) tmp = NULL; + + if (VIR_ALLOC(tmp) < 0 || + VIR_STRDUP(tmp->pdh, src->pdh) < 0 || + VIR_STRDUP(tmp->cert_chain, src->cert_chain) < 0) + return -1; + + tmp->cbitpos = src->cbitpos; + tmp->reduced_phys_bits = src->reduced_phys_bits; + + VIR_STEAL_PTR(*dst, tmp); + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -XXX,XX +XXX,XX @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) for (i = 0; i < qemuCaps->ngicCapabilities; i++) ret->gicCapabilities[i] = qemuCaps->gicCapabilities[i]; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && + virQEMUCapsSEVInfoCopy(&ret->sevCapabilities, + qemuCaps->sevCapabilities) < 0) + goto error; + return ret; error: @@ -XXX,XX +XXX,XX @@ virQEMUCapsCachePrivFree(void *privData) } +static int +virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) +{ + VIR_AUTOPTR(virSEVCapability) sev = NULL; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) + return 0; + + if (virXPathBoolean("boolean(./sev)", ctxt) == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV platform data in QEMU " + "capabilities cache")); + return -1; + } + + if (VIR_ALLOC(sev) < 0) + return -1; + + if (virXPathUInt("string(./sev/cbitpos)", ctxt, &sev->cbitpos) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV cbitpos information " + "in QEMU capabilities cache")); + return -1; + } + + if (virXPathUInt("string(./sev/reducedPhysBits)", ctxt, + &sev->reduced_phys_bits) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV reducedPhysBits information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->pdh = virXPathString("string(./sev/pdh)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV pdh information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->cert_chain = virXPathString("string(./sev/certChain)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV certChain information " + "in QEMU capabilities cache")); + return -1; + } + + VIR_STEAL_PTR(qemuCaps->sevCapabilities, sev); + return 0; +} + + /* * Parsing a doc that looks like * @@ -XXX,XX +XXX,XX @@ virQEMUCapsLoadCache(virArch hostArch, } VIR_FREE(nodes); + if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0) + goto cleanup; + virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM); virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU); @@ -XXX,XX +XXX,XX @@ virQEMUCapsFormatCPUModels(virQEMUCapsPtr qemuCaps, } +static void +virQEMUCapsFormatSEVInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf) +{ + virSEVCapabilityPtr sev = virQEMUCapsGetSEVCapabilities(qemuCaps); + + virBufferAddLit(buf, "<sev>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%u</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reducedPhysBits>%u</reducedPhysBits>\n", + sev->reduced_phys_bits); + virBufferEscapeString(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferEscapeString(buf, "<certChain>%s</certChain>\n", + sev->cert_chain); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); +} + + char * virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) { @@ -XXX,XX +XXX,XX @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) emulated ? "yes" : "no"); } + if (qemuCaps->sevCapabilities) + virQEMUCapsFormatSEVInfo(qemuCaps, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); diff --git a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <gic supported='no'/> <vmcoreinfo supported='yes'/> <genid supported='yes'/> - <sev supported='no'/> + <sev supported='yes'> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + </sev> </features> </domainCapabilities> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <machine name='pc-0.11' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.12' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.10' hotplugCpus='yes' maxCpus='255'/> + <sev> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + <pdh>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</pdh> + <certChain>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</certChain> + </sev> </qemuCaps> -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list This series fixes the following BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1612009 TL;DR: We don't format SEV platform data (PDH, certificate chain,...) into our qemu caps cache which poses a problem after libvirtd restart when we restore from the cache and get a segfault upon issuing virNodeGetSEVInfo. Since v1: - reworked patch 3 so that qemuMonitorJSONGetSEVCapabilities returns more values in order to distinguish between error and whether SEV is actually supported - added the missing backtrace to patch 4 - no other patches were changed apart from patch 3 Erik Skultety (4): tests: sev: Test launch-security with specific QEMU version qemu: Define and use a auto cleanup function with virSEVCapability qemu: Fix probing of AMD SEV support qemu: caps: Format SEV platform data into qemuCaps cache src/conf/domain_capabilities.h | 4 + src/qemu/qemu_capabilities.c | 127 +++++++++++++++++++-- src/qemu/qemu_monitor_json.c | 31 +++-- tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 5 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 6 + tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - ...args => launch-security-sev.x86_64-2.12.0.args} | 19 +-- tests/qemuxml2argvtest.c | 4 +- 8 files changed, 164 insertions(+), 33 deletions(-) rename tests/qemuxml2argvdata/{launch-security-sev.args => launch-security-sev.x86_64-2.12.0.args} (54%) -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list In order to test SEV we need real QEMU capabilities. Ideally, this would be tested with -latest capabilities, however, our capabilities are currently tied to Intel HW, even the 2.12.0 containing SEV were edited by hand, so we can only use that one for now, as splitting the capabilities according to the vendor is a refactor for another day. The need for real capabilities comes from the extended SEV platform data (PDH, cbitpos, etc.) we'll need to cache/parse. Signed-off-by: Erik Skultety <eskultet@redhat.com> Acked-by: Peter Krempa <pkrempa@redhat.com> --- ...ev.args => launch-security-sev.x86_64-2.12.0.args} | 19 ++++++++++++------- tests/qemuxml2argvtest.c | 4 +--- 2 files changed, 13 insertions(+), 10 deletions(-) rename tests/qemuxml2argvdata/{launch-security-sev.args => launch-security-sev.x86_64-2.12.0.args} (54%) diff --git a/tests/qemuxml2argvdata/launch-security-sev.args b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args similarity index 54% rename from tests/qemuxml2argvdata/launch-security-sev.args rename to tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args index XXXXXXX..XXXXXXX 100644 --- a/tests/qemuxml2argvdata/launch-security-sev.args +++ b/tests/qemuxml2argvdata/launch-security-sev.x86_64-2.12.0.args @@ -XXX,XX +XXX,XX @@ USER=test \ LOGNAME=test \ QEMU_AUDIO_DRV=none \ /usr/bin/qemu-system-x86_64 \ --name QEMUGuest1 \ +-name guest=QEMUGuest1,debug-threads=on \ -S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -machine pc-1.0,accel=kvm,usb=off,dump-guest-core=off,memory-encryption=sev0 \ -m 214 \ +-realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ -no-acpi \ --usb \ +-boot strict=on \ +-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,\ -bootindex=1 \ +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,policy=0x1,\ dh-cert-file=/tmp/lib/domain--1-QEMUGuest1/dh_cert.base64,\ -session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 +session-file=/tmp/lib/domain--1-QEMUGuest1/session.base64 \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ +resourcecontrol=deny \ +-msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -XXX,XX +XXX,XX @@ mymain(void) DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw", "s390x"); DO_TEST_CAPS_ARCH_LATEST("vhost-vsock-ccw-auto", "s390x"); - DO_TEST("launch-security-sev", - QEMU_CAPS_KVM, - QEMU_CAPS_SEV_GUEST); + DO_TEST_CAPS_VER("launch-security-sev", "2.12.0"); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Keep with the recent effort of replacing as many explicit *Free functions with their automatic equivalents. Signed-off-by: Erik Skultety <eskultet@redhat.com> Acked-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_capabilities.h | 4 ++++ src/qemu/qemu_capabilities.c | 12 ++++-------- src/qemu/qemu_monitor_json.c | 11 ++++++----- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index XXXXXXX..XXXXXXX 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -XXX,XX +XXX,XX @@ # include "internal.h" # include "domain_conf.h" +# include "viralloc.h" typedef const char * (*virDomainCapsValToStr)(int value); @@ -XXX,XX +XXX,XX @@ char * virDomainCapsFormat(virDomainCapsPtr const caps); void virSEVCapabilitiesFree(virSEVCapability *capabilities); + +VIR_DEFINE_AUTOPTR_FUNC(virSEVCapability, virSEVCapabilitiesFree); + #endif /* __DOMAIN_CAPABILITIES_H__ */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -XXX,XX +XXX,XX @@ static int virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, virDomainCapsPtr domCaps) { - virSEVCapability *sev; virSEVCapability *cap = qemuCaps->sevCapabilities; - int ret = -1; + VIR_AUTOPTR(virSEVCapability) sev = NULL; if (!cap) return 0; @@ -XXX,XX +XXX,XX @@ virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, return -1; if (VIR_STRDUP(sev->pdh, cap->pdh) < 0) - goto cleanup; + return -1; if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0) - goto cleanup; + return -1; sev->cbitpos = cap->cbitpos; sev->reduced_phys_bits = cap->reduced_phys_bits; VIR_STEAL_PTR(domCaps->sev, sev); - ret = 0; - cleanup: - virSEVCapabilitiesFree(sev); - return ret; + return 0; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, virJSONValuePtr cmd; virJSONValuePtr reply = NULL; virJSONValuePtr caps; - virSEVCapability *capability = NULL; - const char *pdh = NULL, *cert_chain = NULL; - unsigned int cbitpos, reduced_phys_bits; + const char *pdh = NULL; + const char *cert_chain = NULL; + unsigned int cbitpos; + unsigned int reduced_phys_bits; + VIR_AUTOPTR(virSEVCapability) capability = NULL; *capabilities = NULL; @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, } if (virJSONValueObjectGetNumberUint(caps, "reduced-phys-bits", - &reduced_phys_bits) < 0) { + &reduced_phys_bits) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-sev-capabilities reply was missing" " 'reduced-phys-bits' field")); @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, ret = 0; cleanup: - virSEVCapabilitiesFree(capability); virJSONValueFree(cmd); virJSONValueFree(reply); -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list So the procedure to detect SEV support works like this: 1) we detect that sev-guest is among the QOM types and set the cap flag 2) we probe the monitor for SEV support - this is tricky, because QEMU with compiled SEV support will always report -object sev-guest and query-sev-capabilities command, that however doesn't mean SEV is supported 3) depending on what the monitor returned, we either keep or clear the capability flag for SEV Commit a349c6c21c6 added an explicit check for "GenericError" in the monitor reply to prevent libvirtd to spam logs about missing 'query-sev-capabilities' command. At the same time though, it returned success in this case which means that we didn't clear the capability flag afterwards and happily formatted SEV into qemuCaps. Therefore, adjust all the relevant callers to handle -1 on errors, 0 on SEV being unsupported and 1 on SEV being supported. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_capabilities.c | 15 +++++++++++---- src/qemu/qemu_monitor_json.c | 20 ++++++++++++++++---- tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml | 1 - 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -XXX,XX +XXX,XX @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, } +/* Returns -1 on error, 0 if SEV is not supported, 1 if SEV is supported */ static int virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { + int rc = -1; virSEVCapability *caps = NULL; - if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0) - return -1; + if ((rc = qemuMonitorGetSEVCapabilities(mon, &caps)) <= 0) + return rc; virSEVCapabilitiesFree(qemuCaps->sevCapabilities); qemuCaps->sevCapabilities = caps; - return 0; + return rc; } @@ -XXX,XX +XXX,XX @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, /* Probe for SEV capabilities */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { - if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) + int rc = virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon); + + if (rc < 0) + goto cleanup; + + if (rc == 0) virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, } +/** + * qemuMonitorJSONGetSEVCapabilities: + * @mon: qemu monitor object + * @capabilities: pointer to pointer to a SEV capability structure to be filled + * + * This function queries and fills in AMD's SEV platform-specific data. + * Note that from QEMU's POV both -object sev-guest and query-sev-capabilities + * can be present even if SEV is not available, which basically leaves us with + * checking for JSON "GenericError" in order to differentiate between + * compiled-in support and actual SEV support on the platform. + * + * Returns -1 on error, 0 if SEV is not supported, and 1 if SEV is supported on + * the platform. + */ int qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, virSEVCapability **capabilities) @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - /* Both -object sev-guest and query-sev-capabilities can be present - * even if SEV is not available */ + /* QEMU has only compiled-in support of SEV */ if (qemuMonitorJSONHasError(reply, "GenericError")) { ret = 0; goto cleanup; @@ -XXX,XX +XXX,XX @@ qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, capability->cbitpos = cbitpos; capability->reduced_phys_bits = reduced_phys_bits; VIR_STEAL_PTR(*capabilities, capability); - ret = 0; - + ret = 1; cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <flag name='tpm-emulator'/> <flag name='mch'/> <flag name='mch.extended-tseg-mbytes'/> - <flag name='sev-guest'/> <flag name='usb-storage.werror'/> <flag name='egl-headless'/> <flag name='vfio-pci.display'/> -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Since we're not saving the platform-specific data into a cache, we're not going to populate the structure, which in turn will cause a crash upon calling virNodeGetSEVInfo because of a NULL pointer dereference. Ultimately, we should start caching this data along with host-specific capabilities like NUMA and SELinux stuff into a separate cache, but for the time being, this is a semi-proper fix for a potential crash. Backtrace (requires libvirtd restart to load qemu caps from cache): #0 qemuGetSEVInfoToParams #1 qemuNodeGetSEVInfo #2 virNodeGetSEVInfo #3 remoteDispatchNodeGetSevInfo #4 remoteDispatchNodeGetSevInfoHelper #5 virNetServerProgramDispatchCall #6 virNetServerProgramDispatch #7 virNetServerProcessMsg #8 virNetServerHandleJob #9 virThreadPoolWorker #10 virThreadHelper https: //bugzilla.redhat.com/show_bug.cgi?id=1612009 Signed-off-by: Erik Skultety <eskultet@redhat.com> Acked-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 100 ++++++++++++++++++++++ tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml | 5 +- tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 6 ++ 3 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index XXXXXXX..XXXXXXX 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -XXX,XX +XXX,XX @@ virQEMUCapsHostCPUDataClear(virQEMUCapsHostCPUDataPtr cpuData) } +static int +virQEMUCapsSEVInfoCopy(virSEVCapabilityPtr *dst, + virSEVCapabilityPtr src) +{ + VIR_AUTOPTR(virSEVCapability) tmp = NULL; + + if (VIR_ALLOC(tmp) < 0 || + VIR_STRDUP(tmp->pdh, src->pdh) < 0 || + VIR_STRDUP(tmp->cert_chain, src->cert_chain) < 0) + return -1; + + tmp->cbitpos = src->cbitpos; + tmp->reduced_phys_bits = src->reduced_phys_bits; + + VIR_STEAL_PTR(*dst, tmp); + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -XXX,XX +XXX,XX @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) for (i = 0; i < qemuCaps->ngicCapabilities; i++) ret->gicCapabilities[i] = qemuCaps->gicCapabilities[i]; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && + virQEMUCapsSEVInfoCopy(&ret->sevCapabilities, + qemuCaps->sevCapabilities) < 0) + goto error; + return ret; error: @@ -XXX,XX +XXX,XX @@ virQEMUCapsCachePrivFree(void *privData) } +static int +virQEMUCapsParseSEVInfo(virQEMUCapsPtr qemuCaps, xmlXPathContextPtr ctxt) +{ + VIR_AUTOPTR(virSEVCapability) sev = NULL; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) + return 0; + + if (virXPathBoolean("boolean(./sev)", ctxt) == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV platform data in QEMU " + "capabilities cache")); + return -1; + } + + if (VIR_ALLOC(sev) < 0) + return -1; + + if (virXPathUInt("string(./sev/cbitpos)", ctxt, &sev->cbitpos) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV cbitpos information " + "in QEMU capabilities cache")); + return -1; + } + + if (virXPathUInt("string(./sev/reducedPhysBits)", ctxt, + &sev->reduced_phys_bits) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or malformed SEV reducedPhysBits information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->pdh = virXPathString("string(./sev/pdh)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV pdh information " + "in QEMU capabilities cache")); + return -1; + } + + if (!(sev->cert_chain = virXPathString("string(./sev/certChain)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing SEV certChain information " + "in QEMU capabilities cache")); + return -1; + } + + VIR_STEAL_PTR(qemuCaps->sevCapabilities, sev); + return 0; +} + + /* * Parsing a doc that looks like * @@ -XXX,XX +XXX,XX @@ virQEMUCapsLoadCache(virArch hostArch, } VIR_FREE(nodes); + if (virQEMUCapsParseSEVInfo(qemuCaps, ctxt) < 0) + goto cleanup; + virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_KVM); virQEMUCapsInitHostCPUModel(qemuCaps, hostArch, VIR_DOMAIN_VIRT_QEMU); @@ -XXX,XX +XXX,XX @@ virQEMUCapsFormatCPUModels(virQEMUCapsPtr qemuCaps, } +static void +virQEMUCapsFormatSEVInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf) +{ + virSEVCapabilityPtr sev = virQEMUCapsGetSEVCapabilities(qemuCaps); + + virBufferAddLit(buf, "<sev>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%u</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reducedPhysBits>%u</reducedPhysBits>\n", + sev->reduced_phys_bits); + virBufferEscapeString(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferEscapeString(buf, "<certChain>%s</certChain>\n", + sev->cert_chain); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); +} + + char * virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) { @@ -XXX,XX +XXX,XX @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps) emulated ? "yes" : "no"); } + if (qemuCaps->sevCapabilities) + virQEMUCapsFormatSEVInfo(qemuCaps, &buf); + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); diff --git a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml +++ b/tests/domaincapsschemadata/qemu_2.12.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <gic supported='no'/> <vmcoreinfo supported='yes'/> <genid supported='yes'/> - <sev supported='no'/> + <sev supported='yes'> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + </sev> </features> </domainCapabilities> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index XXXXXXX..XXXXXXX 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -XXX,XX +XXX,XX @@ <machine name='pc-0.11' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.12' hotplugCpus='yes' maxCpus='255'/> <machine name='pc-0.10' hotplugCpus='yes' maxCpus='255'/> + <sev> + <cbitpos>47</cbitpos> + <reducedPhysBits>1</reducedPhysBits> + <pdh>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</pdh> + <certChain>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</certChain> + </sev> </qemuCaps> -- 2.14.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list