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


Lendacky, Thomas
 

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

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&;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@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Marc-Andr?? Lureau <marcandre.lureau@...>
(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@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
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&;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&;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&;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





Join devel@edk2.groups.io to automatically receive all group messages.