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

["Dov Murik" <dovmurik@linux.ibm.com>]

Thanks Mike for fixing this.

On 21/12/2022 18: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: Dov Murik <dovmurik@linux.ibm.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
> 
>                    );
> 
>    }
> 
>  
>