On Mon, Feb 26, 2018 at 11:53:33 -0600, Brijesh Singh wrote: This patch fails both 'make check' and 'make syntax-check' please make sure that for the next posting you send a series where every single patch passes both test suites. This structure contains also some pointers which would be leaked. I think you can add a capability bit for the presence of the query-sev-capabilities command and call this function based on this fact. Making this whole function depend on the presence of the new command will greatly simplify your pain with the qemucapabilitiestest which this will inflict. If you make sure that only new qemus call this command you won't have to fix all older test suites. Also note that you'll need to add a test case for the new qemu with the command supported so we can test this capability detection. 'VIR_ALLOC' should be enough This field should not be necessary. The presence of the structure itself should be good enough in this case.

Extend hypervisor capabilities to include sev feature. When available, hypervisor supports launching an encrypted VM on AMD platform. The sev feature tag provides additional details like platform diffie-hellman key and certificate chain which can be used by the guest owner to establish a cryptographic session with the SEV firmware to negotiate keys used for attestation or to provide secret during launch. Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com&gt; --- docs/formatdomaincaps.html.in | 31 +++++++++++++++++++++++++++++++ docs/schemas/domaincaps.rng | 10 ++++++++++ src/conf/domain_capabilities.c | 19 +++++++++++++++++++ src/conf/domain_capabilities.h | 11 +++++++++++ src/qemu/qemu_capabilities.c | 41 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 111 insertions(+), 1 deletion(-) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 6bfcaf61caae..8f833477772c 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -417,6 +417,12 @@ &lt;value&gt;3&lt;/value&gt; &lt;/enum&gt; &lt;/gic&gt; + &lt;sev supported='yes'&gt; + &lt;pdh&gt; &lt;/pdh&gt; + &lt;cert-chain&gt; &lt;/cert-chain&gt; + &lt;cbitpos&gt; &lt;/cbitpos&gt; + &lt;reduced-phys-bits&gt; &lt;/reduced-phys-bits&gt; + &lt;/sev&gt; &lt;/features&gt; &lt;/domainCapabilities&gt; </pre> @@ -441,5 +447,30 @@ <code>gic</code> element.</dd> </dl> + <h4><a id="elementsSEV">SEV capabilities</a></h4> + + <p>AMD Secure Encrypted Virtualization (SEV) capabilities are exposed under + the <code>sev</code> element. + SEV is an extension to the AMD-V architecture which supports running + virtual machines (VMs) under the control of a hypervisor. When supported, + guest owner can create a VM whose memory contents will be transparently + encrypted with a key unique to that VM. + </p> + + <dl> + <dt><code>pdh</code></dt> + <dd>Platform diffie-hellman key, which can be exported to remote entities + which wish to establish a secure transport context with the SEV platform + in order to transmit data securely. The key is encoded in base64</dd> + <dt><code>cert-chain</code></dt> + <dd> Platform certificate chain -- which includes platform endorsement key + (PEK), owners certificate authory (OCA) and chip endorsement key (CEK). + The certificate chain is encoded in base64.</dd> + <dt><code>cbitpos</code></dt> + <dd> C-bit position in page-table entry</dd> + <dt><code>reduced-phys-bits</code></dt> + <dd> Physical Address bit reduction</dd> + </dl> + </body> </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 39053181eb9a..6ce8d296c703 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -184,6 +184,16 @@ </element> </define> + <define name='sev'> + <element name='sev'> + <ref name='supported'/> + <ref name='pdh'/> + <ref name='cert-chain'/> + <ref name='cbitpos'/> + <ref name='reduced-phys-bits'/> + </element> + </define> + <define name='value'> <zeroOrMore> <element name='value'> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f7d9be50f82d..6a7a30877042 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -549,6 +549,24 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf, FORMAT_EPILOGUE(gic); } +static void +virDomainCapsFeatureSEVFormat(virBufferPtr buf, + virDomainCapsFeatureSEVPtr const sev) +{ + FORMAT_PROLOGUE(sev); + + if (sev->supported) { + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reduced-phys-bits>%d</reduced-phys-bits>\n", + sev->reduced_phys_bits); + virBufferAsprintf(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferAsprintf(buf, "<cert-chain>%s</cert-chain>\n", + sev->cert_chain); + } + + FORMAT_EPILOGUE(sev); +} + char * virDomainCapsFormat(virDomainCapsPtr const caps) @@ -587,6 +605,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps) virBufferAdjustIndent(&buf, 2); virDomainCapsFeatureGICFormat(&buf, &caps->gic); + virDomainCapsFeatureSEVFormat(&buf, &caps->sev); virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</features>\n"); diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index e13a7fd6ba1b..aed5ec28e9cc 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -102,6 +102,16 @@ struct _virDomainCapsFeatureGIC { virDomainCapsEnum version; /* Info about virGICVersion */ }; +typedef struct _virDomainCapsFeatureSEV virDomainCapsFeatureSEV; +typedef virDomainCapsFeatureSEV *virDomainCapsFeatureSEVPtr; +struct _virDomainCapsFeatureSEV { + bool supported; + char *pdh; /* host platform-diffie hellman key */ + char *cert_chain; /* PDH certificate chain */ + int cbitpos; + int reduced_phys_bits; +}; + typedef enum { VIR_DOMCAPS_CPU_USABLE_UNKNOWN, VIR_DOMCAPS_CPU_USABLE_YES, @@ -171,6 +181,7 @@ struct _virDomainCaps { /* add new domain devices here */ virDomainCapsFeatureGIC gic; + virDomainCapsFeatureSEV sev; /* add new domain features here */ }; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 2c680528deb8..ee8c542679eb 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5880,6 +5880,44 @@ virQEMUCapsSupportsGICVersion(virQEMUCapsPtr qemuCaps, return false; } +/** + * virQEMUCapsFillDomainFeatureSEVCaps: + * @qemuCaps: QEMU capabilities + * @domCaps: domain capabilities + * + * Take the information about SEV capabilities that has been obtained + * using the 'query-sev-capabilities' QMP command and stored in @qemuCaps + * and convert it to a form suitable for @domCaps. + * + * Returns: 0 on success, <0 on failure + */ +static int +virQEMUCapsFillDomainFeatureSEVCaps(virQEMUCapsPtr qemuCaps, + virDomainCapsPtr domCaps) +{ + virDomainCapsFeatureSEVPtr sev = &domCaps->sev; + virSEVCapability *cap = qemuCaps->sevCapabilities; + + if (!cap) + return 0; + + sev->supported = cap->sev; + + if (VIR_STRDUP(sev->pdh, cap->pdh) < 0) + goto failed; + + if (VIR_STRDUP(sev->cert_chain, cap->cert_chain) < 0) + goto failed; + + sev->cbitpos = cap->cbitpos; + sev->reduced_phys_bits = cap->reduced_phys_bits; + + return 0; +failed: + sev->supported = false; + return 0; +} + /** * virQEMUCapsFillDomainFeatureGICCaps: @@ -5958,7 +5996,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, virQEMUCapsFillDomainDeviceGraphicsCaps(qemuCaps, graphics) < 0 || virQEMUCapsFillDomainDeviceVideoCaps(qemuCaps, video) < 0 || virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || - virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) + virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0 || + virQEMUCapsFillDomainFeatureSEVCaps(qemuCaps, domCaps)) return -1; return 0; } -- 2.14.3

On 02/27/2018 02:15 AM, Peter Krempa wrote: How about something like: <memory-encryption type='sev'> <pdh> ..</pdh> .... .... </memory-encryption> if other manufacture implements memory encryption then we can introduce a new type to handle new memory encryption feature. The inputs to the memory encryption type is going to be vendor-specific. I will add some more details to clarify this. I must admit that I am new to libvirt hence need some help. Sorry I am not able to follow your this comment. Do I need to update domaincap.rng or not ? If yes, where do I need to define the names so that we don't break the schema test. Any tips is appreciated. thanks OK, actually I was taking similar approach as 'gic' support -- which leaves the empty element in case if gic is not present. Will now take your recommendation and skip it completely. Yes they are same structure, will reuse it. The function was modeled after gic feature which always returns 0 regardless whether gic is available or not. Similarly we don't want to fail just because SEV feature is not available. I can look into improving this.

Secure Encrypted Virtualization (sev) element is used to provide the guest owners input parameters used for creating an encrypted VM using AMD SEV feature. SEV feature supports running encrypted VM under the control of KVM. Encrypted VMs have their pages (code and data) secured such that only the guest itself has access to the unencrypted version. Each encrypted VM is associated with a unique encryption key; if its data is accessed to a different entity using a different key the encrypted guests data will be incorrectly decrypted, leading to unintelligible data. QEMU >= 2.12 provides 'sev-guest' object which supports launching encrypted VMs. A typical command line # $QEMU ... \ -machine memory-encryption=sev0 \ -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 \ ... Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com&gt; --- docs/formatdomain.html.in | 71 +++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 18 +++++++++++ src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 230 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6fd2189cd2f4..d18e3fb1d976 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8195,6 +8195,77 @@ qemu-kvm -net nic,model=? /dev/null <p>Note: DEA/TDEA is synonymous with DES/TDES.</p> + <h3><a id="sev">Secure Encrypted Virtualization (SEV)</a></h3> + + <p> + The contents of the <code>sev</code> element is used to provide the + guest owners input used for creating an encrypted VM using the AMD + Secure Encrypted Virtualization (SEV) feature. + + SEV is an extension to the AMD-V architecture which supports running + encrypted virtual machine (VMs) under the control of KVM. Encrypted + VMs have their pages (code and data) secured such that only the guest + itself has access to the unencrypted version. Each encrypted VM is + associated with a unique encryption key; if its data is accessed to a + different entity using a different key the encrypted guests data will + be incorrectly decrypted, leading to unintelligible data. + </p> + <pre> +&lt;domain&gt; + ... + &lt;sev&gt; + &lt;policy&gt; 1 &lt;/policy&gt; + &lt;cbitpos&gt; 47 &lt;/cbitpos&gt; + &lt;reduced-phys-bits&gt; 5 &lt;/reduced-phys-bits&gt; + &lt;session&gt; ... &lt;/session&gt; + &lt;dh-cert&gt; ... &lt;/dh&gt; + &lt;/sev&gt; + ... +&lt;/domain&gt; +</pre> + + <p> + A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be nested + within the <code>sev</code> element. + </p> + <dl> + <dt><code>cbitpos</code></dt> + <dd>The <code>cbitpos</code> attribute provides the C-bit (aka encryption bit) + location in guest page table entry. The value of <code>cbitpos</code> is + hypervisor dependent and can be obtained through the <code>sev</code> element + from domaincapabilities. + </dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>The <code>reduced-phys-bits</code> attribute provides the physical + address bit reducation. Similar to <code>cbitpos</code> the value of <code> + reduced-phys-bit</code> is hypervisor dependent and can be obtained + through the <code>sev</code> element from domaincapabilities. + </dd> + <dt><code>policy</code></dt> + <dd>The <code>policy</code> attribute provides the guest policy which must + be maintained by the SEV firmware. This policy is enforced by the firmware + and restricts what configuration and operational commands can be performed + on this guest by the hypervisor. The guest policy provided during guest + launch is bound to the guest and cannot be changed throughout the lifetime + of the guest. The policy is also transmitted during snapshot and migration + flows and enforced on the destination platform. + </dd> + <dt><code>dh-cert</code></dt> + <dd>The <code>dh-cert</code> attribute provides the guest owners public + Diffie-Hellman (DH) key. The key is used to negotiate a master secret + key between the SEV firmware and guest owner. This master secret key is + then used to establish a trusted channel between SEV firmware and guest + owner. The value must be encoded in base64. + </dd> + <dt><code>session</code></dt> + <dd>The <code>session</code> attribute provides the guest owners session + blob defined in SEV API spec. The value must be encoded in base64. + </dd> + </dl> + + <p>Note: More information about <code>policy</code> bit definition, <code> + dh</code> and <code>session</code> is available in SEV API spec.</p> + <h2><a id="examples">Example configs</a></h2> <p> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d96b012b98f0..4c9921b5dca6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15539,6 +15539,61 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, return ret; } +static void +virDomainSevDefFree(virDomainSevDefPtr def) +{ + VIR_FREE(def->dh_cert); + VIR_FREE(def->session); + + VIR_FREE(def); +} + +static virDomainSevDefPtr +virDomainSevDefParseXML(xmlNodePtr sevNode, + xmlXPathContextPtr ctxt) +{ + char *tmp = NULL; + xmlNodePtr save = ctxt->node; + virDomainSevDefPtr def; + unsigned long policy; + + ctxt->node = sevNode; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if ((tmp = virXPathString("string(./dh-cert)", ctxt))) { + if (VIR_STRDUP(def->dh_cert, tmp) < 0) + goto error; + + VIR_FREE(tmp); + } + + if ((tmp = virXPathString("string(./session)", ctxt))) { + if (VIR_STRDUP(def->session, tmp) < 0) + goto error; + + VIR_FREE(tmp); + } + + if (virXPathULongHex("string(./policy)", ctxt, &policy) == 0) { + def->policy = policy; + } else { + def->policy = -1; + } + + virXPathInt("string(./cbitpos)", ctxt, &def->cbitpos); + virXPathInt("string(./reduced-phys-bits)", ctxt, &def->reduced_phys_bits); + + ctxt->node = save; + return def; + +error: + VIR_FREE(tmp); + virDomainSevDefFree(def); + ctxt->node = save; + return NULL; +} static virDomainMemoryDefPtr virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, @@ -20212,6 +20267,15 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; VIR_FREE(nodes); + /* Check for SEV feature */ + if ((n = virXPathNodeSet("./sev", ctxt, &nodes)) < 0) + goto error; + + if (n) { + def->sev = virDomainSevDefParseXML(nodes[0], ctxt); + VIR_FREE(nodes); + } + /* analysis of memory devices */ if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 368f16f3fbf9..f0f267b28f40 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -142,6 +142,9 @@ typedef virDomainPanicDef *virDomainPanicDefPtr; typedef struct _virDomainMemoryDef virDomainMemoryDef; typedef virDomainMemoryDef *virDomainMemoryDefPtr; +typedef struct _virDomainSevDef virDomainSevDef; +typedef virDomainSevDef *virDomainSevDefPtr; + /* forward declarations virDomainChrSourceDef, required by * virDomainNetDef */ @@ -2289,6 +2292,18 @@ struct _virDomainKeyWrapDef { int dea; /* enum virTristateSwitch */ }; +typedef struct _virDomainSevDef virDomainSevDef; +typedef virDomainSevDef *virDomainSevDefPtr; + +struct _virDomainSevDef { + char *dh_cert; + char *session; + int policy; + int cbitpos; + int reduced_phys_bits; +}; + + typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL, @@ -2454,6 +2469,9 @@ struct _virDomainDef { virDomainKeyWrapDefPtr keywrap; + /* SEV-specific domain */ + virDomainSevDefPtr sev; + /* Application-specific custom metadata */ xmlNodePtr metadata; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fa0aa5d5c3d4..653bbe154332 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9663,6 +9663,80 @@ qemuBuildTPMCommandLine(virCommandPtr cmd, return 0; } +static char * +qemuBuildSevCreateFile(const virDomainDef *def, const char *name, char *data) +{ + char *base = virGetUserConfigDirectory(); + char *configDir, *configFile; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(def->uuid, uuidstr); + + if (virAsprintf(&configDir, "%s/sev/%s", base, uuidstr) < 0) + goto error; + VIR_FREE(base); + + if (virFileMakePathWithMode(configDir, S_IRWXU) < 0) { + virReportSystemError(errno, _("cannot create config directory '%s'"), + configDir); + goto error; + } + + if (!(configFile = virFileBuildPath(configDir, name, ".base64"))) + goto error; + + if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) { + virReportSystemError(errno, _("failed to write data to config '%s'"), + configFile); + goto error; + } + + return configFile; + +error: + return NULL; +} + +static int +qemuBuildSevCommandLine(virCommandPtr cmd, + const virDomainDef *def) +{ + virDomainSevDefPtr sev = def->sev; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBuffer obj = VIR_BUFFER_INITIALIZER; + char *dh_cert_file = NULL; + char *session_file = NULL; + + /* qemu accepts DH and session blob as file, create a temporary file */ + if (sev->dh_cert && + !(dh_cert_file = qemuBuildSevCreateFile(def, "dh_cert", sev->dh_cert))) + return -1; + + if (sev->session && + !(session_file = qemuBuildSevCreateFile(def, "session", sev->session))) + return -1; + + virCommandAddArg(cmd, "-machine"); + virBufferAddLit(&buf, "memory-encryption=sev0"); + virCommandAddArgBuffer(cmd, &buf); + + virCommandAddArg(cmd, "-object"); + virBufferAddLit(&obj, "sev-guest,id=sev0"); + if (sev->policy > 0) + virBufferAsprintf(&obj, ",policy=0x%x", sev->policy); + virBufferAsprintf(&obj, ",cbitpos=%d", sev->cbitpos); + virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits); + if (dh_cert_file) + virBufferAsprintf(&obj, ",dh-cert-file=%s", dh_cert_file); + if (session_file) + virBufferAsprintf(&obj, ",session-file=%s", session_file); + virCommandAddArgBuffer(cmd, &obj); + + VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d dh=%s session=%s", + sev->policy, sev->cbitpos, sev->reduced_phys_bits, dh_cert_file, + session_file); + return 0; +} static int qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd, @@ -10108,6 +10182,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0) goto error; + if (def->sev && qemuBuildSevCommandLine(cmd, def) < 0) + goto error; + if (snapshot) virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); -- 2.14.3

On 02/27/2018 02:34 AM, Peter Krempa wrote: To protect the confidentiality of an SEV protected guest while in transit, its memory contents get encrypted with a key that can be recovered by the next platform to execute it. The SEV firmware provides the interfaces to support this protection: SEND_START, SEND_UPDATE_DATA, and SEND_FINISH for the re-encrypting the memory contents in transit. See SEV API spec [1] Appendix A (migration flow) [1] https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf Please note that we do not send the keys from source to destination, instead data gets wrapped with transport key and unwrapped on destination. The transport keys are negotiated before we start the migration. I have not pushed any migration patches yet -- the patches will come after base SEV is accepted. Currently, we add a migration blocker in qemu for the SEV guest. Do we need to do anything special in libvirt, or having migration blocker in QEMU is good enough ? I didn't know about the virDomainMemoryPeek module but from the name it sounds like it read the guest memory contents, does this go through QEMU monitor command (e.g x/xp) or it peeks directly into guest memory ? If request goes through the QEMU monitor and sev guest policy allows debugging then QEMU monitor command will able to use SEV debug APIs to access the guest memory. Sure, I will do that. Inputs to the memory encryption engine is vendor specific, see my previous comment, if the below approach works then I am all for it <memory-encryption type='sev'> <dh> ...</dh> ... ... </memory-encryption> Yes, this is platform/hypervisor dependent. It will be same as what is returned from capabilities. The reason why I expose this to user so that we can handle migration cases in which source and destination HV/platform have different C-bit positions. I am not clear on how libvirt VM migration works, I am thinking that in migration case we simply send the source XML file to destination and ask libvirt to create a guest using the newly provided XML documents. If this is case, we should be providing the sources C-bit position. While creating the SEV guest we validate the requested C-bit position and if we can't work with requested c-bit position then we fail to create a guest. In other words, we if set cbitspos equal to what is present in capabilities then we are not communicating the C-bit properly. I am open to other suggestions. See my comments, the value is host family dependent and if we are migrating across different AMD family then its value will change. I can provide the link of the complete documentation and if needed then I can document here. From x86 software point of view this is a blob of input which comes from the guest owner and must be passed as-is. The blob definition may change from one SEV firmware to another hence I don't see any reason for documenting this. I will provide link to SEV spec in case if someone want to find the details on blob and what exactly it contains etc etc. Will do, so far the link has been static and I am okay with putting it either in here or commit description or both. Got it. Sure. Okay will do so. Will do that. Will do. Will look at those tests. -Brijesh

On Mon, Feb 26, 2018 at 11:53:35AM -0600, Brijesh Singh wrote: In general we'd expect to see additions to the test suite for any XML changes. eg a qemuxml2xmltest and qemuxml2argvtest addition. Minor nitpick - since this inheranted SEV specific, I think we could do with having a generic top level element with a type=sev. eg <launch-security type="sev"> <policy>...</policy> <cbitpos>..</cbitpos> ...etc... </launch> then we can plug in custom data if other vendors invent competing solutions to AMD's SEV. 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 :|

On Tue, Feb 27, 2018 at 11:07:25AM -0600, Brijesh Singh wrote: Memory encryption is a very specific feature. It occurs to me that there could in future be other features that use launch time validation, that are not memory encryption related. 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 :|

On Tue, Feb 27, 2018 at 05:15:30PM +0000, Daniel P. Berrangé wrote: <launch-security> is IMHO still rather specific than generic, since we might need to enable features in the future, which might/might no rely on security, but add additional attributes to the launch validation, in which case I think going for something like <launch-control> or simply <launch> and having a structure similar to the one below: By having the separate <sev> element you can make the sub-elements depend on this parent element, since you can't expect other vendors to favour <cbitpos> which add burden to the documentation to make it clear. Of course, the price you pay for this is a more complex XML structure. <launch> <security> <sev> <sev_specific_elements/> </sev> </security> <other_security_unrelated_validation_options/> </launch> Erik

The virDomainGetSevVmMeasurement() can be used to retrieve the measurement of encrypted VM launched using AMD SEV feature. The measurement is a signature of the memory contents that can be sent to the guest owner as an attestation that the memory was encrypted correctly by the firmware before booting the guest. Signed-off-by: Xiaogang Chen <Xiaogang.Chen(a)amd.com&gt; Signed-off-by: Brijesh Singh <brijesh.singh(a)amd.com&gt; --- include/libvirt/libvirt-domain.h | 4 +++ src/driver-hypervisor.h | 4 +++ src/libvirt-domain.c | 41 +++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 8 ++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ src/remote/remote_driver.c | 3 ++- src/remote/remote_protocol.x | 17 +++++++++++- 11 files changed, 171 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 4048acf38aaf..c0bcfea4723c 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4756,4 +4756,8 @@ int virDomainSetLifecycleAction(virDomainPtr domain, unsigned int action, unsigned int flags); +char * +virDomainGetSevVmMeasurement(virDomainPtr domain, + unsigned int flags); + #endif /* __VIR_LIBVIRT_DOMAIN_H__ */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index ce0e2b252552..73edcd8f059f 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1283,6 +1283,9 @@ typedef int unsigned int action, unsigned int flags); +typedef char * +(*virDrvDomainGetSevVmMeasurement)(virDomainPtr dommain, + unsigned int flags); typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1528,6 +1531,7 @@ struct _virHypervisorDriver { virDrvDomainSetVcpu domainSetVcpu; virDrvDomainSetBlockThreshold domainSetBlockThreshold; virDrvDomainSetLifecycleAction domainSetLifecycleAction; + virDrvDomainGetSevVmMeasurement domainGetSevVmMeasurement; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index eaec0979ad49..f285a3121548 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12095,3 +12095,44 @@ int virDomainSetLifecycleAction(virDomainPtr domain, virDispatchError(domain->conn); return -1; } + +/** + * virDomainGetSevVmMeasurement: + * @domain: pointer to domain object + * @flags: currently unused, pass 0 + * + * Get launch measurement of SEV guest VM + * + * Returns a measurement string, or NULL in case of error. + */ +char * +virDomainGetSevVmMeasurement(virDomainPtr domain, + unsigned int flags) +{ + virConnectPtr conn; + VIR_DOMAIN_DEBUG(domain, "flags=0x%x", flags); + + virResetLastError(); + + virCheckDomainReturn(domain, NULL); + conn = domain->conn; + + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainGetSevVmMeasurement) { + char *ret; + + ret = conn->driver->domainGetSevVmMeasurement(domain, + flags); + if (!ret) + goto error; + + return ret; + } + + virReportUnsupportedError(); + +error: + virDispatchError(domain->conn); + return NULL; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0dbc7b..6e956d965a26 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -783,6 +783,7 @@ LIBVIRT_3.9.0 { LIBVIRT_4.1.0 { global: virStoragePoolLookupByTargetPath; + virDomainGetSevVmMeasurement; } LIBVIRT_3.9.0; # .... define new API here using predicted next version number .... diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 313d730c791f..852d1f0fd2f7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21254,6 +21254,62 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, return ret; } +static char * +qemuDomainGetSevVmMeasurement(virDomainPtr dom, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + char *ret = NULL, *tmp; + + virCheckFlags(0, NULL); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + if (virDomainGetSevVmMeasurementEnsureACL(dom->conn, vm->def) < 0){ + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("get sev vm measurement is not allowed")); + goto cleanup; + } + + if (vm->def->sev) { + goto endjob; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("domain is not SEV guest")); + } + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + VIR_DEBUG("query sev launch measurement"); + if(!(tmp = qemuMonitorGetSevMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon))){ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to get measurement")); + goto endjob; + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + ret = tmp; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, @@ -21474,6 +21530,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainSetVcpu = qemuDomainSetVcpu, /* 3.1.0 */ .domainSetBlockThreshold = qemuDomainSetBlockThreshold, /* 3.2.0 */ .domainSetLifecycleAction = qemuDomainSetLifecycleAction, /* 3.9.0 */ + .domainGetSevVmMeasurement = qemuDomainGetSevVmMeasurement, /* 4.2.0 */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 195248c88ae1..e3dd078e4e73 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4400,3 +4400,11 @@ qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, return qemuMonitorJSONSetWatchdogAction(mon, action); } + +char * +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon) +{ + QEMU_CHECK_MONITOR_NULL(mon); + + return qemuMonitorJSONGetSevMeasurement(mon); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 1b2513650c58..dd0821178c47 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1176,4 +1176,7 @@ virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon); int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon, const char *action); +char * +qemuMonitorGetSevMeasurement(qemuMonitorPtr mon); + #endif /* QEMU_MONITOR_H */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4424abfa7148..1d7f0e7c168e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7974,3 +7974,36 @@ qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; } + +char * +qemuMonitorJSONGetSevMeasurement(qemuMonitorPtr mon) +{ + const char *tmp; + char *measurement = NULL; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-sev-launch-measure", NULL))) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + data = virJSONValueObjectGetObject(reply, "return"); + + if (!(tmp = virJSONValueObjectGetString(data, "data"))) + goto cleanup; + + if (VIR_STRDUP(measurement, tmp) < 0){ + goto cleanup; + } + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return measurement; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 305f789902e9..b03b35ae0e8b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -342,6 +342,8 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, int qemuMonitorJSONSystemWakeup(qemuMonitorPtr mon); +char * qemuMonitorJSONGetSevMeasurement(qemuMonitorPtr mon); + int qemuMonitorJSONGetVersion(qemuMonitorPtr mon, int *major, int *minor, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9ea726dc45c0..080d244db156 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8497,7 +8497,8 @@ static virHypervisorDriver hypervisor_driver = { .domainSetGuestVcpus = remoteDomainSetGuestVcpus, /* 2.0.0 */ .domainSetVcpu = remoteDomainSetVcpu, /* 3.1.0 */ .domainSetBlockThreshold = remoteDomainSetBlockThreshold, /* 3.2.0 */ - .domainSetLifecycleAction = remoteDomainSetLifecycleAction /* 3.9.0 */ + .domainSetLifecycleAction = remoteDomainSetLifecycleAction, /* 3.9.0 */ + .domainGetSevVmMeasurement = remoteDomainGetSevVmMeasurement /* 4.2.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9dbd497b2fff..227ee8345683 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3448,6 +3448,15 @@ struct remote_domain_set_lifecycle_action_args { unsigned int flags; }; +struct remote_domain_get_sev_vm_measurement_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_get_sev_vm_measurement_ret { + remote_nonnull_string sev_measurement; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6135,5 +6144,11 @@ enum remote_procedure { * @priority: high * @acl: storage_pool:getattr */ - REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391 + REMOTE_PROC_STORAGE_POOL_LOOKUP_BY_TARGET_PATH = 391, + + /** + * @generate: both + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_SEV_VM_MEASUREMENT = 392 }; -- 2.14.3

On 02/27/2018 05:07 AM, Daniel P. Berrangé wrote: Sure, I am all for making it generic. Will take your suggestions. thanks Understood Damn... will fix in next rev.