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

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

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
>