Re: [edk2-devel] [PATCH] OvmfPkg/PlatformPei: Validate SEC's GHCB page

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

In the future, please include the reviewers on cc: that are identified in 
the Maintainers.txt file (use ./BaseTools/Scripts/GetMaintainer.py), I 
almost missed this patch.

On 12/8/22 17:43, Adam Dunlap via groups.io 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 bytes of each page of memory and checks to see if a #VC
> exception is generated indicating that the page was not validated.
> 
> Signed-off-by: Adam Dunlap <acdunlap@google.com>

This should include a Fixes: tag.

> ---
>   OvmfPkg/PlatformPei/AmdSev.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index e1b9fd9b7f..c465732068 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -206,6 +206,7 @@ AmdSevEsInitialize (
>   {
>     UINT8                *GhcbBase;
>     PHYSICAL_ADDRESS     GhcbBasePa;
> +  PHYSICAL_ADDRESS     PrevGhcbPa;
>     UINTN                GhcbPageCount;
>     UINT8                *GhcbBackupBase;
>     UINT8                *GhcbBackupPages;
> @@ -293,8 +294,24 @@ AmdSevEsInitialize (
>       GhcbRegister (GhcbBasePa);
>     }
>   
> +  PrevGhcbPa = AsmReadMsr64 (MSR_SEV_ES_GHCB);

In general, I don't think this is necessary.  There is only one path into 
this function and no matter what, the SEC GHCB address must be used.

> +
>     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.
> +  //
> +  ASSERT (PrevGhcbPa == FixedPcdGet32(PcdOvmfSecGhcbBase));

Again, I don't think this is necessary.

> +
> +  ASSERT_RETURN_ERROR(MemEncryptSevSetPageEncMask(

I find this hard to read. Please be consistent with what is currently done 
in the file/function and rename the DecryptStatus variable to just Status 
and do:

   Status = MemEncryptSevSetPageEncMask (
              0,
              ...
   ASSERT_RETURN_ERROR (Status);

(And notice the space between the function name and the opening "(")

> +    0 /*Cr3 -- use system Cr3*/,

Coding standards should have the comma after the 0 and the comment aligned 
out to the longest parameter and using //.  See other examples in the file.

> +    PrevGhcbPa,

Please use FixedPcdGet32(PcdOvmfSecGhcbBase) here.

> +    1 /*Number of pages*/));

The closing ");" must be on a line of its own.

Thanks,
Tom

> +
>     //
>     // 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