2:34 a.m.
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.
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.
This means that the virDomainMemoryPeek will probably need to be fixed
to return error too.
Signed-off-by: Brijesh Singh <brijesh.singh(a)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.
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.
+ </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.
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).
+ </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?
+ </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.
+ </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.
+
<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.
+
+ 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.
+
+ 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.
+
+ 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 ...
+ 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.
+ 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.