Re: [PATCH 1/4] OvmfPkg/AmdSevDxe: Allocate SEV-SNP CC blob as EfiACPIReclaimMemory

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 12/21/22 10:06, Michael Roth wrote:
> The SEV-SNP Confidential Computing blob contains metadata that should
> remain accessible for the life of the guest. Allocate it as
> EfiACPIReclaimMemory to ensure the memory isn't overwritten by the guest
> operating system later.
> 
> Reported-by: Dov Murik <dovmurik@linux.ibm.com>
> Suggested-by: Dov Murik <dovmurik@linux.ibm.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   OvmfPkg/AmdSevDxe/AmdSevDxe.c | 62 +++++++++++++++++++++++++++--------
>   1 file changed, 48 insertions(+), 14 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 662d3c4ccb..8dfda961d7 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -21,15 +21,36 @@
>   #include <Guid/ConfidentialComputingSevSnpBlob.h>
>   #include <Library/PcdLib.h>
>   
> -STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
> -  SIGNATURE_32 ('A',                                    'M', 'D', 'E'),
> -  1,
> -  0,
> -  (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase),
> -  FixedPcdGet32 (PcdOvmfSnpSecretsSize),
> -  (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase),
> -  FixedPcdGet32 (PcdOvmfCpuidSize),
> -};
> +STATIC
> +EFI_STATUS
> +AllocateConfidentialComputingBlob (
> +  OUT CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  **CcBlobPtr
> +  )
> +{
> +  EFI_STATUS                                Status;
> +  CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  *CcBlob;
> +
> +  Status = gBS->AllocatePool (
> +                  EfiACPIReclaimMemory,
> +                  sizeof (CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION),
> +                  (VOID **)&CcBlob
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  CcBlob->Header                 = SIGNATURE_32 ('A', 'M', 'D', 'E');
> +  CcBlob->Version                = 1;
> +  CcBlob->Reserved1              = 0;
> +  CcBlob->SecretsPhysicalAddress = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase);
> +  CcBlob->SecretsSize            = FixedPcdGet32 (PcdOvmfSnpSecretsSize);
> +  CcBlob->CpuidPhysicalAddress   = (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase);
> +  CcBlob->CpuidLSize             = FixedPcdGet32 (PcdOvmfCpuidSize);
> +
> +  *CcBlobPtr = CcBlob;
> +
> +  return EFI_SUCCESS;
> +}
>   
>   EFI_STATUS
>   EFIAPI
> @@ -38,10 +59,11 @@ AmdSevDxeEntryPoint (
>     IN EFI_SYSTEM_TABLE  *SystemTable
>     )
>   {
> -  EFI_STATUS                       Status;
> -  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
> -  UINTN                            NumEntries;
> -  UINTN                            Index;
> +  EFI_STATUS                                Status;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR           *AllDescMap;
> +  UINTN                                     NumEntries;
> +  UINTN                                     Index;
> +  CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  *SnpBootDxeTable;
>   
>     //
>     // Do nothing when SEV is not enabled
> @@ -147,6 +169,18 @@ AmdSevDxeEntryPoint (
>       }
>     }
>   
> +  Status = AllocateConfidentialComputingBlob (&SnpBootDxeTable);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: AllocateConfidentialComputingBlob(): %r\n",
> +      __FUNCTION__,
> +      Status
> +      ));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +
>     //
>     // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB.
>     // It contains the location for both the Secrets and CPUID page.
> @@ -154,7 +188,7 @@ AmdSevDxeEntryPoint (
>     if (MemEncryptSevSnpIsEnabled ()) {
>       return gBS->InstallConfigurationTable (
>                     &gConfidentialComputingSevSnpBlobGuid,
> -                  &mSnpBootDxeTable
> +                  SnpBootDxeTable
>                     );
>     }
>