Re: [RFC PATCH 18/19] OvmfPkg/MemEncryptSevLib: Validate the memory during set or clear enc attribute

["Brijesh Singh" <brijesh.singh@amd.com>]

On 3/24/21 10:32 AM, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>
> The MemEncryptSev{Set,Clear}PageEncMask() functions are used to set or
> clear the memory encryption attribute in the page table. When SEV-SNP is
> active, we also need to validate or invalidate the pages and update the
> RMP entry.
>
> Before clearing the encryption attribute we need to invalidate the memory,
> and then make the page shared in the RMP entry. Similarly, after setting
> the encryption attribute in the page table, we add the memory as private
> in the RMP entry and validate it.
>
> 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/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf     |  3 ++
>  OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf     |  1 +
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c | 32 +++++++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c   | 36 ++++++++++++++++++--
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h       | 27 +++++++++++++++
>  5 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> index f2e162d680..fa8f7719a7 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
> @@ -36,6 +36,8 @@
>  [Sources.X64]
>    X64/MemEncryptSevLib.c
>    X64/PeiDxeVirtualMemory.c
> +  X64/SnpPageStateChangeInternal.c
> +  X64/PeiDxeSnpSetPageState.c
>    X64/VirtualMemory.c
>    X64/VirtualMemory.h
>  
> @@ -49,6 +51,7 @@
>    DebugLib
>    MemoryAllocationLib
>    PcdLib
> +  VmgExitLib
>  
>  [FeaturePcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> index cb9dd2bb21..d16ec44954 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
> @@ -37,6 +37,7 @@
>  [Sources.X64]
>    X64/MemEncryptSevLib.c
>    X64/PeiDxeVirtualMemory.c
> +  X64/PeiDxeSnpSetPageState.c
>    X64/PeiSnpSystemRamValidate.c
>    X64/SnpPageStateTrack.c
>    X64/SnpPageStateChangeInternal.c
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c
> new file mode 100644
> index 0000000000..0a3d58ac22
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeSnpSetPageState.c
> @@ -0,0 +1,32 @@
> +/** @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 "PeiSnpPageStateChange.h"


There is one minor error in this hunk, Please ignore it during your
reviews. I will fix it in next version.

The include should be "../SnpPageStateChange.h" and not
"PeiSnppageStateChange.h".


> +
> +VOID
> +SnpSetMemoryPrivate (
> +  IN  PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN  UINTN                 Length
> +  )
> +{
> +  SetPageStateInternal (PhysicalAddress, EFI_SIZE_TO_PAGES (Length), SevSnpPagePrivate, FALSE);
> +}
> +
> +VOID
> +SnpSetMemoryShared (
> +  IN  PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN  UINTN                 Length
> +  )
> +{
> +  SetPageStateInternal (PhysicalAddress, EFI_SIZE_TO_PAGES (Length), SevSnpPageShared, FALSE);
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index 33d9bafe9f..26d363d427 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -17,6 +17,7 @@
>  #include <Register/Cpuid.h>
>  
>  #include "VirtualMemory.h"
> +#include "SnpSetPageState.h"
>  
>  STATIC BOOLEAN mAddressEncMaskChecked = FALSE;
>  STATIC UINT64  mAddressEncMask;
> @@ -700,22 +701,34 @@ SetMemoryEncDec (
>    UINT64                         AddressEncMask;
>    BOOLEAN                        IsWpEnabled;
>    RETURN_STATUS                  Status;
> +  BOOLEAN                        NeedPageStateChange;
> +  PHYSICAL_ADDRESS               OrigPhysicalAddress;
> +  UINTN                          OrigLength;
>  
>    //
>    // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
>    //
>    PageMapLevel4Entry = NULL;
>  
> +  //
> +  // When SEV-SNP is active, before clearing the encryption attribute from
> +  // the page table we also need to update the RMP entry for the memory
> +  // region to make the region shared. And after setting the encryption
> +  // attribute, the region must be made private in the RMP table.
> +  //
> +  NeedPageStateChange = MemEncryptSevSnpIsEnabled ();
> +
>    DEBUG ((
>      DEBUG_VERBOSE,
> -    "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
> +    "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u Rmpupdate=%u\n",
>      gEfiCallerBaseName,
>      __FUNCTION__,
>      Cr3BaseAddress,
>      PhysicalAddress,
>      (UINT64)Length,
>      (Mode == SetCBit) ? "Encrypt" : "Decrypt",
> -    (UINT32)CacheFlush
> +    (UINT32)CacheFlush,
> +    (UINT32)NeedPageStateChange
>      ));
>  
>    //
> @@ -749,6 +762,18 @@ SetMemoryEncDec (
>      DisableReadOnlyPageWriteProtect ();
>    }
>  
> +  //
> +  // Make the RMP updates before clearing the encryption attribute in the page table.
> +  //
> +  if (NeedPageStateChange && (Mode == ClearCBit)) {
> +    SnpSetMemoryShared (PhysicalAddress, Length);
> +  }
> +
> +  //
> +  // Save the values, we need it later during the Page state change.
> +  //
> +  OrigPhysicalAddress = PhysicalAddress;
> +  OrigLength = Length;
>    Status = EFI_SUCCESS;
>  
>    while (Length != 0)
> @@ -923,6 +948,13 @@ SetMemoryEncDec (
>    //
>    CpuFlushTlb();
>  
> +  //
> +  // Make the RMP updates after setting the encryption attribute in the page table.
> +  //
> +  if (NeedPageStateChange && (Mode == SetCBit)) {
> +    SnpSetMemoryPrivate (OrigPhysicalAddress, OrigLength);
> +  }
> +
>  Done:
>    //
>    // Restore page table write protection, if any.
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h
> new file mode 100644
> index 0000000000..0b29bad612
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SnpSetPageState.h
> @@ -0,0 +1,27 @@
> +/** @file
> +
> +  SEV-SNP Page Validation functions.
> +
> +  Copyright (c) 2020 - 2021, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef PEI_DXE_SNP_PAGE_STATE_INTERNAL_H_
> +#define PEI_DXE_SNP_PAGE_STATE_INTERNAL_H_
> +
> +#include "../SnpPageStateChange.h"
> +
> +VOID
> +SnpSetMemoryPrivate (
> +  IN  PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN  UINTN                 Length
> +  );
> +
> +VOID
> +SnpSetMemoryShared (
> +  IN  PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN  UINTN                 Length
> +  );
> +#endif