QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
VMs on AMD platform using SEV feature. The various inputs required to
launch SEV guest is provided through the <launch-security> tag. A typical
SEV guest launch command line looks like this:

# $QEMU ...\
  -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
  -machine memory-encryption=sev0 \

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++
 src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 682d714..55bbfa2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
             virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
             qemuAppendLoadparmMachineParm(&buf, def);
 
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
+            virBufferAddLit(&buf, ",memory-encryption=sev0");
+
         virCommandAddArgBuffer(cmd, &buf);
     }
 
@@ -9750,6 +9753,35 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
     return 0;
 }
 
+static void
+qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
+                        virDomainSevDefPtr sev)
+{
+    virBuffer obj = VIR_BUFFER_INITIALIZER;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    char *path = NULL;
+
+    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
+              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
+
+    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
+    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
+    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
+
+    if (sev->dh_cert) {
+        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir));
+        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
+        VIR_FREE(path);
+    }
+
+    if (sev->session) {
+        ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir));
+        virBufferAsprintf(&obj, ",session-file=%s", path);
+        VIR_FREE(path);
+    }
+
+    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
+}
 
 static int
 qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
@@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
     if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
         goto error;
 
+    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
+        qemuBuildSevCommandLine(vm, cmd, def->sev);
+
     if (snapshot)
         virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c0105c8..0c93f15 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5745,6 +5745,61 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
     return ret;
 }
 
+static int
+qemuBuildSevCreateFile(const char *configDir, const char *name,
+                       const char *data)
+{
+    char *configFile;
+
+    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
+        return -1;
+
+    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
+        virReportSystemError(errno, _("failed to write data to config '%s'"),
+                             configFile);
+        goto error;
+    }
+
+    VIR_FREE(configFile);
+    return 0;
+
+ error:
+    VIR_FREE(configFile);
+    return -1;
+}
+
+static int
+qemuProcessPrepareSevGuestInput(virDomainObjPtr vm)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    virDomainDefPtr def = vm->def;
+    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
+    virDomainSevDefPtr sev = def->sev;
+
+    if (!sev)
+        return 0;
+
+    VIR_DEBUG("Prepare SEV guest");
+
+    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                        _("Domain %s asked for 'sev' launch but "
+                          "QEMU does not support SEV feature"), vm->def->name);
+        return -1;
+    }
+
+    if (sev->dh_cert) {
+        if (qemuBuildSevCreateFile(priv->libDir, "dh_cert", sev->dh_cert) < 0)
+            return -1;
+    }
+
+    if (sev->session) {
+        if (qemuBuildSevCreateFile(priv->libDir, "session", sev->session) < 0)
+            return -1;
+    }
+
+    return 0;
+}
 
 static int
 qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
@@ -5870,6 +5925,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
     if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0)
         goto cleanup;
 
+    if (qemuProcessPrepareSevGuestInput(vm) < 0)
+        goto cleanup;
+
     ret = 0;
  cleanup:
     virObjectUnref(cfg);
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/02/2018 10:18 AM, Brijesh Singh wrote:
> QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
> VMs on AMD platform using SEV feature. The various inputs required to
> launch SEV guest is provided through the <launch-security> tag. A typical
> SEV guest launch command line looks like this:
> 
> # $QEMU ...\
>   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
>   -machine memory-encryption=sev0 \
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++
>  src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 

(slight delay for next part of review - today was rocket launch day and
then we headed out for a bit ;-))

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 682d714..55bbfa2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>              virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>              qemuAppendLoadparmMachineParm(&buf, def);
>  
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)

Since we already checked sev-guest at prepare host storage (mostly
unconditionally), I don't think we have to make the check here as well -
although I could be wrong...

> +            virBufferAddLit(&buf, ",memory-encryption=sev0");
> +
>          virCommandAddArgBuffer(cmd, &buf);
>      }
>  
> @@ -9750,6 +9753,35 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
>      return 0;
>  }
>  
> +static void

This probably should be an int...

> +qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
> +                        virDomainSevDefPtr sev)
> +{
> +    virBuffer obj = VIR_BUFFER_INITIALIZER;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    char *path = NULL;
> +

    if (!dev->sev)
        return 0;

again, since prepare host storage checked the sev-guest capability we
should be good to go here...

> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
> +              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
> +
> +    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
> +    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);

Here I would say:

    if (sev->policy > 0)
        virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);

and let qemu pick the default (which is 0x1 as I read that code).

> +
> +    if (sev->dh_cert) {
> +        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir));
> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
> +        VIR_FREE(path);
> +    }
> +
> +    if (sev->session) {
> +        ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir));
> +        virBufferAsprintf(&obj, ",session-file=%s", path);
> +        VIR_FREE(path);
> +    }

...since I don't believe we can ignore_value on the paths - especially
since we're using it to create a path to a file containing some sort of
session info or DH key (base64 encrypted).

> +
> +    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
> +}
>  
>  static int
>  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
> @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
>          goto error;
>  
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
> +        qemuBuildSevCommandLine(vm, cmd, def->sev);
> +

I think we're save to change this to:

    if (qemuBuildSevCommandLine(vm, cmd, dev->sev) < 0)
        goto error;

>      if (snapshot)
>          virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c0105c8..0c93f15 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5745,6 +5745,61 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
>      return ret;
>  }
>  

Two blank lines

> +static int
> +qemuBuildSevCreateFile(const char *configDir, const char *name,
> +                       const char *data)

3 lines for args

> +{
> +    char *configFile;
> +
> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
> +        return -1;
> +
> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
> +                             configFile);
> +        goto error;
> +    }

Check out storageBackendCreateQemuImgSecretPath which just goes straight
to safewrite when writing to the file or qemuDomainWriteMasterKeyFile
which is a similar w/r/t a single key file for the domain.

The one thing to think about being the privileges for the file being
created and written and the expectations for QEMU's usage. I think this
is more like the storage secret code, but I could be wrong!

> +
> +    VIR_FREE(configFile);
> +    return 0;
> +
> + error:
> +    VIR_FREE(configFile);
> +    return -1;
> +}
> +

2 blank lines

> +static int
> +qemuProcessPrepareSevGuestInput(virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainDefPtr def = vm->def;
> +    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> +    virDomainSevDefPtr sev = def->sev;
> +
> +    if (!sev)
> +        return 0;
> +
> +    VIR_DEBUG("Prepare SEV guest");
> +
> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Domain %s asked for 'sev' launch but "

s/but/but this/

> +                          "QEMU does not support SEV feature"), vm->def->name);
> +        return -1;
> +    }

Since we check for this here - I think this should be good enough for
command line building.  That is - qemuProcessPrepareHostStorage will
come before command line building - so the check is already made.

> +
> +    if (sev->dh_cert) {
> +        if (qemuBuildSevCreateFile(priv->libDir, "dh_cert", sev->dh_cert) < 0)
> +            return -1;
> +    }
> +
> +    if (sev->session) {
> +        if (qemuBuildSevCreateFile(priv->libDir, "session", sev->session) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}

2 blank lines

John

>  
>  static int
>  qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
> @@ -5870,6 +5925,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>      if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0)
>          goto cleanup;
>  
> +    if (qemuProcessPrepareSevGuestInput(vm) < 0)
> +        goto cleanup;
> +
>      ret = 0;
>   cleanup:
>      virObjectUnref(cfg);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 4/2/18 6:04 PM, John Ferlan wrote:
>
> On 04/02/2018 10:18 AM, Brijesh Singh wrote:
>> QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
>> VMs on AMD platform using SEV feature. The various inputs required to
>> launch SEV guest is provided through the <launch-security> tag. A typical
>> SEV guest launch command line looks like this:
>>
>> # $QEMU ...\
>>   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
>>   -machine memory-encryption=sev0 \
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++
>>  src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 93 insertions(+)
>>
> (slight delay for next part of review - today was rocket launch day and
> then we headed out for a bit ;-))
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 682d714..55bbfa2 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>>              virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
>>              qemuAppendLoadparmMachineParm(&buf, def);
>>  
>> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
> Since we already checked sev-guest at prepare host storage (mostly
> unconditionally), I don't think we have to make the check here as well -
> although I could be wrong...


Yes, probably we can avoid the SEV_GUEST cap check in this case. I will
make the changes in next rev.

>> +            virBufferAddLit(&buf, ",memory-encryption=sev0");
>> +
>>          virCommandAddArgBuffer(cmd, &buf);
>>      }
>>  
>> @@ -9750,6 +9753,35 @@ qemuBuildTPMCommandLine(virCommandPtr cmd,
>>      return 0;
>>  }
>>  
>> +static void
> This probably should be an int...

I think it will be unlikely that this function will fail hence I was
using void. The only time it can fail is when we fail to create a
dh-cert and session blob file (e.g disk is full). I will propagate the
error.

>> +qemuBuildSevCommandLine(virDomainObjPtr vm, virCommandPtr cmd,
>> +                        virDomainSevDefPtr sev)
>> +{
>> +    virBuffer obj = VIR_BUFFER_INITIALIZER;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    char *path = NULL;
>> +
>     if (!dev->sev)
>         return 0;
>
> again, since prepare host storage checked the sev-guest capability we
> should be good to go here...

OK.

>> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
>> +              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
>> +
>> +    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
>> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
>> +    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
> Here I would say:
>
>     if (sev->policy > 0)
>         virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
>
> and let qemu pick the default (which is 0x1 as I read that code).

OK, then I will remove the comment from html about the default value
since I was trying to not depend on QEMU default.

>> +
>> +    if (sev->dh_cert) {
>> +        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir));
>> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
>> +        VIR_FREE(path);
>> +    }
>> +
>> +    if (sev->session) {
>> +        ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir));
>> +        virBufferAsprintf(&obj, ",session-file=%s", path);
>> +        VIR_FREE(path);
>> +    }
> ...since I don't believe we can ignore_value on the paths - especially
> since we're using it to create a path to a file containing some sort of
> session info or DH key (base64 encrypted).

Sure, will do.

>> +
>> +    virCommandAddArgList(cmd, "-object", virBufferContentAndReset(&obj), NULL);
>> +}
>>  
>>  static int
>>  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
>> @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>>      if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
>>          goto error;
>>  
>> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
>> +        qemuBuildSevCommandLine(vm, cmd, def->sev);
>> +
> I think we're save to change this to:
>
>     if (qemuBuildSevCommandLine(vm, cmd, dev->sev) < 0)
>         goto error;
>

Yep

>>      if (snapshot)
>>          virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL);
>>  
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index c0105c8..0c93f15 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5745,6 +5745,61 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
>>      return ret;
>>  }
>>  
> Two blank lines
>
>> +static int
>> +qemuBuildSevCreateFile(const char *configDir, const char *name,
>> +                       const char *data)
> 3 lines for args
>
>> +{
>> +    char *configFile;
>> +
>> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
>> +        return -1;
>> +
>> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
>> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
>> +                             configFile);
>> +        goto error;
>> +    }
> Check out storageBackendCreateQemuImgSecretPath which just goes straight
> to safewrite when writing to the file or qemuDomainWriteMasterKeyFile
> which is a similar w/r/t a single key file for the domain.
>
> The one thing to think about being the privileges for the file being
> created and written and the expectations for QEMU's usage. I think this
> is more like the storage secret code, but I could be wrong!

The data is public in this case, we do not need to protect it with
secret. Hence I am keeping all this certificate keys in unsecure place.
>> +
>> +    VIR_FREE(configFile);
>> +    return 0;
>> +
>> + error:
>> +    VIR_FREE(configFile);
>> +    return -1;
>> +}
>> +
> 2 blank lines
>
>> +static int
>> +qemuProcessPrepareSevGuestInput(virDomainObjPtr vm)
>> +{
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virDomainDefPtr def = vm->def;
>> +    virQEMUCapsPtr qemuCaps = priv->qemuCaps;
>> +    virDomainSevDefPtr sev = def->sev;
>> +
>> +    if (!sev)
>> +        return 0;
>> +
>> +    VIR_DEBUG("Prepare SEV guest");
>> +
>> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                        _("Domain %s asked for 'sev' launch but "
> s/but/but this/

Noted.

>
>> +                          "QEMU does not support SEV feature"), vm->def->name);
>> +        return -1;
>> +    }
> Since we check for this here - I think this should be good enough for
> command line building.  That is - qemuProcessPrepareHostStorage will
> come before command line building - so the check is already made.
>

Got it.

>> +
>> +    if (sev->dh_cert) {
>> +        if (qemuBuildSevCreateFile(priv->libDir, "dh_cert", sev->dh_cert) < 0)
>> +            return -1;
>> +    }
>> +
>> +    if (sev->session) {
>> +        if (qemuBuildSevCreateFile(priv->libDir, "session", sev->session) < 0)
>> +            return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 2 blank lines
>
> John
>
>>  
>>  static int
>>  qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>> @@ -5870,6 +5925,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver,
>>      if (qemuProcessPrepareHostStorage(driver, vm, flags) < 0)
>>          goto cleanup;
>>  
>> +    if (qemuProcessPrepareSevGuestInput(vm) < 0)
>> +        goto cleanup;
>> +
>>      ret = 0;
>>   cleanup:
>>      virObjectUnref(cfg);
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>> +    VIR_DEBUG("policy=0x%x cbitpos=%d reduced_phys_bits=%d",
>>> +              sev->policy, sev->cbitpos, sev->reduced_phys_bits);
>>> +
>>> +    virBufferAsprintf(&obj, "sev-guest,id=sev0,cbitpos=%d", sev->cbitpos);
>>> +    virBufferAsprintf(&obj, ",reduced-phys-bits=%d", sev->reduced_phys_bits);
>>> +    virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
>> Here I would say:
>>
>>     if (sev->policy > 0)
>>         virBufferAsprintf(&obj, ",policy=0x%x", sev->policy);
>>
>> and let qemu pick the default (which is 0x1 as I read that code).
> 
> OK, then I will remove the comment from html about the default value
> since I was trying to not depend on QEMU default.
> 

I understand - your other option is to make required. This is one of
those cases where there is a gray area with respect to libvirt picking
some default or policy that we generally prefer to avoid.

As noted before/elsewhere - what happens when the default changes...

>>> +
>>> +    if (sev->dh_cert) {
>>> +        ignore_value(virAsprintf(&path, "%s/dh_cert.base64", priv->libDir));
>>> +        virBufferAsprintf(&obj, ",dh-cert-file=%s", path);
>>> +        VIR_FREE(path);
>>> +    }
>>> +
>>> +    if (sev->session) {
>>> +        ignore_value(virAsprintf(&path, "%s/session.base64", priv->libDir));
>>> +        virBufferAsprintf(&obj, ",session-file=%s", path);
>>> +        VIR_FREE(path);
>>> +    }
[...]

>>> +qemuBuildSevCreateFile(const char *configDir, const char *name,
>>> +                       const char *data)
>> 3 lines for args
>>
>>> +{
>>> +    char *configFile;
>>> +
>>> +    if (!(configFile = virFileBuildPath(configDir, name, ".base64")))
>>> +        return -1;
>>> +
>>> +    if (virFileRewriteStr(configFile, S_IRUSR | S_IWUSR, data) < 0) {
>>> +        virReportSystemError(errno, _("failed to write data to config '%s'"),
>>> +                             configFile);
>>> +        goto error;
>>> +    }
>> Check out storageBackendCreateQemuImgSecretPath which just goes straight
>> to safewrite when writing to the file or qemuDomainWriteMasterKeyFile
>> which is a similar w/r/t a single key file for the domain.
>>
>> The one thing to think about being the privileges for the file being
>> created and written and the expectations for QEMU's usage. I think this
>> is more like the storage secret code, but I could be wrong!
> 
> The data is public in this case, we do not need to protect it with
> secret. Hence I am keeping all this certificate keys in unsecure place.

It wasn't so much the public keys as it was me (more or less) thinking
out loud about the protections on the file that you're "temporarily"
creating and using to pass the keys.

I noted two other areas which libvirt does something similarly - one is
the master public key file for decrypting the AES secrets for
libvirt/qemu secret manipulation.  The second is the "temporary" file we
create in the storage driver to handle the luks encryption password for
create/resize of a luks encrypted file when using qemu-img. Now that is
slightly different than using a temporary file for the emulator binary.

In any case, since you're creating in libDir it's probably OK as is, but
I know when reading files libvirt creates which qemu will use there have
been issues in the past - I always have to refresh my memory what those
issues are though.

John

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/04/2018 09:22 AM, John Ferlan wrote:
> 
> I understand - your other option is to make required. This is one of
> those cases where there is a gray area with respect to libvirt picking
> some default or policy that we generally prefer to avoid.
> 

I think now I am more inclined towards making it required from libvirt 
-- this matches with SEV spec.


> As noted before/elsewhere - what happens when the default changes...
> 


Theoretically speaking, change in default policy should not cause any 
issue when creating the guest. But the policy change can limit us what a 
hypervisor can do after the boots, e.g disable or disable debug, 
migration or save/restore etc.


On side note, I will be going on 4 weeks of paternity leave starting 
next week and will submit the series after I am back from leave.


>>>> +    }
>>> Check out storageBackendCreateQemuImgSecretPath which just goes straight
>>> to safewrite when writing to the file or qemuDomainWriteMasterKeyFile
>>> which is a similar w/r/t a single key file for the domain.
>>>
>>> The one thing to think about being the privileges for the file being
>>> created and written and the expectations for QEMU's usage. I think this
>>> is more like the storage secret code, but I could be wrong!
>>
>> The data is public in this case, we do not need to protect it with
>> secret. Hence I am keeping all this certificate keys in unsecure place.
> 
> It wasn't so much the public keys as it was me (more or less) thinking
> out loud about the protections on the file that you're "temporarily"
> creating and using to pass the keys.
> 
> I noted two other areas which libvirt does something similarly - one is
> the master public key file for decrypting the AES secrets for
> libvirt/qemu secret manipulation.  The second is the "temporary" file we
> create in the storage driver to handle the luks encryption password for
> create/resize of a luks encrypted file when using qemu-img. Now that is
> slightly different than using a temporary file for the emulator binary.
> 
> In any case, since you're creating in libDir it's probably OK as is, but
> I know when reading files libvirt creates which qemu will use there have
> been issues in the past - I always have to refresh my memory what those
> issues are though.
> 


IIRC, Daniel recommended to use libDir in previous reviews, if its not a 
big deal then we lets use libDir in initial patches and if need arises 
then we can revisit it later.

thanks for all your feedback.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Apr 02, 2018 at 07:04:25PM -0400, John Ferlan wrote:
>
>
> On 04/02/2018 10:18 AM, Brijesh Singh wrote:
> > QEMU >= 2.12 provides 'sev-guest' object which is used to launch encrypted
> > VMs on AMD platform using SEV feature. The various inputs required to
> > launch SEV guest is provided through the <launch-security> tag. A typical
> > SEV guest launch command line looks like this:
> >
> > # $QEMU ...\
> >   -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=5 ...\
> >   -machine memory-encryption=sev0 \
> >
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > ---
> >  src/qemu/qemu_command.c | 35 +++++++++++++++++++++++++++++
> >  src/qemu/qemu_process.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 93 insertions(+)
> >
>
> (slight delay for next part of review - today was rocket launch day and
> then we headed out for a bit ;-))
>
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 682d714..55bbfa2 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -7405,6 +7405,9 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> >              virQEMUCapsGet(qemuCaps, QEMU_CAPS_LOADPARM))
> >              qemuAppendLoadparmMachineParm(&buf, def);
> >
> > +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
>
> Since we already checked sev-guest at prepare host storage (mostly
> unconditionally), I don't think we have to make the check here as well -
> although I could be wrong...

I guess you surely meant qemuProcessPrepareSevGuestInput, but you're right, we
don't need it.

...
> >  static int
> >  qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
> > @@ -10195,6 +10227,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
> >      if (qemuBuildVMCoreInfoCommandLine(cmd, def, qemuCaps) < 0)
> >          goto error;
> >
> > +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST) && def->sev)
> > +        qemuBuildSevCommandLine(vm, cmd, def->sev);
> > +
>
> I think we're save to change this to:

Yep.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list