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

[Jim Fehlig <jfehlig@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> >   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;
> > +}