Re: [edk2-devel] [PATCH v3 4/5] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES

["Laszlo Ersek" <lersek@redhat.com>]

On 04/29/21 19:12, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345
> 
> During PEI, the MMIO range for the TPM is marked as encrypted when running
> as an SEV guest. While this isn't an issue for an SEV guest because of
> the way the nested page fault is handled, it does result in an SEV-ES
> guest terminating because of a mitigation check in the #VC handler to
> prevent MMIO to an encrypted address. For an SEV-ES guest, this range
> must be marked as unencrypted.
> 
> Create a new x86 PEIM for TPM support that will map the TPM MMIO range as
> unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI
> will be unconditionally installed before exiting. The PEIM will exit with
> the EFI_ABORTED status so that the PEIM does not stay resident. This new
> PEIM will depend on the installation of the permanent PEI RAM, by
> PlatformPei, so that in case page table splitting is required during the
> clearing of the encryption bit, the new page table(s) will be allocated
> from permanent PEI RAM.
> 
> Update all OVMF Ia32 and X64 build packages to include this new PEIM.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc                              |  1 +
>  OvmfPkg/OvmfPkgIa32.dsc                                   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                                |  1 +
>  OvmfPkg/OvmfPkgX64.dsc                                    |  1 +
>  OvmfPkg/AmdSev/AmdSevX64.fdf                              |  1 +
>  OvmfPkg/OvmfPkgIa32.fdf                                   |  1 +
>  OvmfPkg/OvmfPkgIa32X64.fdf                                |  1 +
>  OvmfPkg/OvmfPkgX64.fdf                                    |  1 +
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  | 87 ++++++++++++++++++++
>  10 files changed, 135 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index cdb29d53142d..66bbbc80cd18 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -626,6 +626,7 @@ [Components]
>    OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 1730b6558b5c..33fbd767903e 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -706,6 +706,7 @@ [Components]
>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 78a559da0d0b..b13e5cfd9047 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -719,6 +719,7 @@ [Components.IA32]
>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a7d747f6b4ab..999738dc39cd 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -718,6 +718,7 @@ [Components]
>    UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index c0098502aa90..dd0030dbf189 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -147,6 +147,7 @@ [FV.PEIFV]
>  INF  OvmfPkg/AmdSev/SecretPei/SecretPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index f400c845b9c9..b3c8b56f3b60 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -162,6 +162,7 @@ [FV.PEIFV]
>  INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index d055552fd09f..86592c2364a3 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -162,6 +162,7 @@ [FV.PEIFV]
>  INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index d519f8532822..d6be798fcadd 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -174,6 +174,7 @@ [FV.PEIFV]
>  INF  UefiCpuPkg/CpuMpPei/CpuMpPei.inf
>  
>  !if $(TPM_ENABLE) == TRUE
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> new file mode 100644
> index 000000000000..51ad6d0d055d
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> @@ -0,0 +1,40 @@
> +## @file
> +# Map TPM MMIO range unencrypted when SEV-ES is active.
> +# Install gOvmfTpmMmioAccessiblePpiGuid unconditionally.
> +#
> +# Copyright (C) 2021, Advanced Micro Devices, Inc.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = TpmMmioSevDecryptPei
> +  FILE_GUID                      = F12F698A-E506-4A1B-B32E-6920E55DA1C4
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = TpmMmioSevDecryptPeimEntryPoint
> +
> +[Sources]
> +  TpmMmioSevDecryptPeim.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  SecurityPkg/SecurityPkg.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  MemEncryptSevLib
> +  PcdLib
> +  PeimEntryPoint
> +  PeiServicesLib
> +
> +[Ppis]
> +  gOvmfTpmMmioAccessiblePpiGuid                      ## PRODUCES
> +
> +[FixedPcd]
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress    ## CONSUMES
> +
> +[Depex]
> +  gEfiPeiMemoryDiscoveredPpiGuid
> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
> new file mode 100644
> index 000000000000..df2ad623308d
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
> @@ -0,0 +1,87 @@
> +/** @file
> +  Map TPM MMIO range unencrypted when SEV-ES is active.
> +  Install gOvmfTpmMmioAccessiblePpiGuid unconditionally.
> +
> +  Copyright (C) 2021, Advanced Micro Devices, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +
> +#include <PiPei.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeiServicesLib.h>
> +
> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR  mTpmMmioRangeAccessible = {
> +  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> +  &gOvmfTpmMmioAccessiblePpiGuid,
> +  NULL
> +};
> +
> +/**
> +  The entry point for TPM MMIO range mapping driver.
> +
> +  @param[in]  FileHandle   Handle of the file being invoked.
> +  @param[in]  PeiServices  Describes the list of possible PEI Services.
> +
> +  @retval  EFI_ABORTED  No need to keep this PEIM resident
> +**/
> +EFI_STATUS
> +EFIAPI
> +TpmMmioSevDecryptPeimEntryPoint (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  RETURN_STATUS                   DecryptStatus;
> +  EFI_STATUS                      Status;
> +
> +  DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__));
> +
> +  //
> +  // If SEV is active, MMIO succeeds against an encrypted physical address
> +  // because the nested page fault (NPF) that occurs on access does not
> +  // include the encryption bit in the guest physical address provided to the
> +  // hypervisor.
> +  //
> +  // If SEV-ES is active, MMIO would succeed against an encrypted physical
> +  // address because the #VC handler uses the virtual address (which is an
> +  // identity mapped physical address without the encryption bit) as the guest
> +  // physical address of the MMIO target in the VMGEXIT.
> +  //
> +  // However, if SEV-ES is active, before performing the actual MMIO, an
> +  // additional MMIO mitigation check is performed in the #VC handler to ensure
> +  // that MMIO is being done to/from an unencrypted address. To prevent guest
> +  // termination in this scenario, mark the range unencrypted ahead of access.
> +  //
> +  if (MemEncryptSevEsIsEnabled ()) {
> +    DEBUG ((DEBUG_INFO,
> +      "%a: mapping TPM MMIO address range unencrypted\n",
> +      __FUNCTION__));
> +
> +    DecryptStatus = MemEncryptSevClearPageEncMask (
> +                      0,
> +                      FixedPcdGet64 (PcdTpmBaseAddress),
> +                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
> +                      FALSE
> +                      );
> +
> +    if (RETURN_ERROR (DecryptStatus)) {
> +      DEBUG ((DEBUG_ERROR,
> +        "%a: failed to map TPM MMIO address range unencrypted\n",
> +        __FUNCTION__));
> +      ASSERT_RETURN_ERROR (DecryptStatus);
> +    }
> +  }
> +
> +  //
> +  // MMIO range available
> +  //
> +  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return EFI_ABORTED;
> +}
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>