[RFC PATCH 3/3] qemu: Implement the virDomainInjectLaunchSecret API

[Jim Fehlig]

On 11/17/21 03:39, Peter Krempa wrote:
> On Tue, Nov 16, 2021 at 19:23:54 -0700, Jim Fehlig wrote:
>> Inject a launch secret in domain memory using the sev-inject-launch-secret
>> QMP API. Only supported for SEV-enabed domains.
>>
>> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
>> ---
>>   src/qemu/qemu_driver.c       | 53 ++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_monitor.c      | 12 ++++++++
>>   src/qemu/qemu_monitor.h      |  6 ++++
>>   src/qemu/qemu_monitor_json.c | 34 +++++++++++++++++++++++
>>   src/qemu/qemu_monitor_json.h |  5 ++++
>>   5 files changed, 110 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d954635dde..58e3f08afe 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -20104,6 +20104,58 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
>>       return ret;
>>   }
>>   
>> +
>> +static int
>> +qemuDomainInjectLaunchSecret(virDomainPtr domain,
>> +                             const char *secrethdr,
>> +                             const char *secret,
>> +                             unsigned long long injectaddr,
>> +                             unsigned int flags)
>> +{
>> +    virQEMUDriver *driver = domain->conn->privateData;
>> +    virDomainObj *vm;
>> +    int ret = -1;
>> +
>> +    virCheckFlags(0, -1);
>> +
>> +    if (!(vm = qemuDomainObjFromDomain(domain)))
>> +        goto cleanup;
>> +
>> +    if (virDomainInjectLaunchSecretEnsureACL(domain->conn, vm->def) < 0)
>> +        goto cleanup;
>> +
>> +    /* Currently only SEV is supported */
>> +    if (!vm->def->sec ||
>> +        vm->def->sec->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) {
>> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                       _("injecting a launch secret is only supported in SEV-enabled domains"));
>> +        goto cleanup;
>> +    }
> 
> Missing check that the VM is running.
> 
>> +
>> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> +        goto cleanup;
>> +
>> +    if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
>> +        goto endjob;
> 
> No need for async monitor without an async job
> 
>> +
>> +    if (qemuMonitorInjectLaunchSecret(QEMU_DOMAIN_PRIVATE(vm)->mon,
>> +                                      secrethdr, secret, injectaddr) < 0)
>> +        goto endjob;
>> +
>> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> +        goto endjob;
>> +
>> +    ret = 0;
>> +
>> + endjob:
>> +    qemuDomainObjEndJob(driver, vm);
>> +
>> + cleanup:
> 
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 810dac209d..c64469a03b 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -4383,6 +4383,18 @@ qemuMonitorGetSEVMeasurement(qemuMonitor *mon)
>>   }
>>   
>>   
>> +int
>> +qemuMonitorInjectLaunchSecret(qemuMonitor *mon,
>> +                              const char *secrethdr,
>> +                              const char *secret,
>> +                              unsigned long long injectaddr)
>> +{
>> +    QEMU_CHECK_MONITOR(mon);
> 
> You cleverly avoid logging the secret here ...
> 
>> +
>> +    return qemuMonitorJSONInjectLaunchSecret(mon, secrethdr, secret, injectaddr);
>> +}
>> +
>> +
>>   int
>>   qemuMonitorGetPRManagerInfo(qemuMonitor *mon,
>>                               GHashTable **retinfo)
> 
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 4669b9135d..69aef078ec 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -8124,6 +8124,40 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon)
>>   }
>>   
>>   
>> +/**
>> + * The function is used to inject a launch secret in an SEV guest.
>> + *
>> + * Example JSON:
>> + *
>> + * { "execute" : "sev-inject-launch-secret",
>> + *   "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } }
>> + */
>> +int
>> +qemuMonitorJSONInjectLaunchSecret(qemuMonitor *mon,
>> +                                  const char *secrethdr,
>> +                                  const char *secret,
>> +                                  unsigned long long injectaddr)
>> +{
>> +    g_autoptr(virJSONValue) cmd = NULL;
>> +    g_autoptr(virJSONValue) reply = NULL;
>> +
>> +    if (!(cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret",
>> +                                           "s:packet-header", secrethdr,
>> +                                           "s:secret", secret,
>> +                                           "U:gpa", injectaddr,
>> +                                           NULL)))
>> +        return -1;
>> +
>> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> 
> ... but once this is sent to the monitor it can be logged if QMP traffic
> logging is enabled.
> 
> Thus either the secret is not so secret, but the qemu documentation nor
> the documentation you've added seem to be lessening the secrecy of the
> passed string in any way, or the 'sev-inject-launch-secret' is broken by
> design as it doesn't support passing an encrypted object via qcrypto.

AFAICT, the secret is an encrypted object. See section "6.6 LAUNCH_SECRET" in 
the following doc

https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf

I'll need to do more snooping so I can improve the API documentation in 1/3. A 
clearer description of the API and its parameters would help with questions like 
this.

Regards,
Jim

> 
> In case when the secret must really be kept secret the QMP command must
> be fixed first, otherwise libvirt is not able to guarantee any secrecy.
> 
>> +        return -1;
>> +
>> +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>