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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hey
CI failed - https://github.com/tianocore/edk2/pull/3772
Have you run CI before submit patch? Please take a look.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Monday, December 12, 2022 11:01 PM
> To: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum <tobin@ibm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Aktas, Erdem <erdemaktas@google.com>;
> Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan
> L <jordan.l.justen@intel.com>; Michael Roth <michael.roth@amd.com>; Xu,
> Min M <min.m.xu@intel.com>; Tobin Feldman-Fitzthum
> <tobin@linux.ibm.com>
> Subject: Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret
> location as EfiACPIReclaimMemory
> 
> 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
> >                   );
> >   }