[libvirt] [PATCH v5 05/10] qemu: add support to launch SEV guest

[John Ferlan]

[...]

>>> +    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

[...]