Re: [RFC PATCH 12/19] OvmfPkg/MemEncryptSevLib: Add support to validate system RAM

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

Hi
Would you please clarify why the IA32 version implementation is empty?

Does it mean IA32 does not need validate?
Or IA32 should never call this function?

Anyway, I recommend to add some comment to describe it clearly.
If it should never be called, I recommend to add ASSERT(FALSE).


+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
@@ -0,0 +1,17 @@
+#include <Uefi/UefiBaseType.h>
+
+#include "../SnpPageStateChange.h"
+
+/**
+ The function is used to set the page state when SEV-SNP is active. The page state
+ transition consist of changing the page ownership in the RMP table, and using the
+ PVALIDATE instruction to update the Validated bit in RMP table.
+
+ */
+VOID
+SevSnpValidateSystemRamInternal (
+  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
+  IN UINTN                            NumPages
+  )
+{
+}

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Wednesday, March 24, 2021 11:32 PM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: [RFC PATCH 12/19] OvmfPkg/MemEncryptSevLib: Add support to
> validate system RAM
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Many of the integrity guarantees of SEV-SNP are enforced through the
> Reverse Map Table (RMP). Each RMP entry contains the GPA at which a
> particular page of DRAM should be mapped. The guest can request the
> hypervisor to add pages in the RMP table via the Page State Change VMGEXIT
> defined in the GHCB specification section 2.5.1 and 4.1.6. Inside each RMP
> entry is a Validated flag; this flag is automatically cleared to 0 by the
> CPU hardware when a new RMP entry is created for a guest. Each VM page
> can be either validated or invalidated, as indicated by the Validated
> flag in the RMP entry. Memory access to a private page that is not
> validated generates a #VC. A VM can use the PVALIDATE instruction to
> validate the private page before using it.
> 
> During the guest creation, the boot ROM memory is pre-validated by the
> AMD-SEV firmware. The MemEncryptSevSnpValidateSystemRam() can be called
> during the SEC and PEI phase to validate the detected system RAM.
> 
> One of the fields in the Page State Change NAE is the RMP page size. The
> page size input parameter indicates that either a 4KB or 2MB page should
> be used while adding the RMP entry. During the validation, when possible,
> the MemEncryptSevSnpValidateSystemRam() will use the 2MB entry. A
> hypervisor backing the memory may choose to use the different page size
> in the RMP entry. In those cases, the PVALIDATE instruction should return
> SIZEMISMATCH. If a SIZEMISMATCH is detected, then validate all 512-pages
> constituting a 2MB region.
> 
> Upon completion, the PVALIDATE instruction sets the rFLAGS.CF to 0 if
> instruction changed the RMP entry and to 1 if the instruction did not
> change the RMP entry. The rFlags.CF will be 1 only when a memory region
> is already validated. We should not double validate a memory
> as it could lead to a security compromise. If double validation is
> detected, terminate the boot.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Include/Library/MemEncryptSevLib.h                            |  15 ++
>  OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c        |  17
> ++
>  OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf          |   4 +
>  OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c    |
> 20 ++
>  OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h             |  37
> +++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c    |
> 23 ++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c |
> 254 ++++++++++++++++++++
>  7 files changed, 370 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 03d9eda392..47d6802b61 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -215,4 +215,19 @@ MemEncryptSevGetAddressRangeState (
>    IN UINTN                    Length
>    );
> 
> +/**
> +  If SEV-SNP is active then set the page state of the specified virtual
> +  address range. This should be called in SEC and PEI phases only.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base
> address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSevSnpValidateSystemRam (
> +  IN PHYSICAL_ADDRESS                   BaseAddress,
> +  IN UINTN                              NumPages
> +  );
> +
>  #endif // _MEM_ENCRYPT_SEV_LIB_H_
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
> new file mode 100644
> index 0000000000..dace5c0bcf
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/SnpPageStateChange.c
> @@ -0,0 +1,17 @@
> +#include <Uefi/UefiBaseType.h>
> +
> +#include "../SnpPageStateChange.h"
> +
> +/**
> + The function is used to set the page state when SEV-SNP is active. The page
> state
> + transition consist of changing the page ownership in the RMP table, and using
> the
> + PVALIDATE instruction to update the Validated bit in RMP table.
> +
> + */
> +VOID
> +SevSnpValidateSystemRamInternal (
> +  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
> +  IN UINTN                            NumPages
> +  )
> +{
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
> b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
> index 279c38bfbc..8595e244c2 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
> @@ -31,15 +31,19 @@
> 
>  [Sources]
>    SecMemEncryptSevLibInternal.c
> +  SnpPageStateChange.h
> 
>  [Sources.X64]
>    X64/MemEncryptSevLib.c
>    X64/SecVirtualMemory.c
> +  X64/SecSnpSystemRamValidate.c
> +  X64/SnpPageStateChangeInternal.c
>    X64/VirtualMemory.c
>    X64/VirtualMemory.h
> 
>  [Sources.IA32]
>    Ia32/MemEncryptSevLib.c
> +  Ia32/SnpPageStateChange.c
> 
>  [LibraryClasses]
>    BaseLib
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> index 69852779e2..35a222e75e 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> +++
> b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> @@ -17,6 +17,8 @@
>  #include <Register/Cpuid.h>
>  #include <Uefi/UefiBaseType.h>
> 
> +#include "SnpPageStateChange.h"
> +
>  /**
>    Reads and sets the status of SEV features.
> 
> @@ -172,3 +174,21 @@
> MemEncryptSevLocateInitialSmramSaveStateMapPages (
>  {
>    return RETURN_UNSUPPORTED;
>  }
> +
> +/**
> +  If SEV-SNP is active then set the page state of the specified virtual
> +  address range. This should be called in SEC and PEI phases only.
> +
> +  @param[in]  BaseAddress             Base address
> +  @param[in]  NumPages                Number of pages starting from the base
> address
> +
> +**/
> +VOID
> +EFIAPI
> +MemEncryptSevSnpValidateSystemRam (
> +  IN PHYSICAL_ADDRESS                   BaseAddress,
> +  IN UINTN                              NumPages
> +  )
> +{
> +  SevSnpValidateSystemRam (BaseAddress, NumPages);
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h
> b/OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h
> new file mode 100644
> index 0000000000..3040930999
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SnpPageStateChange.h
> @@ -0,0 +1,37 @@
> +/** @file
> +
> +  SEV-SNP Page Validation functions.
> +
> +  Copyright (c) 2020 - 2021, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef SNP_PAGE_STATE_INTERNAL_H_
> +#define SNP_PAGE_STATE_INTERNAL_H_
> +
> +//
> +// SEV-SNP Page states
> +//
> +typedef enum {
> +  SevSnpPagePrivate,
> +  SevSnpPageShared,
> +
> +} SEV_SNP_PAGE_STATE;
> +
> +VOID
> +SevSnpValidateSystemRam (
> +  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
> +  IN UINTN                            NumPages
> +  );
> +
> +VOID
> +SetPageStateInternal (
> +  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
> +  IN UINTN                            NumPages,
> +  IN SEV_SNP_PAGE_STATE               State,
> +  IN BOOLEAN                          UseLargeEntry
> +  );
> +
> +#endif
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> new file mode 100644
> index 0000000000..915706aad0
> --- /dev/null
> +++
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecSnpSystemRamValidate.c
> @@ -0,0 +1,23 @@
> +/** @file
> +
> +  SEV-SNP Page Validation functions.
> +
> +  Copyright (c) 2020 - 2021, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Library/BaseLib.h>
> +
> +#include "../SnpPageStateChange.h"
> +
> +VOID
> +SevSnpValidateSystemRam (
> +  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
> +  IN UINTN                            NumPages
> +  )
> +{
> +  SetPageStateInternal (BaseAddress, NumPages, SevSnpPagePrivate, TRUE);
> +}
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
> new file mode 100644
> index 0000000000..5a34db33fe
> --- /dev/null
> +++
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpPageStateChangeInternal.c
> @@ -0,0 +1,254 @@
> +/** @file
> +
> +  SEV-SNP Page Validation functions.
> +
> +  Copyright (c) 2020 - 2021, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/VmgExitLib.h>
> +
> +#include <Register/Amd/Ghcb.h>
> +#include <Register/Amd/Msr.h>
> +
> +#include "../SnpPageStateChange.h"
> +
> +#define IS_ALIGNED(x, y)        ((((x) & (y - 1)) == 0))
> +#define PAGES_PER_LARGE_ENTRY   512
> +#define EFI_LARGE_PAGE          (EFI_PAGE_SIZE * PAGES_PER_LARGE_ENTRY)
> +
> +STATIC
> +UINTN
> +MemoryStateToGhcbOp (
> +  IN SEV_SNP_PAGE_STATE   State
> +  )
> +{
> +  UINTN Cmd;
> +
> +  switch (State) {
> +    case SevSnpPageShared: Cmd = SNP_PAGE_STATE_SHARED; break;
> +    case SevSnpPagePrivate: Cmd = SNP_PAGE_STATE_PRIVATE; break;
> +    default: ASSERT(0);
> +  }
> +
> +  return Cmd;
> +}
> +
> +STATIC
> +VOID
> +SnpPageStateFailureTerminate (
> +  VOID
> +  )
> +{
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +
> +  //
> +  // Use the GHCB MSR Protocol to request termination by the hypervisor
> +  //
> +  Msr.GhcbPhysicalAddress = 0;
> +  Msr.GhcbTerminate.Function = GHCB_INFO_TERMINATE_REQUEST;
> +  Msr.GhcbTerminate.ReasonCodeSet = GHCB_TERMINATE_GHCB;
> +  Msr.GhcbTerminate.ReasonCode = GHCB_TERMINATE_GHCB_GENERAL;
> +  AsmWriteMsr64 (MSR_SEV_ES_GHCB, Msr.GhcbPhysicalAddress);
> +
> +  AsmVmgExit ();
> +
> +  ASSERT (FALSE);
> +  CpuDeadLoop ();
> +}
> +
> +STATIC
> +UINTN
> +IssuePvalidate (
> +  IN    UINTN       Address,
> +  IN    UINTN       RmpPageSize,
> +  IN    BOOLEAN     Validate
> +  )
> +{
> +  IA32_EFLAGS32         EFlags;
> +  UINTN                 Ret;
> +
> +  Ret = AsmPvalidate (RmpPageSize, Validate, Address, &EFlags);
> +
> +  //
> +  // Check the rFlags.CF to verify that PVALIDATE updated the RMP
> +  // entry. If there was a no change in the RMP entry then we are
> +  // either double validating or invalidating the memory. This can
> +  // lead to a security compromise.
> +  //
> +  if (EFlags.Bits.CF) {
> +    DEBUG ((DEBUG_ERROR, "%a:%a: Double %a detected for address 0x%Lx\n",
> +           gEfiCallerBaseName,
> +           __FUNCTION__,
> +           Validate ? "Validate" : "Invalidate",
> +           Address));
> +    SnpPageStateFailureTerminate ();
> +  }
> +
> +  return Ret;
> +}
> +
> +/**
> + This function issues the PVALIDATE instruction to validate or invalidate the
> memory
> + range specified. If PVALIDATE returns size mismatch then it tries validating
> with
> + smaller page size.
> +
> + */
> +STATIC
> +VOID
> +PvalidateRange (
> +  IN  SNP_PAGE_STATE_CHANGE_INFO    *Info,
> +  IN  UINTN                         StartIndex,
> +  IN  UINTN                         EndIndex,
> +  IN  BOOLEAN                       Validate
> +  )
> +{
> +  UINTN         Address, RmpPageSize, Ret, i;
> +
> +  for (; StartIndex < EndIndex; StartIndex++) {
> +    Address = Info->Entry[StartIndex].GuestFrameNumber << EFI_PAGE_SHIFT;
> +    RmpPageSize = Info->Entry[StartIndex].PageSize;
> +
> +    Ret = IssuePvalidate (Address, RmpPageSize, Validate);
> +
> +    //
> +    // If we fail to validate due to size mismatch then try with the
> +    // smaller page size. This senario will occur if the backing page in
> +    // the RMP entry is 4K and we are validating it as a 2MB.
> +    //
> +    if ((Ret == PVALIDATE_RET_FAIL_SIZEMISMATCH) &&
> +        (RmpPageSize == PVALIDATE_PAGE_SIZE_2M)) {
> +      for (i = 0; i < PAGES_PER_LARGE_ENTRY; i++) {
> +
> +        Ret = IssuePvalidate (Address, PVALIDATE_PAGE_SIZE_4K, Validate);
> +        if (Ret) {
> +          break;
> +        }
> +
> +        Address = Address + EFI_PAGE_SIZE;
> +      }
> +    }
> +
> +    if (Ret) {
> +    DEBUG ((DEBUG_ERROR, "%a:%a: Failed to %a address 0x%Lx Error
> code %d\n",
> +           gEfiCallerBaseName,
> +           __FUNCTION__,
> +           Validate ? "Validate" : "Invalidate",
> +           Address,
> +           Ret));
> +      SnpPageStateFailureTerminate ();
> +    }
> +  }
> +}
> +
> +/**
> + The function is used to set the page state when SEV-SNP is active. The page
> state
> + transition consist of changing the page ownership in the RMP table, and using
> the
> + PVALIDATE instruction to update the Validated bit in RMP table.
> +
> + When the UseLargeEntry is set to TRUE, then use the large RMP entry.
> + */
> +VOID
> +SetPageStateInternal (
> +  IN EFI_PHYSICAL_ADDRESS             BaseAddress,
> +  IN UINTN                            NumPages,
> +  IN SEV_SNP_PAGE_STATE               State,
> +  IN BOOLEAN                          UseLargeEntry
> +  )
> +{
> +  EFI_STATUS                      Status;
> +  GHCB                            *Ghcb;
> +  EFI_PHYSICAL_ADDRESS            NextAddress, EndAddress;
> +  MSR_SEV_ES_GHCB_REGISTER        Msr;
> +  BOOLEAN                         InterruptState;
> +  SNP_PAGE_STATE_CHANGE_INFO      *Info;
> +  UINTN                           i, RmpPageSize;
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +  Ghcb = Msr.Ghcb;
> +
> +  EndAddress = BaseAddress + EFI_PAGES_TO_SIZE (NumPages);
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a:%a Address 0x%Lx - 0x%Lx State = %a
> LargeEntry = %d\n",
> +          gEfiCallerBaseName,
> +          __FUNCTION__,
> +          BaseAddress,
> +          EndAddress,
> +          State == SevSnpPageShared ? "Shared" : "Private",
> +          UseLargeEntry));
> +
> +  for (; BaseAddress < EndAddress; BaseAddress = NextAddress) {
> +
> +    //
> +    // Initialize the GHCB and setup scratch sw to point to shared buffer.
> +    //
> +    VmgInit (Ghcb, &InterruptState);
> +    Info = (SNP_PAGE_STATE_CHANGE_INFO *) Ghcb->SharedBuffer;
> +
> +    SetMem (Info, sizeof (*Info), 0);
> +
> +    //
> +    // Build page state change buffer
> +    //
> +    for (i = 0; (EndAddress > BaseAddress) && i < SNP_PAGE_STATE_MAX_ENTRY;
> +          BaseAddress = NextAddress, i++) {
> +      //
> +      // Is this a 2MB aligned page? Check if we can use the Large RMP entry.
> +      //
> +      if (UseLargeEntry &&
> +          IS_ALIGNED (BaseAddress, EFI_LARGE_PAGE) &&
> +          ((EndAddress - BaseAddress) >> EFI_PAGE_SHIFT) >=
> PAGES_PER_LARGE_ENTRY) {
> +        RmpPageSize = PVALIDATE_PAGE_SIZE_2M;
> +        NextAddress = BaseAddress + EFI_LARGE_PAGE;
> +      } else {
> +        RmpPageSize = PVALIDATE_PAGE_SIZE_4K;
> +        NextAddress = BaseAddress + EFI_PAGE_SIZE;
> +      }
> +
> +      Info->Entry[i].GuestFrameNumber = BaseAddress >> EFI_PAGE_SHIFT;
> +      Info->Entry[i].PageSize = RmpPageSize;
> +      Info->Entry[i].Op = MemoryStateToGhcbOp (State);
> +      Info->Entry[i].CurrentPage = 0;
> +    }
> +
> +    Info->Header.CurrentEntry = 0;
> +    Info->Header.EndEntry = i - 1;
> +
> +    //
> +    // If the request page state change is shared then invalidate the pages before
> +    // adding the page in the RMP table.
> +    //
> +    if (State == SevSnpPageShared) {
> +      PvalidateRange (Info, 0, i, FALSE);
> +    }
> +
> +    //
> +    // Issue the VMGEXIT and retry if hypervisor failed to process all the entries.
> +    //
> +    Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
> +    while (Info->Header.CurrentEntry <= Info->Header.EndEntry) {
> +      Status = VmgExit (Ghcb, SVM_EXIT_SNP_PAGE_STATE_CHANGE, 0, 0);
> +      if (EFI_ERROR (Status)) {
> +        SnpPageStateFailureTerminate ();
> +      }
> +    }
> +
> +    //
> +    // If the request page state change is shared then invalidate the pages before
> +    // adding the page in the RMP table.
> +    //
> +    if (State == SevSnpPagePrivate) {
> +      PvalidateRange (Info, 0, i, TRUE);
> +    }
> +
> +    VmgDone (Ghcb, InterruptState);
> +  }
> +}
> --
> 2.17.1