[libvirt] [PATCH 3/4] conf: introduce sev element in domain
Brijesh Singh
brijesh.singh at amd.com
Tue Feb 27 17:01:02 UTC 2018
On 02/27/2018 02:34 AM, Peter Krempa wrote:
> On Mon, Feb 26, 2018 at 11:53:35 -0600, Brijesh Singh wrote:
>> 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 \
>> ...
>
> A general question. How does this work with migration? Since you state
> that only the guest has access to unencrypted state I presume that the
> qemu process sees it only encrypted data. Thus the encryption keys need
> to be transferred to the other host machine.
>
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.
> Also this means that we need to make sure that the migration will not
> work if the destination host of the migration does not support this
> (you'll need to add a migration cookie flag for this). Or if migration
> is not supported at all we'll need to reject it right away.
>
Do we need to do anything special in libvirt, or having migration
blocker in QEMU is good enough ?
> This means that the virDomainMemoryPeek will probably need to be fixed
> to return error too.
>
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.
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> 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(+)
>
> It would be preferable if you'd split the qemu bits from the config
> part.
>
Sure, I will do that.
>>
>> 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.
>
> As said in the previous review I'd prefer if we'd agree on a less-vendor
> specific XML for this.
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>
>
>> + </p>
>> + <pre>
>> +<domain>
>> + ...
>> + <sev>
>> + <policy> 1 </policy>
>> + <cbitpos> 47 </cbitpos>
>> + <reduced-phys-bits> 5 </reduced-phys-bits>
>> + <session> ... </session>
>> + <dh-cert> ... </dh>
>> + </sev>
>> + ...
>> +</domain>
>> +</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.
>
> It sounds a bit more platform dependant. Is there any way where this
> could be different from the data present in the capabilities? If not it
> does not make much sense to allow configuring it.
>
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.
> Also does it change the machine ABI? I'm asking whether it's necessary
> to expose this at all. (e.g. it's necessary to keep the value the same
> accross migrations).
See my comments, the value is host family dependent and if we are
migrating across different AMD family then its value will change.
>
>
>> + </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.
>
> Same applies for this.
>
>> + </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.
>
> So this is a hex number. Any documentation on the possible values?
I can provide the link of the complete documentation and if needed then
I can document here.
>
>> + </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.
>
> This should also be more documented.
>
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.
>> + </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>
>
> At least by providing the specification document. Preferably commiting
> it to the libvirt repository so that it does not change URIs in the
> future.
>
Will do, so far the link has been static and I am okay with putting it
either in here or commit description or both.
>> +
>> <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;
>
> virXPathNode since you are not interrested in more than one element.
>
Got it.
>> +
>> + 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);
>
> I think we should put these in the VM file directory rather than into
> the user config directory.
>
Sure.
>> +
>> + 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;
>
> These files should not be created in the command line generator but
> rather in 'qemuProcessPrepareHost'. Otherwise the test suite will leak
> these.
>
Okay will do so.
>> +
>> + virCommandAddArg(cmd, "-machine");
>> + virBufferAddLit(&buf, "memory-encryption=sev0");
>> + virCommandAddArgBuffer(cmd, &buf);
>> +
>> + virCommandAddArg(cmd, "-object");
>> + virBufferAddLit(&obj, "sev-guest,id=sev0");
>
> I don't see any capability check that qemu actually supports this
> object. You'll need to add them ...
Will do that.
>
>> + 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);
>
> This should be right at the beginning of the func.
>
Will do.
>> + 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);
>
> Also this is lacking test cases for the qemuxml2argvtest,
> qemuxml2xmltest/genericxml2xmltest.
>
Will look at those tests.
-Brijesh
More information about the libvir-list
mailing list