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

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

I'm going to ask for v3 after all:

On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> 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.

(1) Please spell out that the new PEIM will depend on the installation
of the permanent PEI RAM, by PlatformPei -- the reason being that, in
case page table splitting proves necessary for clearing the C-bit, the
new page table(s) should be allocated from permanent PEI RAM.


> 
> The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a
> Depex for IA32 and X64 builds so that the MMIO range is properly mapped
> for SEV-ES before the Tcg2Config PEIM is loaded.

(2) The Tcg2Config depex change should be a separate patch -- the last
patch in the series. That covers both the Tcg2Config hunk in the patch
body, and this commit message paragraph right here.


(3) While at it: those parts of this patch that are *not* related to the
Tcg2Config PEIM can be squashed into the previous patch -- if you prefer
that.

I'm certainly happy with three separate patches though: for the DEC
file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the
Tcg2Config PEIM. So in total the series should include 4 or 5 patches
(up to you).


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

(4) Marc-André's name is garbled here too.

The following git config options are related:

- For encoding non-ASCII characters in git commits, the
"i18n.commitencoding" knob is relevant. Almost universally, this should
be "UTF-8" (assuming your text editor used for composing commit messages
produces UTF-8-encoded files).

- For formatting commits to patch emails, "i18n.logOutputEncoding"
matters. This should *always* be "UTF-8", when git-format-patch is invoked.


> 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/Tcg2Config/Tcg2ConfigPei.inf                  |  2 +-
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++
>  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c  | 76 ++++++++++++++++++++
>  11 files changed, 125 insertions(+), 1 deletion(-)

Right, skipping Bhyve is justified, per your previous report /
<https://bugzilla.tianocore.org/show_bug.cgi?id=3354>.

> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index cdb29d53142d..5a5246c64bf7 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -627,6 +627,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>

(5) Functionally correct, but it reads more nicely (from a logical
dependency POV) if we place the new PEIM first.

(Please apply to the rest of the DSC files, and the FDF files too.)


> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 1730b6558b5c..a33c14c673a0 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -707,6 +707,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 78a559da0d0b..a4ff7ed44705 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -720,6 +720,7 @@ [Components.IA32]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index a7d747f6b4ab..3fb56b3f9ff9 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -719,6 +719,7 @@ [Components]
>  
>  !if $(TPM_ENABLE) == TRUE
>    OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>    SecurityPkg/Tcg/TcgPei/TcgPei.inf
>    SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>      <LibraryClasses>
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
> index c0098502aa90..ab58a9c0b4da 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.fdf
> +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
> @@ -148,6 +148,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> index f400c845b9c9..fc0ae1f280df 100644
> --- a/OvmfPkg/OvmfPkgIa32.fdf
> +++ b/OvmfPkg/OvmfPkgIa32.fdf
> @@ -163,6 +163,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> index d055552fd09f..306fc5a9b60d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> @@ -163,6 +163,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> index d519f8532822..22c8664427d6 100644
> --- a/OvmfPkg/OvmfPkgX64.fdf
> +++ b/OvmfPkg/OvmfPkgX64.fdf
> @@ -175,6 +175,7 @@ [FV.PEIFV]
>  
>  !if $(TPM_ENABLE) == TRUE
>  INF  OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +INF  OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
>  INF  SecurityPkg/Tcg/TcgPei/TcgPei.inf
>  INF  SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
>  !endif
> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> index 6776ec931ce0..39d1deeed16b 100644
> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
> @@ -57,7 +57,7 @@ [Pcd]
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid                 ## PRODUCES
>  
>  [Depex.IA32, Depex.X64]
> -  TRUE
> +  gOvmfTpmMmioAccessiblePpiGuid
>  
>  [Depex.ARM, Depex.AARCH64]
>    gOvmfTpmDiscoveredPpiGuid
> diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> new file mode 100644
> index 000000000000..926113b8ffb0
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
> @@ -0,0 +1,40 @@
> +## @file
> +# Map TPM MMIO range unencrypted when SEV is active

(6) Please add another sentence here: "Install
gOvmfTpmMmioAccessiblePpiGuid unconditionally".


> +#
> +# Copyright (C) 2021, Advanced Micro Devices, Inc.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

(7) The latest INF spec version is 1.29:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Draft-Specification
https://tianocore-docs.github.io/edk2-InfSpecification/draft/

plus INF_VERSION no longer requires a binary-only (hex-only) format. So
please just write "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
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  SecurityPkg/SecurityPkg.dec

(8) Is MdeModulePkg necessary?


> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MemEncryptSevLib
> +  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..dd1f1a80b5b0
> --- /dev/null
> +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c
> @@ -0,0 +1,76 @@
> +/** @file
> +  Map TPM MMIO range unencrypted when SEV is active

(9) Same request as (6) -- please add another sentence: "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/PeiServicesLib.h>

(10) This Library #include list does not match the [LibraryClasses]
section of the INF file (the PeimEntryPoint class apart, which should
indeed only be in the INF file). In other words, BaseLib appears
superfluous in the INF file.


> +
> +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 or SEV-ES 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.
> +  //
> +  // 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 an unencrypted address. To prevent guest
> +  // termination in this scenario, mark the range unencrypted ahead of access.
> +  //

Lovely comment, thanks!

> +  if (MemEncryptSevEsIsEnabled ()) {
> +    DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__));
> +
> +    DecryptStatus = MemEncryptSevClearPageEncMask (
> +                      0,
> +                      PcdGet64 (PcdTpmBaseAddress),

(11) The INF file says [FixedPcd], so it would be cleanest to say
FixedPcdGet64() here.


(12) PcdLib is missing from both the [LibraryClasses] section and the
#include directives.


> +                      EFI_SIZE_TO_PAGES ((UINTN) 0x5000),
> +                      FALSE
> +                      );
> +
> +    if (RETURN_ERROR (DecryptStatus)) {
> +      DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__));

(13) Overlong line.


(14) Please report errors with DEBUG_ERROR.


> +      ASSERT_RETURN_ERROR (DecryptStatus);
> +    }
> +  }
> +
> +  //
> +  // MMIO range available
> +  //
> +  Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return EFI_ABORTED;
> +}
> 

Thanks!
Laszlo