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
