Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory

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

On 12/12/22 02:08, Dov Murik wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
> 
> Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret
> area as reserved") marked the launch secret area itself (1 page) as
> reserved so the guest OS can use it during the lifetime of the OS.
> However, the address and size of the secret area held in the
> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as STATIC in
> OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
> not be written over by OS data.
> 
> Fix this by allocating the memory for the
> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
> EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
> this memory.
> 
> Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved")
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <Jiewen.Yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

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

> 
> ---
> 
> v2 changes:
> * Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
> ---
>   OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> index 3d84b2545052..4f65b1ce5ba5 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -8,11 +8,6 @@
>   #include <Library/UefiBootServicesTableLib.h>
>   #include <Guid/ConfidentialComputingSecret.h>
>   
> -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION  mSecretDxeTable = {
> -  FixedPcdGet32 (PcdSevLaunchSecretBase),
> -  FixedPcdGet32 (PcdSevLaunchSecretSize),
> -};
> -
>   EFI_STATUS
>   EFIAPI
>   InitializeSecretDxe (
> @@ -20,8 +15,23 @@ InitializeSecretDxe (
>     IN EFI_SYSTEM_TABLE  *SystemTable
>     )
>   {
> +  EFI_STATUS                             Status;
> +  CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
> +
> +  Status = gBS->AllocatePool (
> +                  EfiACPIReclaimMemory,
> +                  sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
> +                  (VOID **)&SecretDxeTable
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
> +  SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
> +
>     return gBS->InstallConfigurationTable (
>                   &gConfidentialComputingSecretGuid,
> -                &mSecretDxeTable
> +                SecretDxeTable
>                   );
>   }