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

2018-06-04 Thread 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 
> > > ---
> > >   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

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

2018-06-01 Thread Brijesh Singh 




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


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




And you can both send additional arguments as well as receive arguments
according to the remote implementation, it's IMHO safer, but it's extremely
ugly at the same time and we're hitting the 'one function does all' kind of
scenario which we also shouldn't do. At the same time though, we could say that
any arguments that you might need to alter the result of the measurement should
be available prior to launching the guest in its XML config file, that way,
you'll never need anything apart from the recv_params,recv_nparams and the name
you're suggesting can stay, since only by using flags you can tell libvirt
daemon whether you want to work with the info in the config or take a
measurement or something else.

This was just me thinking out loud and would like to get some input from you
and/or other reviewer because even though I'm okay with the code, I don't want
to make a decision we can't take back just yet.

Erik



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Erik Skultety 
On Mon, May 28, 2018 at 04:36:54PM +0200, 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 
> > ---

Putting Dan Berrange on CC to comment, forgot to do that in my original reply.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-28 Thread Erik Skultety 
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 
> ---
>  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);

And you can both send additional arguments as well as receive arguments
according to the remote implementation, it's IMHO safer, but it's extremely
ugly at the same time and we're hitting the 'one function does all' kind of
scenario which we also shouldn't do. At the same time though, we could say that
any arguments that you might need to alter the result of the measurement should
be available prior to launching the guest in its XML config file, that way,
you'll never need anything apart from the recv_params,recv_nparams and the name
you're suggesting can stay, since only by using flags you can tell libvirt
daemon whether you want to work with the info in the config or take a
measurement or something else.

This was just me thinking out loud and would like to get some input from you
and/or other reviewer because even though I'm okay with the code, I don't want
to make a decision we can't take back just yet.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2018-05-23 Thread Brijesh Singh 
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 
---
 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"
+
+int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
+   virTypedParameterPtr *params,
+   int *nparams,
+   unsigned int flags);
+
 #endif /* __VIR_LIBVIRT_DOMAIN_H__ */
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index e71a72a44132..bdf13536d337 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1286,6 +1286,12 @@ typedef int
   unsigned int action,
   unsigned int flags);
 
+typedef int
+(*virDrvDomainGetLaunchSecurityInfo)(virDomainPtr domain,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags);
+
 
 typedef struct _virHypervisorDriver virHypervisorDriver;
 typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1532,6 +1538,7 @@ struct _virHypervisorDriver {
 virDrvDomainSetVcpu domainSetVcpu;
 virDrvDomainSetBlockThreshold domainSetBlockThreshold;
 virDrvDomainSetLifecycleAction domainSetLifecycleAction;
+virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo;
 };
 
 
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 2d86e48979d3..dd5a8712484d 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -12101,3 +12101,51 @@ int virDomainSetLifecycleAction(virDomainPtr domain,
 virDispatchError(domain->conn);
 return -1;
 }
+
+/**
+ * virDomainGetLaunchSecurityInfo:
+ * @domain: a domain object
+ * @params: where to store security info
+ * @nparams: number of items in @params
+ * @flags: currently used, set to 0.
+ *
+ * Get the launch security info. In case of the SEV guest, this will
+ * return the launch measurement.
+ *
+ * Returns -1 in case of failure, 0 in case of success.
+ */
+int virDomainGetLaunchSecurityInfo(virDomainPtr domain,
+   virTypedParameterPtr *params,
+   int *nparams,
+   unsigned int flags)
+{
+virConnectPtr conn = domain->conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p flags=0x%x",
+ params, nparams, flags);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+virCheckNonNullArgGoto(params, error);
+virCheckNonNullArgGoto(nparams, error);
+virCheckReadOnlyGoto(conn->flags, error);
+
+if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+flags |= VIR_TYPED_PARAM_STRING_OKAY;
+
+if (conn->driver->domainGetLaunchSecurityInfo) {
+int ret;
+ret = conn->driver->domainGetLaunchSecurityInfo(domain, params,
+nparams, flags);
+if (ret < 0)
+goto error;
+return ret;
+}
+virReportUnsupportedError();
+
+ error:
+virDispatchError(domain->conn);
+return -1;
+}
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;
+
 #  define new API here using predicted next version number 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list