[libvirt] [PATCH v6 6/9] libvirt: add new public API to get launch security info

[Erik Skultety]

On Fri, Jun 01, 2018 at 11:34:19AM -0500, Brijesh Singh wrote:
>
>
> On 05/28/2018 09:36 AM, Erik Skultety wrote:
> > On Wed, May 23, 2018 at 04:18:31PM -0500, Brijesh Singh wrote:
> > > The API can be used outside the libvirt to get the launch security
> > > information. When SEV is enabled, the API can be used to get the
> > > measurement of the launch process.
> > >
> > > Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> > > ---
> > >   include/libvirt/libvirt-domain.h | 17 ++++++++++++++
> > >   src/driver-hypervisor.h          |  7 ++++++
> > >   src/libvirt-domain.c             | 48 ++++++++++++++++++++++++++++++++++++++++
> > >   src/libvirt_public.syms          |  5 +++++
> > >   4 files changed, 77 insertions(+)
> > >
> > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > index d7cbd187969d..f252d18da72f 100644
> > > --- a/include/libvirt/libvirt-domain.h
> > > +++ b/include/libvirt/libvirt-domain.h
> > > @@ -4764,4 +4764,21 @@ int virDomainSetLifecycleAction(virDomainPtr domain,
> > >                                   unsigned int action,
> > >                                   unsigned int flags);
> > >
> > > +/**
> > > + * Launch Security API
> > > + */
> > > +
> > > +/**
> > > + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT:
> > > + *
> > > + * Macro represents the launch measurement of the SEV guest,
> > > + * as VIR_TYPED_PARAM_STRING.
> > > + */
> > > +#define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement"
> >
> > fails make syntax-check because of indentation, should be "# define ..."
> >
> > ...
> >
> > > diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> > > index 95df3a0dbc7b..5ccae5da8883 100644
> > > --- a/src/libvirt_public.syms
> > > +++ b/src/libvirt_public.syms
> > > @@ -785,4 +785,9 @@ LIBVIRT_4.1.0 {
> > >           virStoragePoolLookupByTargetPath;
> > >   } LIBVIRT_3.9.0;
> > >
> > > +LIBVIRT_4.4.0 {
> > > +    global:
> > > +        virDomainGetLaunchSecurityInfo;
> > > +} LIBVIRT_4.1.0;
> >
> > Will most probably become 4.5.0 :(
> >
> > Technically, I don't have any notes related to the functional changes,
> > therefore I'd give you my RB, however, I still find the naming confusing and I
> > can't think of something better. What if one day we'll actually be able to/need
> > to modify the configuration for some reason, we should reserve a name like this
> > for future modifications of launch-security data of the guest. Next, you're
> > preparing for adding support for some kind of setter in the virsh command, any
> > idea of what the setter data might be? Because I can imagine that you'd still
> > want to perform a measurement, but want to send additional arguments, to the
> > remote side's firmware to change the behaviour of the measurement and you can't
> > do this with a simple flag, you also need typed params for that which means
> > wou'd end up with something like:
> >
> > int
> > virDomainLaunchSecurityMeasure(virDomainPtr domain,
> >                                 virTypedParamsPtr send_params,
> >                                 unsigned int nsend_params,
> >                                 virTypedParamsPtr *recv_params,
> >                                 unsigned int nrecv_params);
> >
>
>
> Actually we had similar API in previous patches but I followed the review
> feedback from earlier versions where it was hinted to use launch-security so
> that in future if any other architecture or platform provides the similar
> feature but with different naming then we don't have to introduce yet
> another APIs.

Yes I saw that one, but that one was IMHO more about not having 'SEV' in the
name, so it wouldn't be platform specific. Even if some other vendor comes up
with a different technique, they either will do some kind of validation before
starting in which case 'Measure' could seems like an appropriate name or they
won't need it at all because they're going to use a different approach.
On the other hand, we're going into v7 and nobody has spoken up since the
virDomainGetLaunchSecurityInfo suggestion, so I'll stop the bikeshed I started
and let's go with what you've got here now.

>
> After guest owner validates the measurement, it will provide some additional
> information to VM. Typically this may include the some type of secret
> information and we may need to pass the following inputs via the API.
>
> - secret blob
> - length of the blob
> - the memory address where the blob need to be copied
>

Okay, so this is after the measurement, thanks.

There was actually another question hidden in my thoughts in the previous reply
where I described a potential need for send arguments to somehow augment the
process of measuring, e.g. passing some kind of nonce or a seed to the FW for
some reason when you're invoking the API above to get the measurement, because
you can't do that with a simple flag. But that should be a separate setter so
as not to overcomplicate things, this could potentially be achieved by the
setter API you're going to introduce at some point and simply adding typed
parameters for that.

Thanks,
Erik