[RFC PATCH v1 0/5] Add virDomainGetSevAttestationReport API

[Cole Robinson]

On 4/14/22 2:46 PM, Tyler Fanelli wrote:
> On 4/11/22 10:57 AM, Cole Robinson wrote:
>> On 3/23/22 3:36 PM, Tyler Fanelli wrote:
>>> This an RFC discussing a new API, virDomainGetSevAttestationReport
>>> (along with a
>>> virsh command "domgetsevreport"), with initial QEMU support via the
>>> "query-sev-attestation-report" QAPI mechanism.
>>> "query-sev-attestation-report" is
>>> supplied a base64-encoded 16 byte "mnonce" string as input, with a
>>> purpose of
>>> being embedded into the attestation report to provide protection.
>>>
>>> My main point of concern is the design/communication of the
>>> virTypedParameterPtr
>>> exchanged between the client and libvirtd and how they interact
>>> together, as I
>>> have seen no other API follow the method I used. Namely, the same
>>> virTypedParameterPtr is used for both input _AND_ output. The same
>>> virTypedParameterPtr containing the original mnonce string inputted
>>> to the API is
>>> also used to contain the attestation report upon being returned from
>>> the API.
>>>
>>> This contrasts with much of the APIs I've noticed, which use a
>>> virTypedParameterPtr for either input or output, but not both.
>>>
>>> This patch is not final, as I still would like some human-readable
>>> outputting
>>> and storage of the attestation report.
>>>
>>> Looking for thoughts on the design of this API, as well as suggested
>>> improvements.
>> The proposed API name contains Sev, when all the existing domain APIs
>> have generic names: virDomainGetLaunchSecurityInfo,
>> virDomainSetLaunchSecurityState.
>>
>> I was thinking it would be nice to avoid that Sev specific bit. So I
>> looked at upcoming SNP and TDX qemu patches to see if they add any qmp
>> commands that take input and return a lot of output. Then maybe it would
>> make sense to name this something like
>> virDomainGetLaunchSecurityInfoWithParams  which we could use more in the
>> future.
>>
>> qemu SNP patches: https://github.com/AMDESE/qemu/tree/snp-v3
>> - Only extend existing query-sev and query-sev-capabilities
>>
>> qemu TDX patches: https://github.com/intel/qemu-tdx
>> - Adds query-tdx and query-tdx-capabilities, both with no input
>>
>> So, that doesn't help.
> 
> What about adding the attestation report to
> virDomainGetLaunchSecurityInfo? If that route is taken, there would
> probably need to be some extra logic added to decipher getting launch
> security info on SEV vs. SGX/TDX, right?
> 

The problem is virDomainGetLaunchSecurityInfo doesn't have any way to
pass in the nonce, so we can't reuse that API as is.

>>
>>
>> After that, my question is, what query-sev-attestation-report adds. The
>> kernel commit explains what it is:
>> https://github.com/torvalds/linux/commit/2c07ded06427dd3339278487a1413d5e478f05f9
>>
>>
>>>      The SEV FW version >= 0.23 added a new command that can be used
>>> to query
>>>      the attestation report containing the SHA-256 digest of the
>>> guest memory
>>>      encrypted through the KVM_SEV_LAUNCH_UPDATE_{DATA, VMSA}
>>> commands and
>>>      sign the report with the Platform Endorsement Key (PEK).
>>>           See the SEV FW API spec section 6.8 for more details.
>>>           Note there already exist a command (KVM_SEV_LAUNCH_MEASURE)
>>> that can be
>>>      used to get the SHA-256 digest. The main difference between the
>>>      KVM_SEV_LAUNCH_MEASURE and KVM_SEV_ATTESTATION_REPORT is that
>>> the latter
>>>      can be called while the guest is running and the measurement
>>> value is
>>>      signed with PEK.
>> So it is LAUNCH_MEASURE, available at VM runtime, signed with an extra
>> key.
>>

And with a nonce passed in, I forgot to mention that. That's another bit
I'm not sure what it adds over the traditional measurement

>> qemu caches the LAUNCH_MEASURE value at VM startup and lets us query it
>> with qmp any time, so I don't think runtime access matters.
> 
> I'm surprised by this. I was under the impression that LAUNCH_MEASURE
> always had to be called when a VM is paused.
> 
>>
>> Maybe the extra key signing is a security fix or something. I haven't
>> figured it out.
> 
> Signing with the PEK also allows a user to verify the root of trust
> between the owner and the platform.
> 

But I don't understand what that means in practice. I figured key
management via the session blob already took care of this, but I haven't
tried to wrap my head around the details.

>>
>> But as is it's not clear what this buys us over the launch measurement
>> we already report with virDomainGetLaunchSecurityInfo
>>
>>
>> If we figure out what the point of this is, IMO we can more easily
>> reason about whether it makes sense to add a Sev specific libvirt API,
>> and whether we need virTypedParams for both input and output. For
>> example if the API really is specific to this one and only KVM ioctl/QMP
>> command, we could hardcode the parameters and skip the virTypedParams
>> question entirely.
> 
> Interesting, although wouldn't hardcoding an nonce basically render it
> useless? User-specified nonce would allow a user to verify that their
> call was propagated to firmware at that instance. If they can't supply
> the nonce, they can't verify it's an attestation report from that
> specific call.
> 

Sorry if I was unclear, I didn't mean hardcoding a specific nonce value.
I meant that, if we decide we there's not much value in making this API
generic, then we can strip out the virTypedParams usage and make the
signature closer to:

int
virDomainGetSevAttestationReport(virDomainPtr domain,
                                 const char *mnonce,
                                 char *report);

Thanks,
Cole