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

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

On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=G1GwQc6sZqRuNHWC5vbdb78gCOl4YkAq%2BHi0F0ceucg%3D&amp;reserved=0
>>
>> 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.

Will do.

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

Ok, I'll split that out.

> 
> 
> (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).

I'll do it as 5 patches.

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

We were having problems with sending patches via git-format-patch and
git-send-email and our email system. Likely some left over .gitconfig
entries that are causing the problem. I'll double check everything.

> 
> 
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3354&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=HPF7LTIV17CwZ%2BCFehXpnPb%2BtQCgpFPvLsVqfNj9HBI%3D&amp;reserved=0>.
> 
>>
>> 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.)

Ok, I was going with the alphabetical placement. I'll switch it up.

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

Will do.

> 
> 
>> +#
>> +# 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Draft-Specification&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=AXuQkvUSwLjEyZiwivQQaUwTaY7Mo0wLSHUf8QKNLC8%3D&amp;reserved=0
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftianocore-docs.github.io%2Fedk2-InfSpecification%2Fdraft%2F&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=YUHs6g5aMPWBjCNjcZPKnTSEs2gBazDX094nqj9qpnE%3D&amp;reserved=0
> 
> plus INF_VERSION no longer requires a binary-only (hex-only) format. So
> please just write "1.29".

Will do.

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

I don't think so. Let me double check it.

> 
> 
>> +
>> +[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".
> 

Will do.

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

Ok, let me check on that and fix as appropriate.

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

Will do.

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

Right, I'll update that.

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

Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
complain.

> 
> 
> (14) Please report errors with DEBUG_ERROR.

Yup, will change.

Thanks,
Tom

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