On 05/28/2018 02:25 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:26PM -0500, Brijesh Singh wrote:

...

QEMU version >= 2.12 provides support for launching an encrypted VMs on AMD x86 platform using Secure Encrypted Virtualization (SEV) feature. This patch adds support to query the SEV capability from the qemu.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- src/conf/domain_capabilities.h | 13 ++++ src/qemu/qemu_capabilities.c | 47 ++++++++++++++ src/qemu/qemu_capabilities.h | 4 ++ src/qemu/qemu_capspriv.h | 4 ++ src/qemu/qemu_monitor.c | 9 +++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 74 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 3 + .../caps_2.12.0.x86_64.replies | 10 +++ tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml | 3 +- 10 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 9b852e8649bf..c1093234ceb8 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -137,6 +137,19 @@ struct _virDomainCapsCPU { virDomainCapsCPUModelsPtr custom; };

+/* + * SEV capabilities + */ +typedef struct _virSEVCapability virSEVCapability; +typedef virSEVCapability *virSEVCapabilityPtr; +struct _virSEVCapability { + char *pdh; + char *cert_chain; + unsigned int cbitpos; + unsigned int reduced_phys_bits; +}; + + struct _virDomainCaps { virObjectLockable parent;

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8a63db5f4f33..49b74f7e12c1 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "screendump_device", "hda-output", "blockdev-del", + "sev-guest", );

@@ -555,6 +556,8 @@ struct _virQEMUCaps { size_t ngicCapabilities; virGICCapability *gicCapabilities;

+ virSEVCapability *sevCapabilities; + virQEMUCapsHostCPUData kvmCPU; virQEMUCapsHostCPUData tcgCPU; }; @@ -1121,6 +1124,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtual-css-bridge", QEMU_CAPS_CCW }, { "vfio-ccw", QEMU_CAPS_DEVICE_VFIO_CCW }, { "hda-output", QEMU_CAPS_HDA_OUTPUT }, + { "sev-guest", QEMU_CAPS_SEV_GUEST }, };

static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { @@ -2050,6 +2054,28 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, }

+void +virQEMUSevCapabilitiesFree(virSEVCapability *cap)

Since virSEVCapability will be added to virDomainCaps too, you need to move ^this into domain_capabilities.c so it will become virSEVCapabilityFree, I've got a further comment regarding this in patch 2 as well.

NOTE: notice the SEV in the function name, we should stay consistent in naming and since SEV is the name of the feature...

Noted, I will make these changes in next rev.

...

+{ + if (!cap) + return; + + VIR_FREE(cap->pdh); + VIR_FREE(cap->cert_chain); + VIR_FREE(cap); +} + + +void +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, + virSEVCapability *capabilities) +{ + virQEMUSevCapabilitiesFree(qemuCaps->sevCapabilities);

virSEVCapabilityFree(qemuCaps->sevCapabilities)

...

+ + qemuCaps->sevCapabilities = capabilities; +} + + static int virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) @@ -2580,6 +2606,21 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, }

+static int +virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + virSEVCapability *caps = NULL; + + if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0) + return -1; + + virQEMUCapsSetSEVCapabilities(qemuCaps, caps); + + return 0; +} + + bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque) @@ -3965,6 +4006,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW); }

+ /* Probe for SEV capabilities */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) { + if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0) + virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST); + } + ret = 0; cleanup: return ret; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3e120e64c0b4..8b7eef4359b7 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -473,6 +473,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_SCREENDUMP_DEVICE, /* screendump command accepts device & head */ QEMU_CAPS_HDA_OUTPUT, /* -device hda-output */ QEMU_CAPS_BLOCKDEV_DEL, /* blockdev-del is supported */ + QEMU_CAPS_SEV_GUEST, /* -object sev-guest,... */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; @@ -599,4 +600,7 @@ bool virQEMUCapsGuestIsNative(virArch host, bool virQEMUCapsCPUFilterFeatures(const char *name, void *opaque);

+void +virQEMUSevCapabilitiesFree(virSEVCapability *capabilities); + #endif /* __QEMU_CAPABILITIES_H__*/ diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 0199501c931b..20b03876d470 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -85,6 +85,10 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps, virGICCapability *capabilities, size_t ncapabilities);

+void +virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps, + virSEVCapability *capabilities); + int virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 43f1d2f81671..3b034930408c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3778,6 +3778,15 @@ qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, return qemuMonitorJSONGetGICCapabilities(mon, capabilities); }

1 more blank line here...

...

+int +qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetSEVCapabilities(mon, capabilities); +} +

int qemuMonitorNBDServerStart(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c28db1a52b8b..b1b7ef09c929 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -710,6 +710,9 @@ int qemuMonitorSetMigrationCapabilities(qemuMonitorPtr mon, int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities);

+int qemuMonitorGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities); + typedef enum { QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0, QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with non-shared storage with full disk copy */ diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9f5c35879587..24d3a2ff412f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6436,6 +6436,80 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, return ret; }

Need 1 more blank line here...

...

+int +qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr caps; + virSEVCapability *capability = NULL; + const char *pdh = NULL, *cert_chain = NULL; + int cbitpos, reduced_phys_bits;

We declared ^these as uint in virSEVCapability, so that fact should be reflected here too...

Noted.

...

+ + *capabilities = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-sev-capabilities", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + caps = virJSONValueObjectGetObject(reply, "return"); + + if (virJSONValueObjectGetNumberInt(caps, "cbitpos", &cbitpos) < 0) {

GetNumberUInt()...

Noted.

...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'cbitpos' field is missing")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberInt(caps, "reduced-phys-bits", + &reduced_phys_bits) < 0) {

GetNumberUInt()...

Noted.

...

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'reduced-phys-bits' field is missing"));

we should mention the query command that failed, i.e. "query-sev-capabilities reply was missing 'xyz' field"

Noted.

...

+ goto cleanup; + } + + if (!(pdh = virJSONValueObjectGetString(caps, "pdh"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'pdh' field is missing"));

same here

...

+ goto cleanup; + } + + if (!(cert_chain = virJSONValueObjectGetString(caps, "cert-chain"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

same here...

...

+ _("'cert-chain' field is missing")); + goto cleanup; + } + + if (VIR_ALLOC(capability) < 0) + goto cleanup; + + if (VIR_STRDUP(capability->pdh, pdh) < 0) + goto cleanup; + + if (VIR_STRDUP(capability->cert_chain, cert_chain) < 0) + goto cleanup; + + capability->cbitpos = cbitpos; + capability->reduced_phys_bits = reduced_phys_bits; + VIR_STEAL_PTR(*capabilities, capability); + ret = 0; + + cleanup: + virQEMUSevCapabilitiesFree(capability); + virJSONValueFree(cmd); + virJSONValueFree(reply); + + return ret; +} + static virJSONValuePtr qemuMonitorJSONBuildInetSocketAddress(const char *host, const char *port) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f4ac8319ac8a..129aab22bf98 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -152,6 +152,9 @@ int qemuMonitorJSONSetMigrationCapabilities(qemuMonitorPtr mon, int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, virGICCapability **capabilities);

+int qemuMonitorJSONGetSEVCapabilities(qemuMonitorPtr mon, + virSEVCapability **capabilities); + int qemuMonitorJSONMigrate(qemuMonitorPtr mon, unsigned int flags, const char *uri); diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies index c40046beef6b..ace35374ef96 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.replies @@ -18995,6 +18995,16 @@ "id": "libvirt-51" }

+{ + "return" : { + "reduced-phys-bits": 1, + "cbitpos": 47, + "cert-chain": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA", + "pdh": "AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA" + }, + "id": "libvirt-52" +} + { "return": { }, diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml index 3c7dadffcd8a..58a1bf835a73 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml @@ -204,9 +204,10 @@ <flag name='screendump_device'/> <flag name='hda-output'/> <flag name='blockdev-del'/> + <flag name='sev-guest'/> <version>2011090</version> <kvmVersion>0</kvmVersion> - <microcodeVersion>390813</microcodeVersion> + <microcodeVersion>391059</microcodeVersion> <package>v2.12.0-rc0</package> <arch>x86_64</arch> <hostCPU type='kvm' model='base' migratability='yes'>

Erik

On 05/28/2018 05:28 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:27PM -0500, Brijesh Singh wrote:

...

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 (PDH) 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@amd.com> ---

...and this one should be IMHO named conf: Expose SEV in domain capabilities

...

docs/formatdomaincaps.html.in | 40 ++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincaps.rng | 20 ++++++++++++++++++++ src/conf/domain_capabilities.c | 20 ++++++++++++++++++++ src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ 5 files changed, 83 insertions(+)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index b68ae4b4f1f3..f37b059ba6b1 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -434,6 +434,12 @@ </enum> </gic> <vmcoreinfo supported='yes'/> + <sev> + <pdh>UWxKSlNrVlRTRk5KVGtkSVFVMUU=</pdh> + <cert-chain>VVd4S1NsTnJWbFJUUms1S1ZHdGtTVkZWTVVVPQ==</cert-chain> + <cbitpos>47</cbitpos> + <reduced-phys-bits>1</reduced-phys-bits> + </sev> </features> </domainCapabilities> </pre> @@ -462,5 +468,39 @@

<p>Reports whether the vmcoreinfo feature can be enabled</p>

+ <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> + + <p> + For more details on SEV feature see: + <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf"> + SEV API spec</a> and <a href="http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf"> + SEV White Paper</a> + </p> + + <dl> + <dt><code>pdh</code></dt> + <dd>A base64 encoded platform Diffie-Hellman public key which can be + exported to remote entities that desire to establish a secure transport + context with the SEV platform in order to transmit data securely.</dd> + <dt><code>cert-chain</code></dt> + <dd> A base64 encoded platform certificate chain that includes the platform + endorsement key (PEK), owners certificate authority (OCD), and chip + endorsement key (CEK).</dd> + <dt><code>cbitpos</code></dt> + <dd>When memory encryption is enabled, one of the physical address bits + (aka the C-bit) is utilized to mark if a memory page is protected. The + C-bit position is Hypervisor dependent.</dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>When memory encryption is enabled, we lose certain bits in physical + address space. The number of bits we lose is hypervisor dependent.</dd> + </dl> + </body> </html> diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 5913d711a3fe..26265645b82a 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -184,6 +184,9 @@ <interleave> <ref name='gic'/> <ref name='vmcoreinfo'/> + <optional> + <ref name='sev'/>

needs 1 more level of indent...

Noted.

...

+ </optional> </interleave> </element> </define> @@ -201,6 +204,23 @@ </element> </define>

+ <define name='sev'> + <element name='sev'> + <element name='pdh'> + <data type='string'/> + </element> + <element name='cert-chain'> + <data type='string'/> + </element> + <element name='cbitpos'> + <data type='unsignedInt'/> + </element> + <element name='reduced-phys-bits'> + <data type='unsignedInt'/> + </element> + </element> + </define> + <define name='value'> <zeroOrMore> <element name='value'> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 6e2ab0a28796..3b767c45cbb3 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -542,6 +542,25 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf, FORMAT_EPILOGUE(gic); }

+static void +virDomainCapsFeatureSEVFormat(virBufferPtr buf, + virSEVCapabilityPtr const sev) +{ + if (!sev) + return; + + virBufferAddLit(buf, "<sev supported='yes'>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<cbitpos>%d</cbitpos>\n", sev->cbitpos); + virBufferAsprintf(buf, "<reduced-phys-bits>%d</reduced-phys-bits>\n", + sev->reduced_phys_bits); + virBufferEscapeString(buf, "<pdh>%s</pdh>\n", sev->pdh); + virBufferEscapeString(buf, "<cert-chain>%s</cert-chain>\n", + sev->cert_chain);

As I said, I have to agree with Dan here that reporting the 8k string in capabilities is a good idea, so we can store it, we just wouldn't format it ^here...

Sure, if decide to go with new APIs then this code need to be reworked.

...

+ virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</sev>\n"); +} +

char * virDomainCapsFormat(virDomainCapsPtr const caps) @@ -585,6 +604,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps) virDomainCapsFeatureGICFormat(&buf, &caps->gic); virBufferAsprintf(&buf, "<vmcoreinfo supported='%s'/>\n", caps->vmcoreinfo ? "yes" : "no"); + 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 c1093234ceb8..e33bef525ef4 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -172,6 +172,7 @@ struct _virDomainCaps {

virDomainCapsFeatureGIC gic; bool vmcoreinfo; + virSEVCapabilityPtr sev; /* add new domain features here */ };

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 49b74f7e12c1..3345b09fa384 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4998,6 +4998,8 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps, virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) return -1; + + domCaps->sev = qemuCaps->sevCapabilities;

domCaps is a one-time-use object needed when you format the qemuCaps into domCaps which is returned to the user and is therefore cleaned up on the API exit, so ^this shouldn't be considered safe and you should create virQEMUCapsFillDomainFeatureSEVCaps where you allocate a fresh domCaps->sev structure and then you'll need to put virSEVCapabilityFree I mentioned in patch 1 to virDomainCapsDispose so as not to leak any memory related to copying the SEV data.

I had something very similar in v2 or v3 but was asked in one of review feedback to drop the function and simply save the pointers. I will revisit and re-introduce the function. -Brijesh

On Wed, May 23, 2018 at 04:18:28PM -0500, Brijesh Singh wrote:

The launch-security element can be used to define the security model to use when launching a domain. Currently we support 'sev'.

When 'sev' is used, the VM will be launched with AMD SEV feature enabled. 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.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- docs/formatdomain.html.in | 115 ++++++++++++++++++ docs/schemas/domaincommon.rng | 39 ++++++ src/conf/domain_conf.c | 133 +++++++++++++++++++++ src/conf/domain_conf.h | 27 +++++ tests/genericxml2xmlindata/launch-security-sev.xml | 24 ++++ tests/genericxml2xmltest.c | 2 + 6 files changed, 340 insertions(+) create mode 100644 tests/genericxml2xmlindata/launch-security-sev.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 665d0f25293e..cab08ea52003 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -8310,6 +8310,121 @@ 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><launch-security type='sev'></code> element + is used to provide the guest owners input used for creating an encrypted + VM using the AMD 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. + + For more information see various input parameters and its format see the SEV API spec + <a href="https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf"> https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf </a> + <span class="since">Since 4.4.0</span> + </p> + <pre> +<domain> + ... + <launch-security type='sev'> + <policy> 0x0001 </policy> + <cbitpos> 47 </cbitpos> + <reduced-phys-bits> 1 </reduced-phys-bits> + <session> AAACCCDD=FFFCCCDSDS </session> + <dh-cert> RBBBSDDD=FDDCCCDDDG </dh> + </sev> + ... +</domain> +</pre> + + <dl> + <dt><code>cbitpos</code></dt> + <dd>The required <code>cbitpos</code> element 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 the domain capabilities.

From the cover letter it seemed like this is always the same as the value reported in the domain capabilities and therefore I wrote what I wrote, but is it always the same value as the one reported in capabilities as it's hypervisor-dependent or can it in fact be configurable? Because if it can change, then of course it makes sense to let user config this for a guest and this is okay.

+ </dd> + <dt><code>reduced-phys-bits</code></dt> + <dd>The required <code>reduced-phys-bits</code> element 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 the domain capabilities. + </dd>

Same here...

+ <tr> + <td> 15:6 </td>

Just wondering, you write 16:32, shouldn't ^this be 6:15 then instead of 15:6?

+ <td> reserved </td> + </tr> + <tr> + <td> 16:32 </td> + <td> The guest must not be transmitted to another platform with a + lower firmware version. </td> + </tr> + </table> +

...

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f16e157397d4..69b6c84b9540 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -77,6 +77,9 @@ <optional> <ref name='keywrap'/> </optional> + <optional> + <ref name='launch-security'/> + </optional> </interleave> </element> </define> @@ -436,6 +439,42 @@ </element> </define>

+ <define name="launch-security"> + <element name="launch-security"> + <attribute name="type"> + <value>sev</value> + </attribute> + <interleave> + <element name="cbitpos"> + <data type='unsignedInt'/> + </element> + <element name="reduced-phys-bits"> + <data type='unsignedInt'/> + </element> + <optional> + <element name="policy"> + <ref name='hexuint'/> + </element> + </optional>

You made 'policy' required per v5 comments, so the RNG schema needs to be updated as well.

+ <optional> + <element name="handle"> + <ref name='unsignedInt'/> + </element> + </optional> + <optional> + <element name="dh-cert"> + <data type="string"/> + </element> + </optional> + <optional> + <element name="session"> + <data type="string"/> + </element> + </optional> + </interleave> + </element> + </define> + <!-- Enable or disable perf events for the domain. For each of the events the following rules apply: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 225309809029..8fde7f8d0359 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -933,6 +933,10 @@ VIR_ENUM_IMPL(virDomainShmemModel, VIR_DOMAIN_SHMEM_MODEL_LAST, "ivshmem-plain", "ivshmem-doorbell")

+VIR_ENUM_IMPL(virDomainLaunchSecurity, VIR_DOMAIN_LAUNCH_SECURITY_LAST, + "", + "sev") + static virClassPtr virDomainObjClass; static virClassPtr virDomainXMLOptionClass; static void virDomainObjDispose(void *obj); @@ -2904,6 +2908,19 @@ virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune) }

+static void +virDomainSevDefFree(virDomainSevDefPtr def)

The naming nit pick again...I'd prefer SEV instead of Sev

+{ + if (!def) + return; + + VIR_FREE(def->dh_cert); + VIR_FREE(def->session); + + VIR_FREE(def); +} + + void virDomainDefFree(virDomainDefPtr def) { size_t i; @@ -3085,6 +3102,8 @@ void virDomainDefFree(virDomainDefPtr def) if (def->namespaceData && def->ns.free) (def->ns.free)(def->namespaceData);

+ virDomainSevDefFree(def->sev); + xmlFreeNode(def->metadata);

VIR_FREE(def); @@ -15688,6 +15707,85 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, }

+static virDomainSevDefPtr +virDomainSevDefParseXML(xmlNodePtr sevNode, + xmlXPathContextPtr ctxt) +{ + char *tmp = NULL; + char *type = NULL; + xmlNodePtr save = ctxt->node; + virDomainSevDefPtr def; + unsigned long policy; + + ctxt->node = sevNode; + + if (VIR_ALLOC(def) < 0) + return NULL; + + if (!(type = virXMLPropString(sevNode, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing launch-security type")); + goto error; + } + + def->sectype = virDomainLaunchSecurityTypeFromString(type); + switch ((virDomainLaunchSecurity) def->sectype) { + case VIR_DOMAIN_LAUNCH_SECURITY_SEV: + break; + case VIR_DOMAIN_LAUNCH_SECURITY_NONE: + case VIR_DOMAIN_LAUNCH_SECURITY_LAST: + default: + virReportError(VIR_ERR_XML_ERROR, + _("unsupported launch-security type '%s'"), + type); + goto error; + } + + if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("failed to get launch-security cbitpos")); + goto error; + }

If in fact cbitpos and reduced-phys-bits can be configurable, then again, this is fine, if not, we can feed that from capabilities in qemu driver's post parse callback and this function can thus be simplified. Actually, even if wasn't configurable at the moment but it can change in the future, then we could settle on a middle ground here and leave the cbitpos and reduced-phys-bits from the XML for now, we'll fill those in internally according to the qemu caps and if we have a need to expose these fields through the XML later, we can add it then, e.g. if this is needed for migration for some reason.

+ + if (virXPathUInt("string(./reduced-phys-bits)", ctxt, + &def->reduced_phys_bits) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("failed to get launch-security reduced-phys-bits")); + goto error; + } + + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("failed to get launch-security policy")); + goto error; + } + + def->policy = policy; + + 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); + } + + ctxt->node = save; + return def; + + error: + VIR_FREE(tmp); + virDomainSevDefFree(def); + ctxt->node = save; + return NULL; +}

...

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 37356df42d71..d1c101cc4673 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;

_virDomainSEVDef

+typedef virDomainSevDef *virDomainSevDefPtr; ...

+ +struct _virDomainSevDef { + int sectype; /* enum virDomainLaunchSecurity */ + char *dh_cert; + char *session; + unsigned int policy; + unsigned int cbitpos; + unsigned int reduced_phys_bits;

Waiting on your notes, but ^these last two could be dropped if those can't be configurable as the user pleases. Erik

On Fri, Jun 01, 2018 at 11:34:19AM -0500, Brijesh Singh wrote:

On 05/28/2018 09:36 AM, Erik Skultety wrote:

...

On Wed, May 23, 2018 at 04:18:31PM -0500, Brijesh Singh wrote:

...

The API can be used outside the libvirt to get the launch security information. When SEV is enabled, the API can be used to get the measurement of the launch process.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- include/libvirt/libvirt-domain.h | 17 ++++++++++++++ src/driver-hypervisor.h | 7 ++++++ src/libvirt-domain.c | 48 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++++ 4 files changed, 77 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index d7cbd187969d..f252d18da72f 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4764,4 +4764,21 @@ int virDomainSetLifecycleAction(virDomainPtr domain, unsigned int action, unsigned int flags);

+/** + * Launch Security API + */ + +/** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT: + * + * Macro represents the launch measurement of the SEV guest, + * as VIR_TYPED_PARAM_STRING. + */ +#define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"

fails make syntax-check because of indentation, should be "# define ..."

...

...

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 95df3a0dbc7b..5ccae5da8883 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 { virStoragePoolLookupByTargetPath; } LIBVIRT_3.9.0;

+LIBVIRT_4.4.0 { + global: + virDomainGetLaunchSecurityInfo; +} LIBVIRT_4.1.0;

Will most probably become 4.5.0 :(

Technically, I don't have any notes related to the functional changes, therefore I'd give you my RB, however, I still find the naming confusing and I can't think of something better. What if one day we'll actually be able to/need to modify the configuration for some reason, we should reserve a name like this for future modifications of launch-security data of the guest. Next, you're preparing for adding support for some kind of setter in the virsh command, any idea of what the setter data might be? Because I can imagine that you'd still want to perform a measurement, but want to send additional arguments, to the remote side's firmware to change the behaviour of the measurement and you can't do this with a simple flag, you also need typed params for that which means wou'd end up with something like:

int virDomainLaunchSecurityMeasure(virDomainPtr domain, virTypedParamsPtr send_params, unsigned int nsend_params, virTypedParamsPtr *recv_params, unsigned int nrecv_params);

Actually we had similar API in previous patches but I followed the review feedback from earlier versions where it was hinted to use launch-security so that in future if any other architecture or platform provides the similar feature but with different naming then we don't have to introduce yet another APIs.

Yes I saw that one, but that one was IMHO more about not having 'SEV' in the name, so it wouldn't be platform specific. Even if some other vendor comes up with a different technique, they either will do some kind of validation before starting in which case 'Measure' could seems like an appropriate name or they won't need it at all because they're going to use a different approach. On the other hand, we're going into v7 and nobody has spoken up since the virDomainGetLaunchSecurityInfo suggestion, so I'll stop the bikeshed I started and let's go with what you've got here now.

After guest owner validates the measurement, it will provide some additional information to VM. Typically this may include the some type of secret information and we may need to pass the following inputs via the API.

- secret blob - length of the blob - the memory address where the blob need to be copied

Okay, so this is after the measurement, thanks. There was actually another question hidden in my thoughts in the previous reply where I described a potential need for send arguments to somehow augment the process of measuring, e.g. passing some kind of nonce or a seed to the FW for some reason when you're invoking the API above to get the measurement, because you can't do that with a simple flag. But that should be a separate setter so as not to overcomplicate things, this could potentially be achieved by the setter API you're going to introduce at some point and simply adding typed parameters for that. Thanks, Erik

On Wed, May 23, 2018 at 04:18:33PM -0500, Brijesh Singh wrote:

This patch implements the internal driver API for launch event into qemu driver. When SEV is enabled, execute 'query-sev-launch-measurement' to get the measurement of memory encrypted through launch sequence.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> ---

Apart from what I'm writing below, naming comments from patch 6 apply here too...

src/qemu/qemu_driver.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 8 ++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 42 +++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 ++ 5 files changed, 123 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3a328e5d4679..6569dea32fce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21194,6 +21194,73 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, }

+static int +qemuDomainGetSevMeasurement(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + int ret = -1; + char *tmp; + int maxpar = 0; + + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + return -1; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + tmp = qemuMonitorGetSevMeasurement(QEMU_DOMAIN_PRIVATE(vm)->mon); + if (tmp == NULL) + goto endjob; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + if (virTypedParamsAddString(params, nparams, &maxpar, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT, + tmp) < 0) + goto endjob; + + ret = 0; + + endjob:

You're going to leak @tmp here, since virTypedParamsAddString makes its own copy of tmp.

+ qemuDomainObjEndJob(driver, vm); + return ret; +} + + +static int +qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = domain->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + + if (!(vm = qemuDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainGetLaunchSecurityInfoEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; + + if (vm->def->sev) { + if (qemuDomainGetSevMeasurement(driver, vm, params, nparams, flags) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -21414,6 +21481,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainSetVcpu = qemuDomainSetVcpu, /* 3.1.0 */ .domainSetBlockThreshold = qemuDomainSetBlockThreshold, /* 3.2.0 */ .domainSetLifecycleAction = qemuDomainSetLifecycleAction, /* 3.9.0 */ + .domainGetLaunchSecurityInfo = qemuDomainGetLaunchSecurityInfo, /* 4.2.0 */ };

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3b034930408c..977cbe5a41f8 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4226,3 +4226,11 @@ qemuMonitorBlockdevDel(qemuMonitorPtr mon,

return qemuMonitorJSONBlockdevDel(mon, nodename); } + +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 b1b7ef09c929..8a64ae5f3d96 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1137,4 +1137,7 @@ int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, int qemuMonitorBlockdevDel(qemuMonitorPtr mon, const char *nodename);

+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 24d3a2ff412f..041f595ca1e4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8024,3 +8024,45 @@ qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; } + +/** + * The function is used to retrieve the measurement of SEV guest.

s/of SEV/of a SEV

+ * The measurement is signature of the memory contents that was encrypted + * through the SEV launch flow. + * + * A example jason output:

s/jason/JSON

+ * + * { "execute" : "query-sev-launch-measure" } + * { "return" : { "data" : "4l8LXeNlSPUDlXPJG5966/8%YZ" } } + */ +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)

We should call qemuMonitorJSONCheckReply instead, not only does it call JSONCheckError, but checks the return type as well which is suited for callers who actually care about the data returned, not just whether the query passed or failed.

+ 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;

Erik

On Wed, May 23, 2018 at 04:18:34PM -0500, Brijesh Singh wrote:

Add new 'launch-security' command, the command can be used to get or set the launch security information when booting encrypted VMs.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- tools/virsh-domain.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 2 files changed, 86 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cfbbf5a7bc39..27bb702c8bb7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13870,6 +13870,81 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd) return ret >= 0; }

+/* + * "launch-security" command

Sigh... feature/command/API naming is soo tricky in libvirt, even though we always try to be careful, we'll often hit the issues with commands being named wrongly or just confusing, this is one of those cases, because the XML element is called launch-security this really does make the impression the command makes changes to the launch-security configuration for the guest, which is not the case here. I think that if we ever find ourselves in that position that we need to change the configuration of launch-security for a guest, then we need to reserve this name for that, but now, launch-security-measure sounds more descriptive of what it does right now to me, see my question about the setter below, so that we can find a more suitable name for it. This of course means the underlying APIs would need to change too, see my comments to patches 6-8. Any ideas for the naming guys?

+ */ +static const vshCmdInfo info_launch_security[] = { + {.name = "help", + .data = N_("Get or set launch-security information")

So since we don't have the setter yet, it should only report as a getter, because we're not introducing the setter switch as of now, once we do, we can change the commentary as well...

+ }, + {.name = "desc", + .data = N_("Get or set the current launch-security information for " + "a guest domain.\n"

Here too...

+ " To get the launch-security information use following" + " command: \n\n" + " virsh # launch-security <domain>")

^these 3 lines are unnecessary because that's contained in the SYNOPSIS already.

+ }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_launch_security[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; + +static void +virshPrintLaunchSecurityInfo(vshControl *ctl, virTypedParameterPtr params, + int nparams) +{ + size_t i; + + for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + vshPrintExtra(ctl, "%-15s: %s\n", params[i].field, params[i].value.s); + } +}

No need for ^this Print helper really...

+ +static bool +cmdLaunchSecurity(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int nparams = 0; + virTypedParameterPtr params = NULL; + bool ret = false; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; +

So what should the setter then do, conceptually is enough for me as an answer? I'm asking because I don't see a point in using VIR_DOMAIN_AFFECT_X at all because those are for commands that do perform changes to the VM configuration, either live or offline, getting a measurement doesn't touch the configuration at all. Erik

+ if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (virDomainGetLaunchSecurityInfo(dom, ¶ms, &nparams, flags) != 0) { + vshError(ctl, "%s", _("Unable to get launch security info")); + goto cleanup; + } + + virshPrintLaunchSecurityInfo(ctl, params, nparams); + + ret = true; + cleanup: + virTypedParamsFree(params, nparams); + virshDomainFree(dom); + return ret; +} + + const vshCmdDef domManagementCmds[] = { {.name = "attach-device", .handler = cmdAttachDevice, @@ -14485,5 +14560,11 @@ const vshCmdDef domManagementCmds[] = { .info = info_domblkthreshold, .flags = 0 }, + {.name = "launch-security-info", + .handler = cmdLaunchSecurity, + .opts = opts_launch_security, + .info = info_launch_security, + .flags = 0 + }, {.name = NULL} }; diff --git a/tools/virsh.pod b/tools/virsh.pod index 929958a9533c..31bb26bda2ac 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2899,6 +2899,11 @@ See B<vcpupin> for information on I<cpulist>. Output the IP address and port number for the VNC display. If the information is not available the processes will provide an exit code of 1.

+=item B<launch-security-info> I<domain> + +Get the measurement of the memory contents encrypted through the launch +sequence when I<launch-security> is provided. + =back

=head1 DEVICE COMMANDS -- 2.14.3