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

["Brijesh Singh" <brijesh.singh@amd.com>]

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. 

In SEV-SNP, the secrets page is not tight up with just the remote
attestation. 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.


> 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;
>>