[libvirt] [PATCH v6 3/9] conf: introduce launch-security element in domain
Brijesh Singh
brijesh.singh at amd.com
Tue May 29 16:36:27 UTC 2018
On 05/28/2018 05:57 AM, Erik Skultety wrote:
> 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 at 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.
As I said in cover letter patch response, the value need to be user
config to make the SEV launch migration safe. Consider the below example
1) user launches a guest on platform A.
On this platform hypervisor reports cbitpos=47 and reduced-phys-bit=1.
While creating the guest QEMU requires caller to specify the cbitpos and
reduced-phys-bit parameters. These params gets validate before creating
the guest. If the user specified values are acceptable then QEMU will
create the guest otherwise it will fail to create guest.
Typically cbitpos and reduced-phys-bit is platform and hypervisor
dependent. If user does not know the value then it can query it with
"virsh domcapabilities" before creating the domain XML file.
2) user tries to migrate the above guest to platform B.
a) If the platform B hypervisor supports the cbitpos and
reduced-phys-bits value used in #1 then we have no issues.
b) if platform B hypervisor does not support those value then we should
fail the migration. If we don't make the <cbitpos> and
<reduced-phys-bits> user configurable then we will end up pick the
different value compare to what was used during the launch flow.
>
>> + </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?
Good catch, thanks. I should be 6:15
>
>> + <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.
>
Noted.
>> + <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
>
OK. I will go ahead and Use SEV instead of 'Sev' in all other places in
function name to be consistent.
>> +{
>> + 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.
>
As explained above, <cbitpos> and <reduced-phys-bits> must be
configurable. We had good discussion about this topic here:
https://marc.info/?l=qemu-devel&m=151740619818655&w=2
>> +
>> + 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
>
More information about the libvir-list
mailing list