Re: [PATCH V3 3/4] qemu: Implement the virDomainSetLaunchSecurityState API

[Jim Fehlig <jfehlig@xxxxxxxx>]

Hi John,

Thanks for taking a look!

On 12/17/21 12:48, John Ferlan wrote:
> On 12/14/21 23:46, Jim Fehlig wrote:
> > Set a launch secret in guest memory using the sev-inject-launch-secret
> > QMP API. Only supported for SEV-enabled guests in a paused state.
> >
> > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> > ---
> >   src/qemu/qemu_driver.c       | 88 ++++++++++++++++++++++++++++++++++++
> >   src/qemu/qemu_monitor.c      | 14 ++++++
> >   src/qemu/qemu_monitor.h      |  7 +++
> >   src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++
> >   src/qemu/qemu_monitor_json.h |  6 +++
> >   tests/qemumonitorjsontest.c  |  3 ++
> >   6 files changed, 163 insertions(+)
>
> Just checking - not active day to day, but this technology is what some of my 
> team works on now... I only realized this because Dan pointed it out and I was 
> trying to help someone new on my team see how commands are created in libvirt. 
> He's more familiar with sevctl processing, but will need to contribute to 
> libvirt/qemu in the coming months.
>
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index aae622ea5d..889892a097 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -20070,6 +20070,93 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain,
> >       return ret;
> >   }
> > +
> > +static int
> > +qemuDomainSetLaunchSecurityState(virDomainPtr domain,
> > +                                 virTypedParameterPtr params,
> > +                                 int nparams,
> > +                                 unsigned int flags)
> > +{
> > +    virQEMUDriver *driver = domain->conn->privateData;
> > +    virDomainObj *vm;
> > +    int ret = -1;
> > +    int rc;
> > +    size_t i;
> > +    g_autofree char *secrethdr = NULL;
> > +    g_autofree char *secret = NULL;
> > +    unsigned long long setaddr = 0;
> > +    bool hasSetaddr = false;
> > +    int state;
> > +
> > +    virCheckFlags(0, -1);
> > +    if (virTypedParamsValidate(params, nparams,
> > +                               VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER,
> > +                               VIR_TYPED_PARAM_STRING,
> > +                               VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET,
> > +                               VIR_TYPED_PARAM_STRING,
> > +                               
> > VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS,
> > +                               VIR_TYPED_PARAM_ULLONG,
> > +                               NULL) < 0)
> > +        return -1;
> > +
> > +    if (!(vm = qemuDomainObjFromDomain(domain)))
> > +        goto cleanup;
> > +
> > +    if (virDomainSetLaunchSecurityStateEnsureACL(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",
> > +                       _("setting a launch secret is only supported in 
> > SEV-enabled domains"));
> > +        goto cleanup;
> > +    }
>
> Would we need a specific capability check here before calling 
> sev-inject-launch-secret since it's only available since qemu-6.0?

Good point. I made an attempt at including a caps check in V4

https://listman.redhat.com/archives/libvir-list/2021-December/msg00800.html

Regards,
Jim

> Seems to me VIR_DOMAIN_LAUNCH_SECURITY_SEV would be keyed off the presence of 
> the sev-guest object only. That would seem to be a qemu 2.12 type check. It's 
> been a while since I looked at sources, so I'm rusty, but I think 
> qemu_validate.c/qemuValidateDomainDef would check for QEMU_CAPS_SEV_GUEST only.
>
> This is unlike the query-sev-capabilities and query-sev-launch-measure which are 
> present in 2.12.
>
> John
>
> > +
> > +    for (i = 0; i < nparams; i++) {
> > +        virTypedParameterPtr param = &params[i];
> > +
> > +        if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER)) {
> > +            secrethdr = g_strdup(param->value.s);
> > +        } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET)) {
> > +            secret = g_strdup(param->value.s);
> > +        } else if (STREQ(param->field, 
> > VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS)) {
> > +            setaddr =  param->value.ul;
> > +            hasSetaddr = true;
> > +        }
> > +    }
> > +
> > +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> > +        goto cleanup;
> > +
> > +    if (virDomainObjCheckActive(vm) < 0)
> > +        goto endjob;
> > +
> > +    state = virDomainObjGetState(vm, NULL);
> > +    if (state != VIR_DOMAIN_PAUSED) {
> > +        virReportError(VIR_ERR_OPERATION_INVALID,
> > +                       "%s", _("domain must be in a paused state"));
> > +        goto endjob;
> > +    }
> > +
> > +    qemuDomainObjEnterMonitor(driver, vm);
> > +    rc = qemuMonitorSetLaunchSecurityState(QEMU_DOMAIN_PRIVATE(vm)->mon,
> > +                                           secrethdr, secret, setaddr, 
> > hasSetaddr);
> > +    qemuDomainObjExitMonitor(driver, vm);
> > +    if (rc < 0)
> > +        goto endjob;
> > +
> > +    ret = 0;
> > +
> > + endjob:
> > +    qemuDomainObjEndJob(driver, vm);
> > +
> > + cleanup:
> > +    virDomainObjEndAPI(&vm);
> > +    return ret;
> > +}
> > +
> > +
> >   static const unsigned int qemuDomainGetGuestInfoSupportedTypes =
> >       VIR_DOMAIN_GUEST_INFO_USERS |
> >       VIR_DOMAIN_GUEST_INFO_OS |
> > @@ -20943,6 +21030,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
> >       .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */
> >       .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */
> >       .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */
> > +    .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 
> > 8.0.0 */
> >   };
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index dda6ae9796..5272d49c2e 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -4379,6 +4379,20 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon,
> >   }
> > +int
> > +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon,
> > +                                  const char *secrethdr,
> > +                                  const char *secret,
> > +                                  unsigned long long setaddr,
> > +                                  bool hasSetaddr)
> > +{
> > +    QEMU_CHECK_MONITOR(mon);
> > +
> > +    return qemuMonitorJSONSetLaunchSecurityState(mon, secrethdr, secret,
> > +                                                 setaddr, hasSetaddr);
> > +}
> > +
> > +
> >   int
> >   qemuMonitorGetPRManagerInfo(qemuMonitor *mon,
> >                               GHashTable **retinfo)
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index 29746f0b8e..87826e6268 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -1454,6 +1454,13 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon,
> >       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> >       ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
> > +int
> > +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon,
> > +                                  const char *secrethdr,
> > +                                  const char *secret,
> > +                                  unsigned long long setaddr,
> > +                                  bool hasSetaddr);
> > +
> >   typedef struct _qemuMonitorPRManagerInfo qemuMonitorPRManagerInfo;
> >   struct _qemuMonitorPRManagerInfo {
> >       bool connected;
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index a3d6eca569..37ee859a33 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -8262,6 +8262,51 @@ qemuMonitorJSONGetSEVInfo(qemuMonitor *mon,
> >   }
> > +/**
> > + * Set a launch secret in guest memory
> > + *
> > + * Example JSON:
> > + *
> > + * { "execute" : "sev-inject-launch-secret",
> > + *   "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } }
> > + *
> > + * The guest physical address (gpa) parameter is optional
> > + */
> > +int
> > +qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon,
> > +                                      const char *secrethdr,
> > +                                      const char *secret,
> > +                                      unsigned long long setaddr,
> > +                                      bool hasSetaddr)
> > +{
> > +    g_autoptr(virJSONValue) cmd = NULL;
> > +    g_autoptr(virJSONValue) reply = NULL;
> > +
> > +    if (hasSetaddr) {
> > +        cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret",
> > +                                         "s:packet-header", secrethdr,
> > +                                         "s:secret", secret,
> > +                                         "U:gpa", setaddr,
> > +                                         NULL);
> > +    } else {
> > +        cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret",
> > +                                         "s:packet-header", secrethdr,
> > +                                         "s:secret", secret,
> > +                                         NULL);
> > +    }
> > +    if (cmd == NULL)
> > +        return -1;
> > +
> > +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> > +        return -1;
> > +
> > +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > +
> > +
> >   /*
> >    * Example return data
> >    *
> > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> > index e88dfc9d50..64d9ebdaa3 100644
> > --- a/src/qemu/qemu_monitor_json.h
> > +++ b/src/qemu/qemu_monitor_json.h
> > @@ -476,6 +476,12 @@ qemuMonitorJSONGetVersion(qemuMonitor *mon,
> >                             char **package)
> >       ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
> > +int qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon,
> > +                                          const char *secrethdr,
> > +                                          const char *secret,
> > +                                          unsigned long long setaddr,
> > +                                          bool hasSetaddr);
> > +
> >   int
> >   qemuMonitorJSONGetMachines(qemuMonitor *mon,
> >                              qemuMonitorMachineInfo ***machines)
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 1b0bd0870d..48e2a457ab 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -1196,6 +1196,8 @@ GEN_TEST_FUNC(qemuMonitorJSONSetAction,
> >                 QEMU_MONITOR_ACTION_REBOOT_RESET,
> >                 QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN,
> >                 QEMU_MONITOR_ACTION_PANIC_SHUTDOWN)
> > +GEN_TEST_FUNC(qemuMonitorJSONSetLaunchSecurityState, "sev_secret_header",
> > +              "sev_secret", 0, true)
> >   static int
> >   testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque)
> > @@ -3067,6 +3069,7 @@ mymain(void)
> >       DO_TEST_GEN(qemuMonitorJSONJobComplete);
> >       DO_TEST_GEN(qemuMonitorJSONBlockJobCancel);
> >       DO_TEST_GEN(qemuMonitorJSONSetAction);
> > +    DO_TEST_GEN(qemuMonitorJSONSetLaunchSecurityState);
> >       DO_TEST(qemuMonitorJSONGetBalloonInfo);
> >       DO_TEST(qemuMonitorJSONGetBlockInfo);
> >       DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo);