Re: [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page

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

Merged https://github.com/tianocore/edk2/pull/3774

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Tuesday, December 13, 2022 3:30 AM
> To: Adam Dunlap <acdunlap@google.com>; devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen
> <jiewen.yao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Dionna Glaze
> <dionnaglaze@google.com>
> Subject: Re: [PATCH v3] OvmfPkg/PlatformPei: Validate SEC's GHCB page
> 
> On 12/9/22 15:04, Adam Dunlap wrote:
> > When running under SEV-ES, a page of shared memory is allocated for the
> > GHCB during the SEC phase at address 0x809000. This page of memory is
> > eventually passed to the OS as EfiConventionalMemory. When running
> > SEV-SNP, this page is not PVALIDATE'd in the RMP table, meaning that if
> > the guest OS tries to access the page, it will think that the host has
> > voilated the security guarantees and will likely crash.
> >
> > This patch validates this page immediately after EDK2 switches to using
> > the GHCB page allocated for the PEI phase.
> >
> > This was tested by writing a UEFI application that reads to and writes
> > from one byte of each page of memory and checks to see if a #VC
> > exception is generated indicating that the page was not validated.
> >
> > Fixes: 6995a1b79bab ("OvmfPkg: Create a GHCB page for use during Sec
> phase")
> >
> > Signed-off-by: Adam Dunlap <acdunlap@google.com>
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> > ---
> >
> > Removed the PcdStatus variable and just use Status for all statuses in
> > this function. Use uncrustify to fix some formatting errors.
> >
> >   OvmfPkg/PlatformPei/AmdSev.c | 40 ++++++++++++++++++++++++----------
> --
> >   1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/OvmfPkg/PlatformPei/AmdSev.c
> b/OvmfPkg/PlatformPei/AmdSev.c
> > index e1b9fd9b7f..b2f2f3ac26 100644
> > --- a/OvmfPkg/PlatformPei/AmdSev.c
> > +++ b/OvmfPkg/PlatformPei/AmdSev.c
> > @@ -212,7 +212,7 @@ AmdSevEsInitialize (
> >     UINTN                GhcbBackupPageCount;
> >     SEV_ES_PER_CPU_DATA  *SevEsData;
> >     UINTN                PageCount;
> > -  RETURN_STATUS        PcdStatus, DecryptStatus;
> > +  RETURN_STATUS        Status;
> >     IA32_DESCRIPTOR      Gdtr;
> >     VOID                 *Gdt;
> >
> > @@ -220,8 +220,8 @@ AmdSevEsInitialize (
> >       return;
> >     }
> >
> > -  PcdStatus = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> > -  ASSERT_RETURN_ERROR (PcdStatus);
> > +  Status = PcdSetBoolS (PcdSevEsIsEnabled, TRUE);
> > +  ASSERT_RETURN_ERROR (Status);
> >
> >     //
> >     // Allocate GHCB and per-CPU variable pages.
> > @@ -240,20 +240,20 @@ AmdSevEsInitialize (
> >     // only clear the encryption mask for the GHCB pages.
> >     //
> >     for (PageCount = 0; PageCount < GhcbPageCount; PageCount += 2) {
> > -    DecryptStatus = MemEncryptSevClearPageEncMask (
> > -                      0,
> > -                      GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> > -                      1
> > -                      );
> > -    ASSERT_RETURN_ERROR (DecryptStatus);
> > +    Status = MemEncryptSevClearPageEncMask (
> > +               0,
> > +               GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> > +               1
> > +               );
> > +    ASSERT_RETURN_ERROR (Status);
> >     }
> >
> >     ZeroMem (GhcbBase, EFI_PAGES_TO_SIZE (GhcbPageCount));
> >
> > -  PcdStatus = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> > -  ASSERT_RETURN_ERROR (PcdStatus);
> > -  PcdStatus = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE
> (GhcbPageCount));
> > -  ASSERT_RETURN_ERROR (PcdStatus);
> > +  Status = PcdSet64S (PcdGhcbBase, GhcbBasePa);
> > +  ASSERT_RETURN_ERROR (Status);
> > +  Status = PcdSet64S (PcdGhcbSize, EFI_PAGES_TO_SIZE (GhcbPageCount));
> > +  ASSERT_RETURN_ERROR (Status);
> >
> >     DEBUG ((
> >       DEBUG_INFO,
> > @@ -295,6 +295,20 @@ AmdSevEsInitialize (
> >
> >     AsmWriteMsr64 (MSR_SEV_ES_GHCB, GhcbBasePa);
> >
> > +  //
> > +  // Now that the PEI GHCB is set up, the SEC GHCB page is no longer
> necessary
> > +  // to keep shared. Later, it is exposed to the OS as
> EfiConventionalMemory, so
> > +  // it needs to be marked private. The size of the region is hardcoded in
> > +  // OvmfPkg/ResetVector/ResetVector.nasmb in the definition of
> > +  // SNP_SEC_MEM_BASE_DESC_2.
> > +  //
> > +  Status = MemEncryptSevSetPageEncMask (
> > +             0,                                  // Cr3 -- use system Cr3
> > +             FixedPcdGet32 (PcdOvmfSecGhcbBase), // BaseAddress
> > +             1                                   // NumPages
> > +             );
> > +  ASSERT_RETURN_ERROR (Status);
> > +
> >     //
> >     // The SEV support will clear the C-bit from non-RAM areas.  The early
> GDT
> >     // lives in a non-RAM area, so when an exception occurs (like a #VC) the
> GDT