Re: [PATCH] docs: Add measurement calculation details to amd-memory-encryption.txt

[Daniel P . Berrangé]

On Thu, Dec 16, 2021 at 11:41:27PM +0200, Dov Murik wrote:
> 
> 
> On 16/12/2021 18:09, Daniel P. Berrangé wrote:
> > On Thu, Dec 16, 2021 at 12:38:34PM +0200, Dov Murik wrote:
> >>
> >>
> >> On 14/12/2021 20:39, Daniel P. Berrangé wrote:
> >>> On Tue, Dec 14, 2021 at 01:59:10PM +0000, Dov Murik wrote:
> >>>> Add a section explaining how the Guest Owner should calculate the
> >>>> expected guest launch measurement for SEV and SEV-ES.
> >>>>
> >>>> Also update the name and link to the SEV API Spec document.
> >>>>
> >>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> >>>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> >>>> ---
> >>>>  docs/amd-memory-encryption.txt | 50 +++++++++++++++++++++++++++++++---
> >>>>  1 file changed, 46 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/docs/amd-memory-encryption.txt 
> >>>> b/docs/amd-memory-encryption.txt
> >>>> index ffca382b5f..f97727482f 100644
> >>>> --- a/docs/amd-memory-encryption.txt
> >>>> +++ b/docs/amd-memory-encryption.txt
> >>>> @@ -43,7 +43,7 @@ The guest policy is passed as plaintext. A hypervisor 
> >>>> may choose to read it,
> >>>>  but should not modify it (any modification of the policy bits will 
> >>>> result
> >>>>  in bad measurement). The guest policy is a 4-byte data structure 
> >>>> containing
> >>>>  several flags that restricts what can be done on a running SEV guest.
> >>>> -See KM Spec section 3 and 6.2 for more details.
> >>>> +See SEV API Spec [1] section 3 and 6.2 for more details.
> >>>>  
> >>>>  The guest policy can be provided via the 'policy' property (see below)
> >>>>  
> >>>> @@ -88,7 +88,7 @@ expects.
> >>>>  LAUNCH_FINISH finalizes the guest launch and destroys the cryptographic
> >>>>  context.
> >>>>  
> >>>> -See SEV KM API Spec [1] 'Launching a guest' usage flow (Appendix A) for 
> >>>> the
> >>>> +See SEV API Spec [1] 'Launching a guest' usage flow (Appendix A) for the
> >>>>  complete flow chart.
> >>>>  
> >>>>  To launch a SEV guest
> >>>> @@ -113,6 +113,45 @@ a SEV-ES guest:
> >>>>   - Requires in-kernel irqchip - the burden is placed on the hypervisor 
> >>>> to
> >>>>     manage booting APs.
> >>>>  
> >>>> +Calculating expected guest launch measurement
> >>>> +---------------------------------------------
> >>>> +In order to verify the guest launch measurement, The Guest Owner must 
> >>>> compute
> >>>> +it in the exact same way as it is calculated by the AMD-SP.  SEV API 
> >>>> Spec [1]
> >>>> +section 6.5.1 describes the AMD-SP operations:
> >>>> +
> >>>> +    GCTX.LD is finalized, producing the hash digest of all plaintext 
> >>>> data
> >>>> +    imported into the guest.
> >>>> +
> >>>> +    The launch measurement is calculated as:
> >>>> +
> >>>> +    HMAC(0x04 || API_MAJOR || API_MINOR || BUILD || GCTX.POLICY || 
> >>>> GCTX.LD || MNONCE; GCTX.TIK)
> >>>> +
> >>>> +    where "||" represents concatenation.
> >>>> +
> >>>> +The values of API_MAJOR, API_MINOR, BUILD, and GCTX.POLICY can be 
> >>>> obtained
> >>>> +from the 'query-sev' qmp command.
> >>>> +
> >>>> +The value of MNONCE is part of the response of 
> >>>> 'query-sev-launch-measure': it
> >>>> +is the last 16 bytes of the base64-decoded data field (see SEV API Spec 
> >>>> [1]
> >>>> +section 6.5.2 Table 52: LAUNCH_MEASURE Measurement Buffer).
> >>>> +
> >>>> +The value of GCTX.LD is SHA256(firmware_blob || kernel_hashes_blob || 
> >>>> vmsas_blob),
> >>>> +where:
> >>>> +
> >>>> +* firmware_blob is the content of the entire firmware flash file (for 
> >>>> example,
> >>>> +  OVMF.fd).
> >>>
> >>> Lets add a caveat that the firmware flash should be built to be stateless
> >>> ie that it is not secure to attempt to measure a guest where the firmware
> >>> uses an NVRAM store.
> >>>
> >>
> >> * firmware_blob is the content of the entire firmware flash file (for   
> >>   example, OVMF.fd).  Note that you must build a stateless firmware file   
> >>  
> >>   which doesn't use an NVRAM store, because the NVRAM area is not
> >>   measured, and therefore it is not secure to use a firmware which uses 
> >>   state from an NVRAM store.
> > 
> > Looks good to me.
> > 
> >>>> +* if kernel is used, and kernel-hashes=on, then kernel_hashes_blob is 
> >>>> the
> >>>> +  content of PaddedSevHashTable (including the zero padding), which 
> >>>> itself
> >>>> +  includes the hashes of kernel, initrd, and cmdline that are passed to 
> >>>> the
> >>>> +  guest.  The PaddedSevHashTable struct is defined in target/i386/sev.c 
> >>>> .
> >>>> +* if SEV-ES is enabled (policy & 0x4 != 0), vmsas_blob is the 
> >>>> concatenation of
> >>>> +  all VMSAs of the guest vcpus.  Each VMSA is 4096 bytes long; its 
> >>>> content is
> >>>> +  defined inside Linux kernel code as struct vmcb_save_area, or in AMD 
> >>>> APM
> >>>> +  Volume 2 [2] Table B-2: VMCB Layout, State Save Area.
> >>>
> >>> Is there any practical guidance we can give apps on the way the VMSAs
> >>> can be expected to be initialized ? eg can they assume essentially
> >>> all fields in vmcb_save_area are 0 initialized except for certain
> >>> ones ? Is initialization likely to vary at all across KVM or EDK2
> >>> vesions or something ?
> >>
> >> From my own experience, the VMSA of vcpu0 doesn't change; it is basically 
> >> what QEMU
> >> sets up in x86_cpu_reset() (which is mostly zeros but not all).  I don't 
> >> know if it
> >> may change in newer QEMU (machine types?) or kvm.  As for vcpu1+, in 
> >> SEV-ES the
> >> CS:EIP for the APs is taken from a GUIDed table at the end of the OVMF 
> >> image, and has
> >> actually changed a few months ago when the memory layout changed to 
> >> support both TDX
> >> and SEV.
> > 
> > That is an unplesantly large number of moving parts that could
> > potentially impact the expected state :-(  I think we need to
> > be careful to avoid gratuitous changes, to avoid creating a
> > combinatorial expansion in the number of possibly valid VMSA
> > blocks.
> > 
> > It makes me wonder if we need to think about defining some
> > standard approach for distro vendors (and/or cloud vendors)
> > to publish the expected contents for various combinations
> > of their software pieces.
> > 
> >>
> >>
> >> Here are the VMSAs for my 2-vcpu SEV-ES VM:
> >>
> >>
> >> $ hd vmsa/vmsa_cpu0.bin
> > 
> > ...snipp...
> > 
> > was there a nice approach / tool you used to capture
> > this initial state ?
> > 
> 
> I wouldn't qualify this as nice: I ended up modifying my
> host kernel's kvm (see patch below).  Later I wrote a
> script to parse that hex dump from the kernel log into
> proper 4096-byte binary VMSA files.

Heh, that's basically the same as Sergio Lopez told me he did
for libkrun.

He suggested that it might be desirable to expose this info
in sysfs. Perhaps a entry for debugfs from KVM for each
VM to export the initial state.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7fbce342eec4..4e45fe37b93d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -624,6 +624,12 @@ static int sev_launch_update_vmsa(struct kvm *kvm, 
> struct kvm_sev_cmd *argp)
>                  */
>                 clflush_cache_range(svm->vmsa, PAGE_SIZE);
> 
> +                /* dubek */
> +                pr_info("DEBUG_VMSA - cpu %d START ---------------\n", i);
> +                print_hex_dump(KERN_INFO, "DEBUG_VMSA", DUMP_PREFIX_OFFSET, 
> 16, 1, svm->vmsa, PAGE_SIZE, true);
> +                pr_info("DEBUG_VMSA - cpu %d END ---------------\n", i);
> +                /* ----- */
> +
>                 vmsa.handle = sev->handle;
>                 vmsa.address = __sme_pa(svm->vmsa);
>                 vmsa.len = PAGE_SIZE;
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|