Re: [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD

["Laszlo Ersek" <lersek@redhat.com>]

On 06/07/21 17:58, Brijesh Singh wrote:
> 
> On 6/7/21 7:26 AM, Laszlo Ersek wrote:
>> On 05/27/21 01:11, Brijesh Singh wrote:
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C32a95d87f0984b88080708d929af878f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586656154129803%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=JyrMLVE%2BMNq%2B1sUTI7WnbxkjloKi81PcISiLvz2geLg%3D&reserved=0
>>>
>>> When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
>>> secrets page.
>> For pure SEV?
> 
> The secrets page is applicable to all the SEV's (SEV, SEV-ES and
> SEV-SNP) but there is some difference see below.
> 
> 
>>
>>> When SEV-SNP is enabled, the secrets page contains the VM platform
>>> communication keys. The guest BIOS and OS can use this key to communicate
>>> with the SEV firmware to get attesation report. See the SEV-SNP firmware
>>> spec for more details for the content of the secrets page.
>>>
>>> When SEV and SEV-ES is enabled, the secrets page contains the information
>>> provided by the guest owner after the attestation. See the SEV
>>> LAUNCH_SECRET command for more details.
>>>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>> ---
>>>  OvmfPkg/OvmfPkgX64.dsc                 |  2 ++
>>>  OvmfPkg/OvmfPkgX64.fdf                 |  5 +++++
>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.inf |  1 +
>>>  OvmfPkg/AmdSev/SecretPei/SecretPei.c   | 15 ++++++++++++++-
>>>  4 files changed, 22 insertions(+), 1 deletion(-)
>> How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc"
>> platform, where remote attestation is not a goal?
>>
>> What you describe makes sense to me, but only for the remote-attested
>> "OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes
>> SecretPei and SecretDxe, and sets the necessary PCDs.)
>>
>> Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei"
>> module, the commit message does not explain sufficiently why the secrets
>> page must be reserved for good. The "SEV-SNP firmware spec" reference is
>> vague at best; I'm permanently lost between the dozen PDF files I have
>> downloaded locally from the AMD website. Please include a specific
>> document number, revision number, and chapter/section identifier.
> 
> 
> There is a fundamental difference between SEV and SEV-SNP attestation
> flow. In the case of SEV and SEV-ES, the attestation happens before the
> VM is booted, and the secrets page contains the data provided by the
> guest owner after the attestation is complete. The hypervisor injects
> that data into the guest memory before booting it.  However, with
> SEV-SNP, the guest uses the data from the secrets page to build a
> message for the PSP. The guest can send the following message to the PSP:
> 
> 1. Expand the filtered CPUID list
> 2. Query attestation report
> 2. Derive a key
> 3. VM export, import, and absorb -- migration specific command
> 
> See chapter 7 [1] for all possible commands that a guest can send to PSP
> through the guest message request. I understand that it is confusing,
> but the secrets page is *not* same as SEV/SEV-ES. But since SEV-SNP spec
> calls it secrets, so I used the same name. 

I thought the secrets page was entirely opaque to the guest firmware;
i.e., all the guest firmware would do with it is (a) cover it with an
allocation in SecretPei, (b) forward it to the guest OS via a UEFI
system config table in SecretDxe.

This patch uses the same PCD names ("launch secret", where I understand
the SEV-SNP case *not* to be a *launch* secret; is that right?), plus it
uses the same drivers. That's way too confusing.


So what is this "SNP secrets" page supposed to contain:

- both the previously defined SEV/SEV-ES level launch secret, and the
SNP-specific VMPCK (?)

- how are these secret bits separated from each other in the page?

- does the guest (firmware and/or OS) *write* to the new locations in
the page, possibly for secure message construction?


Either way, I think the proposed repurposing of the page, for the sake
of SNP secrets (VMPCK and maybe even secure message construction?),
breaks the current declarations of the PCDs, in "OvmfPkg.dec":

  ## The base address and size of the SEV Launch Secret Area provisioned
  #  after remote attestation.  If this is set in the .fdf, the platform
  #  is responsible for protecting the area from DXE phase overwrites.
  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
  gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43

> 
> In SEV-SNP, the secrets page is not tight up with just the remote
> attestation.

This is the most important statement. We need the SNP secrets page even
without remote attestation. OvmfPkgX64.dsc does not deal with remote
attestation.

But then (putting all the PCD naming confusion aside), if a driver is
promoted to "common use", from the AmdSevX64 platform to multiple
OvmfPkg platforms, then it should be lifted to the top-level OvmfPkg
directory.

> Later, the AmdSev.dsc can include a library to perform the
> SEV-SNP-specific attestation. The library can use the SNP secrets page
> to get the key and message counter use for constructing the guest
> message to query the attestation report.
> 
> I hope it clarifies it.
> 
> [1] https://www.amd.com/system/files/TechDocs/56860.pdf
> 
> 
>> Honestly I'm getting a *rushed* vibe on this whole series. Why is that?
> 
> I am not sure why you are getting this feel, please let me know where I
> can help to clarify but the series is *rushed* at all. Its building on
> existing support. It's possible that we are getting mixed with the
> fundamental difference between the SEV and SEV-SNP attestation flow and
> recent patches from Dov to expand the attestation to cover other aspects
> of the boot flow.
> 
> In case of SEV-SNP, some folks may prefer to do all the attestation in
> the OVMF and others may prefer to do the attestation in the guest OS. We
> should try to not restrict one approach over the other.
> 
> 
>>
>> Assume that I'm dumb. You won't be far from the truth. Then hold my hand
>> through all this?
> 
> 
> Please let me know if the above explanation helps or I should expand more.

You should please (a) expand your *commit messages*, (b) add a *wall* of
text in the "OvmfPkg.dec" file, where the PCDs in questions are
declared. When I grep the OvmfPkg subdirectory in two years for
"PcdSevLaunchSecretBase", I'd like to find the DEC file's comments to be
consistent with the actual uses of the PCD, and I'd like git-blame to
tell me something useful about those lines, too.


One problem is that I'm supposed to internalize about 50 pages from yet
from another technical specification, in order to get the basics of a
single patch. I can't even follow the *set* of AMD documents I should
have a local copy of. How am I supposed to interleave all that with, for
example, reviewing a 57 slide TDX design presentation?

Honestly, this has gone off the rails. The pressure that Confidential
Computing has generated for me as an OvmfPkg co-maintainer over the
course of a few months exceeds what I've been under for nearly a
*decade*, including all prior work with SEV and SEV-ES.

This makes me incredibly unhappy.

Laszlo


> 
> 
>> Laszlo
>>
>>
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index 999738dc39cd..ea08e1fabc65 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -716,6 +716,7 @@ [Components]
>>>    OvmfPkg/SmmAccess/SmmAccessPei.inf
>>>  !endif
>>>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>>> +  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>  
>>>  !if $(TPM_ENABLE) == TRUE
>>>    OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>>> @@ -966,6 +967,7 @@ [Components]
>>>    OvmfPkg/PlatformDxe/Platform.inf
>>>    OvmfPkg/AmdSevDxe/AmdSevDxe.inf
>>>    OvmfPkg/IoMmuDxe/IoMmuDxe.inf
>>> +  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>  
>>>  !if $(SMM_REQUIRE) == TRUE
>>>    OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index d6be798fcadd..9126b8eb5014 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -88,6 +88,9 @@ [FD.MEMFD]
>>>  0x00C000|0x001000
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>>  
>>> +0x00D000|0x001000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
>>> +
>>>  0x010000|0x010000
>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>  
>>> @@ -179,6 +182,7 @@ [FV.PEIFV]
>>>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>>>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>>>  !endif
>>> +INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>>  
>>>  ################################################################################
>>>  
>>> @@ -314,6 +318,7 @@ [FV.DXEFV]
>>>  INF  ShellPkg/Application/Shell/Shell.inf
>>>  
>>>  INF MdeModulePkg/Logo/LogoDxe.inf
>>> +INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
>>>  
>>>  #
>>>  # Network modules
>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> index 08be156c4bc0..9265f8adee12 100644
>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>>> @@ -26,6 +26,7 @@ [LibraryClasses]
>>>    HobLib
>>>    PeimEntryPoint
>>>    PcdLib
>>> +  MemEncryptSevLib
>>>  
>>>  [FixedPcd]
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>>> diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> index ad491515dd5d..51eb094555aa 100644
>>> --- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> +++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
>>> @@ -7,6 +7,7 @@
>>>  #include <PiPei.h>
>>>  #include <Library/HobLib.h>
>>>  #include <Library/PcdLib.h>
>>> +#include <Library/MemEncryptSevLib.h>
>>>  
>>>  EFI_STATUS
>>>  EFIAPI
>>> @@ -15,10 +16,22 @@ InitializeSecretPei (
>>>    IN CONST EFI_PEI_SERVICES     **PeiServices
>>>    )
>>>  {
>>> +  UINTN   Type;
>>> +
>>> +  //
>>> +  // The location of the secret page should be marked reserved so that guest OS
>>> +  // does not treated as a system RAM.
>>> +  //
>>> +  if (MemEncryptSevSnpIsEnabled ()) {
>>> +    Type = EfiReservedMemoryType;
>>> +  } else {
>>> +    Type = EfiBootServicesData;
>>> +  }
>>> +
>>>    BuildMemoryAllocationHob (
>>>      PcdGet32 (PcdSevLaunchSecretBase),
>>>      PcdGet32 (PcdSevLaunchSecretSize),
>>> -    EfiBootServicesData
>>> +    Type
>>>      );
>>>  
>>>    return EFI_SUCCESS;
>>>
>