[libvirt] [PATCH v5 03/10] conf: introduce launch-security element in domain
John Ferlan
jferlan at redhat.com
Mon Apr 2 18:26:36 UTC 2018
On 04/02/2018 10:18 AM, 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.
>
> Reviewed-by: "Daniel P. Berrangé" <berrange at redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> ---
> docs/formatdomain.html.in | 120 ++++++++++++++++++++++++++++++++++++++++++
> docs/schemas/domaincommon.rng | 39 ++++++++++++++
> src/conf/domain_conf.c | 110 ++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 26 +++++++++
> 4 files changed, 295 insertions(+)
>
Again - I see the R-by from Daniel, but I have more comments for the
patches...
Also since this patch is where we introduce the XML, the xml2xml changes
that are in the last patch should move into here so that we keep
everything together.
We're probably getting beyond the point of me doing the movement, so a
v6 will be needed.
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 82e7d7c..2a6bed7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8200,6 +8200,126 @@ 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 SEV API spec
s/see/see the/
> + <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.2.0</span>
I'll be 4.3.0 now that 4.2.0 is released.
> + </p>
> + <pre>
> +<domain>
> + ...
> + <launch-security type='sev'>
> + <policy> 0 </policy>
> + <cbitpos> 47 </cbitpos>
> + <reduced-phys-bits> 5 </reduced-phys-bits>
> + <session> ... </session>
> + <dh-cert> ... </dh>
> + </sev>
> + ...
> +</domain>
> +</pre>
Your example should provide some more realistic values.
> +
> + <p>
> + A least <code>cbitpos</code> and <code>reduced-phys-bits</code> must be
> + nested within the <code>launch-security</code> element.
> + </p>
The above won't be necessary as long as you note required below, e.g.
> + <dl>
> + <dt><code>cbitpos</code></dt>
> + <dd>The <code>cbitpos</code> element provides the C-bit (aka encryption bit)
s/The/The required/
> + 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.
s/from domaincapabilities/from the domain capabilities/
> + </dd>
> + <dt><code>reduced-phys-bits</code></dt>
> + <dd>The <code>reduced-phys-bits</code> element provides the physical
s/The/The required/
> + address bit reducation. Similar to <code>cbitpos</code> the value of <code>
s/reducation/reduction/
> + reduced-phys-bit</code> is hypervisor dependent and can be obtained
> + through the <code>sev</code> element from domaincapabilities.
s/from domaincapabilities/from the domain capabilities/
> + </dd>
> + <dt><code>policy</code></dt>
> + <dd>The optional <code>policy</code> element 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.
> +
There's some long lines above and below - while not a policy - for those
using 80 character wide displays, we try to keep lines in the 70-75 char
range.
> + The guest policy is a 4-byte structure with the fields shown in Table:
How about 4 unsigned byte
> +
> + <table class="top_table">
> + <tr>
> + <th> Bit(s) </th>
> + <th> Description </th>
> + </tr>
> + <tr>
> + <td> 0 </td>
> + <td> Debugging of the guest is disallowed when set </td>
> + </tr>
> + <tr>
> + <td> 1 </td>
> + <td> Sharing keys with other guests is disallowed when set </td>
> + </tr>
> + <tr>
> + <td> 2 </td>
> + <td> SEV-ES is required when set</td>
> + </tr>
> + <tr>
> + <td> 3 </td>
> + <td> Sending the guest to another platform is disallowed when set</td>
> + </tr>
> + <tr>
> + <td> 4 </td>
> + <td> The guest must not be transmitted to another platform that is
> + not in the domain when set. </td>
> + </tr>
> + <tr>
> + <td> 5 </td>
> + <td> The guest must not be transmitted to another platform that is
> + not SEV capable when set. </td>
> + </tr>
> + <tr>
> + <td> 15:6 </td>
> + <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>
I don't see this in the qemu policies:
#define SEV_POLICY_NODBG 0x1
#define SEV_POLICY_NOKS 0x2
#define SEV_POLICY_ES 0x4
#define SEV_POLICY_NOSEND 0x8
#define SEV_POLICY_DOMAIN 0x10
#define SEV_POLICY_SEV 0x20
Sois there just one bit for the lower firmware thing or is this a word
field with some value expected?
> + </tr>
> + </table>
> + Default value is 0x1
So this is the QEMU default currently; however, that could change in the
future. I think we could just not say what the default is. It'll make
some changes a bit later on easier too.
> +
> + </dd>
> + <dt><code>dh-cert</code></dt>
> + <dd>The optional <code>dh-cert</code> element provides the guest owners public
s/owners/owners base64 encoded/
> + 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
s/SEV/the SEV/
> + owner. The value must be encoded in base64.
and the last sentence wouldn't be needed any more...
> + </dd>
> + <dt><code>session</code></dt>
> + <dd>The optional <code>session</code> element provides the guest owners
> + session blob defined in SEV API spec. The value must be encoded in base64.
s/session/base64 encoded session/
s/in/in the/
and the last sentence wouldn't be needed any more...
> +
> + See SEV spec LAUNCH_START section for session blob format.
s/for/for the/
> + </dd>
> + </dl>
> +
> <h2><a id="examples">Example configs</a></h2>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index a72c919..6a0e129 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>
Hopefully hexuint will suffice over time... On the other hand, this
patch uses virXPathULongHex in order to parse.
> + <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>
> +
Similar to patch 2 questions... Should this be
"launchSecurity"
"reducedPhysBits"
"DHCert"
Changing has fairly major implications, but I'd rather not have it baked
in and then have someone throw a complaint and have to redo things.
Since many people are off today, let's give them at least until tomorrow
to provide feedback before spinning a v6...
> <!--
> 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 ae7c0d9..4acf45c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -929,6 +929,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);
> @@ -2897,6 +2901,14 @@ virDomainCachetuneDefFree(virDomainCachetuneDefPtr cachetune)
> VIR_FREE(cachetune);
> }
>
again 2 blank lines between functions
> +static void
> +virDomainSevDefFree(virDomainSevDefPtr def)
> +{
Rather than making callers know...
if (!def)
return;
> + VIR_FREE(def->dh_cert);
> + VIR_FREE(def->session);
> +
> + VIR_FREE(def);
> +}
>
> void virDomainDefFree(virDomainDefPtr def)
> {
> @@ -3079,6 +3091,9 @@ void virDomainDefFree(virDomainDefPtr def)
> if (def->namespaceData && def->ns.free)
> (def->ns.free)(def->namespaceData);
>
> + if (def->sev)
Meaning this one won't be necessary
> + virDomainSevDefFree(def->sev);
> +
> xmlFreeNode(def->metadata);
>
> VIR_FREE(def);
> @@ -15537,6 +15552,70 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node,
> return ret;
> }
>
2 blank lines
> +static virDomainSevDefPtr
> +virDomainSevDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt)
Two lines:
virDomainSevDefParseXML(xmlNodePtr sevNode,
xmlXPathContextPtr ctxt)
> +{
> + char *tmp = NULL, *type = NULL;
Prefer 2 lines here too:
char *tmp = NULL;
char *type = NULL;
> + xmlNodePtr save = ctxt->node;
> + virDomainSevDefPtr def;
> + unsigned long policy;
probably should initialize = 0;
> +
> + 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;
> + }
> +
Assuming 'sev' for domain_conf isn't "normal" - that is it's not future
proof...
> + if (virDomainLaunchSecurityTypeFromString(type) !=
> + VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
This produces a libvirt failed for some reason type error message...
> + goto error;
> + }
So, as ugly as this will look, using switch/case logic:
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;
}
NB: I've *added* ->sectype to @def [see below]
> +
> + if (virXPathUInt("string(./cbitpos)", ctxt, &def->cbitpos) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get cbitpos"));
s/get/get launch-security/
> + goto error;
> + }
> +
> + if (virXPathUInt("string(./reduced-phys-bits)", ctxt,
> + &def->reduced_phys_bits) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("failed to get reduced-phys-bits"));
s/get/get launch-security/
> + goto error;
> + }
> +
> + if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0)> + policy = 0x1;
Hmmm... This one is optional which makes things a bit interesting. How
do you know it was provided? Today the default could be 0x1, but
sometime in the future if you decide upon a different default, then
things get really complicated. Or for some other chip and/or hypervisor
the default won't be 1.
Also virXPathULongHex returns -1 or -2 w/ different meanings - if it's
not there, You get a -1 when not provided and -2 when you have invalid
value in field, which should be an error.
Finally, ULongHex returns 64 bits and your field is 32 bits leading to
possible overflow
So, either you have to have a "policy_provided" boolean of some sort or
I think preferably leave it as 0 (zero) indicating not provided and then
when generating a command line check against 0 and provide the
hypervisor dependent value on the command line *OR* just don't provided
it an let the hypervisor choose it's default value because the value
wasn't provided (that's a later patch though)
Also see [1] below...
So my suggestion is:
if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 ||
policy > UINT_MAX) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("invalid launch-security policy value"));
goto error;
}
def->policy = policy;
If -1 is returned, the def->policy = 0; otherwise, it's set to something.
> +
> + 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;
> +}
2 blank lines
>
> static virDomainMemoryDefPtr
> virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
> @@ -20210,6 +20289,13 @@ virDomainDefParseXML(xmlDocPtr xml,
> ctxt->node = node;
> VIR_FREE(nodes);
>
> + /* Check for SEV feature */
> + if ((node = virXPathNode("./launch-security", ctxt)) != NULL) {
> + def->sev = virDomainSevDefParseXML(node, ctxt);
> + if (!def->sev)
> + goto error;
> + }
> +
> /* analysis of memory devices */
> if ((n = virXPathNodeSet("./devices/memory", ctxt, &nodes)) < 0)
> goto error;
> @@ -26087,6 +26173,27 @@ virDomainKeyWrapDefFormat(virBufferPtr buf, virDomainKeyWrapDefPtr keywrap)
> }
>
2 blank lines
> static void
> +virDomainSevDefFormat(virBufferPtr buf, virDomainSevDefPtr sev)
Two lines e.g.
virDomainSevDefFormat(virBufferPtr buf,
virDomainSevDefPtr sev)
> +{
if (!sev)
return;
> + virBufferAddLit(buf, "<launch-security type='sev'>\n");
Since we shouldn't be assuming 'sev' at this point
virBufferAsprintf(buf, "<launch-security type='%s'>\n",
virDomainLaunchSecurityTypeToString(def->sectype));
> + 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);
> + virBufferAsprintf(buf, "<policy>%d</policy>\n", sev->policy);
This has been defined as a "0x" number, but you're printing in decimal
format. Let's print in 0x value.
[1] Also if you leave this a 0 (zero), then you can compare sev->policy
> 0 before printing, e.g.:
if (sev->policy)
virBufferAsprintf(buf, "<policy>0x%x</policy>\n", sev->policy);
you may also want to do a "0x%04x"
> + if (sev->dh_cert)
> + virBufferAsprintf(buf, "<dh_cert>%s</dh_cert>\n", sev->dh_cert);
Use virBufferEscapeString
> +
> + if (sev->session)
> + virBufferAsprintf(buf, "<session>%s</session>\n", sev->session);
Use virBufferEscapeString
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</launch-security>\n");
> +}
> +
> +
> +static void
> virDomainPerfDefFormat(virBufferPtr buf, virDomainPerfDefPtr perf)
> {
> size_t i;
> @@ -27258,6 +27365,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> if (def->keywrap)
> virDomainKeyWrapDefFormat(buf, def->keywrap);
>
> + if (def->sev)
> + virDomainSevDefFormat(buf, def->sev);
> +
See above, no need for the if (def->sev)
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</domain>\n");
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 61379e5..60f9dd2 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
> */
> @@ -2290,6 +2293,25 @@ struct _virDomainKeyWrapDef {
> };
>
> typedef enum {
> + VIR_DOMAIN_LAUNCH_SECURITY_NONE,
> + VIR_DOMAIN_LAUNCH_SECURITY_SEV,
> +
> + VIR_DOMAIN_LAUNCH_SECURITY_LAST,
> +} virDomainLaunchSecurity;
> +
> +typedef struct _virDomainSevDef virDomainSevDef;
> +typedef virDomainSevDef *virDomainSevDefPtr;
> +
> +struct _virDomainSevDef {
int sectype; /* enum virDomainLaunchSecurity */
John
> + char *dh_cert;
> + char *session;
> + unsigned int policy;
> + unsigned int cbitpos;
> + unsigned int reduced_phys_bits;
> +};
> +
> +
> +typedef enum {
> VIR_DOMAIN_IOMMU_MODEL_INTEL,
>
> VIR_DOMAIN_IOMMU_MODEL_LAST
> @@ -2454,6 +2476,9 @@ struct _virDomainDef {
>
> virDomainKeyWrapDefPtr keywrap;
>
> + /* SEV-specific domain */
> + virDomainSevDefPtr sev;
> +
> /* Application-specific custom metadata */
> xmlNodePtr metadata;
>
> @@ -3345,6 +3370,7 @@ VIR_ENUM_DECL(virDomainMemorySource)
> VIR_ENUM_DECL(virDomainMemoryAllocation)
> VIR_ENUM_DECL(virDomainIOMMUModel)
> VIR_ENUM_DECL(virDomainShmemModel)
> +VIR_ENUM_DECL(virDomainLaunchSecurity)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
>
More information about the libvir-list
mailing list