[PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV


Lendacky, Thomas
 

On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.

Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

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: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten by
not having the address marked as shared/unencrypted.

I was also thinking of just marking everything above the highest system
memory address below 4GB and up to the PcdOvmfFdBaseAddress as
unencrypted. That also works.

Once DXE starts it takes care of all this and there isn't an issue in DXE.
This is just for PEI.

Thoughts? Other suggestions?

Thanks,
Tom

//
// Check and perform SEV-ES initialization if required.
//


Laszlo Ersek
 

On 04/21/21 01:13, Lendacky, Thomas wrote:
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?


Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

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: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten
by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.

Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:

diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)


(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).


(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.

(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)

Thanks
Laszlo


Laszlo Ersek
 

On 04/22/21 09:34, Laszlo Ersek wrote:

Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei
is guaranteed to be dispatched before Tcg2ConfigPei. The effective
depex of Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according
to the build report file. If Tcg2ConfigPei runs first, whatever we do
in PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range
even if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:

diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in
"MemEncryptSev.c" should check MemEncryptSevIsEnabled(), and then make
the above-seen MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the
library *class* carries SEV in the name, which is inherently
X64-specific, thus I wouldn't even like the lib *class* to leak into
ArmVirtPkg.)
Here's another thing. Above, I mention that nothing guarantees that
Tcg2ConfigPei runs before PlatformPei. That raises a problem even if we
use approach (2).

In approach (2), we massage page table entries, and ultimately use

OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf

for that. But that library instance can allocate full pages, in case
page table splitting is needed (from 1GB to 2MB to 4KB).

I can't tell off-hand if such page table splitting will actually occur
when we decrypt the TPM MMIO address range -- but even if it does not,
for whatever reason, I wouldn't like to rely on that particular reason.
And I definitely don't want such page allocations to be satisfied from
the temporary SEC/PEI heap, before we migrate to permanent PEI RAM. See
how "PcdOvmfSecPeiTempRamBase" and "PcdOvmfSecPeiTempRamSize" are set in
the FDF files, and see the OVMF log too:

Temp Stack : BaseAddress=0x818000 Length=0x8000
Temp Heap : BaseAddress=0x810000 Length=0x8000
Total temporary memory: 65536 bytes.
temporary memory stack ever used: 30128 bytes.
temporary memory heap used for HobList: 7208 bytes.
temporary memory heap occupied by memory pages: 0 bytes.
What I'm saying is that we've probably been missing the following hunk
for a long time now:

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
index 03a78c32df28..1b3808305415 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf
@@ -55,3 +55,6 @@ [FeaturePcd]

[FixedPcd]
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
+
+[Depex]
+ gEfiPeiMemoryDiscoveredPpiGuid
In other words, whatever PEIM uses the PeiMemEncryptSevLib instance,
should be delayed until PlatformPei installs the permanent PEI RAM, by
inheriting a gEfiPeiMemoryDiscoveredPpiGuid depex from
PeiMemEncryptSevLib.

... Unfortunately, this wouldn't work, because PlatformPei itself uses
PeiMemEncryptSevLib [*], so we'd create a circular dependency.

[*] first from commit 13b5d743c87a ("OvmfPkg/PlatformPei: Set memory
encryption PCD when SEV is enabled", 2017-07-10), which called
MemEncryptSevIsEnabled(),

and then from commit 449a6e493418 ("OvmfPkg: Create GHCB pages for
use during Pei and Dxe phase", 2020-08-17), which even called
MemEncryptSevClearPageEncMask() -- but note that AmdSevInitialize()
is called *after* PublishPeiMemory(), in PlatformPei!

So, we can't add this "permanent PEI RAM" dependency to
"PeiMemEncryptSevLib.inf" directly. Instead, as a work-around, we should
add the dependency to "Tcg2ConfigPei".

(5a) So ultimately, please update the "Tcg2ConfigPei.inf" file like
this:

diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..6605b9bbaf91 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,8 +60,11 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
- TRUE
+ gEfiPeiMemoryDiscoveredPpiGuid

[Depex.ARM, Depex.AARCH64]
gOvmfTpmDiscoveredPpiGuid
(5b) And in the commit message, please state that:

We don't want PeiMemEncryptSevLib to allocate any pages, for
potential page table splitting, from the temporary SEC/PEI heap. But
we can't make PeiMemEncryptSevLib itself depend on
gEfiPeiMemoryDiscoveredPpiGuid, because PlatformPei, which installs
the permanent PEI RAM, consumes PeiMemEncryptSevLib too. Thus,
restrict the DEPEX of Tcg2ConfigPei directly.

--*--

(

Note that, in OVMF, the other PEIM that (indirectly) uses
PeiMemEncryptSevLib is "UefiCpuPkg/CpuMpPei/CpuMpPei.inf". But, the
effective initialization of that PEIM is already delayed until after the
permanent PEI RAM is installed -- not with a depex, but with a notify
callback.

Also note that the library instance

OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf

doesn't support manipulating the page tables at all, and so it doesn't
need to allocate memory for page table splitting either. So that's good.

)

Thanks
Laszlo


Laszlo Ersek
 

On 04/22/21 09:34, Laszlo Ersek wrote:

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
Sorry, another point:

(6) where we determine that no TPM is available:

//
// If no TPM2 was detected, we still need to install
// TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
// the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
// to install the PPI in its place, in order to unblock any dependent
// PEIMs.
//
Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);

we should re-encrypt the address range, as if nothing had happened.

For this, we'll likely need a similarly polymorphic function called
InternalTpmEncryptAddressRange().

(

For some background on this particular branch of the code, please refer
to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09):

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
&gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
(Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

)

Thanks
Laszlo


Lendacky, Thomas
 

On 4/22/21 2:34 AM, Laszlo Ersek wrote:
On 04/21/21 01:13, Lendacky, Thomas wrote:
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io 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&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&amp;reserved=0

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?
Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.



Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

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: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten
by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.

Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
I'll take the input from each of your emails on this and see how that all
works. Thanks for the insight and knowledge!

Tom


diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)


(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).


(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.

(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)

Thanks
Laszlo


Lendacky, Thomas
 

On 4/22/21 9:51 AM, Tom Lendacky wrote:
On 4/22/21 2:34 AM, Laszlo Ersek wrote:
On 04/21/21 01:13, Lendacky, Thomas wrote:
On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io 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&;data=04%7C01%7Cthomas.lendacky%40amd.com%7C6b8da1f9a3bf4fb5f01e08d905613998%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637546737416495415%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=5vPlHPzGlS2%2Bqu3U4RPMITpyY%2F2ZxKJlaVYfFZItONQ%3D&amp;reserved=0

The TPM support in OVMF performs MMIO accesses during the PEI phase. At
this point, MMIO ranges have not been marked un-encyrpted, so an SEV-ES
guest will fail attempting to perform MMIO to an encrypted address.
(1) The subject says SEV, not SEV-ES, and the code in the patch too
suggests SEV, not SEV-ES. If that's correct, can you please update the
commit message?
Yes, I'll update the commit message. The action is correct for all SEV
guests in general, but it is only with SEV-ES, where the tighter MMIO
checks can be performed, that an actual issue shows up.



Read the PcdTpmBaseAddress and mark the specification defined range
(0x5000 in length) as un-encrypted, to allow an SEV-ES guest to process
the MMIO requests.

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: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/PlatformPei/AmdSev.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..de60332e9390 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -113,6 +113,7 @@ [Pcd]

[FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory
gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index dddffdebda4b..d524929f9e10 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -141,6 +141,7 @@ AmdSevInitialize (
)
{
UINT64 EncryptionMask;
+ UINT64 TpmBaseAddress;
RETURN_STATUS PcdStatus;

//
@@ -206,6 +207,24 @@ AmdSevInitialize (
}
}

+ //
+ // PEI TPM support will perform MMIO accesses, be sure this range is not
+ // marked encrypted.
+ //
+ TpmBaseAddress = PcdGet64 (PcdTpmBaseAddress);
+ if (TpmBaseAddress != 0) {
+ RETURN_STATUS DecryptStatus;
+
+ DecryptStatus = MemEncryptSevClearPageEncMask (
+ 0,
+ TpmBaseAddress,
+ EFI_SIZE_TO_PAGES (0x5000),
+ FALSE
+ );
+
+ ASSERT_RETURN_ERROR (DecryptStatus);
+ }
+
Laszlo, I'm not sure if this is the best way to approach this. It is
simple and straight forward and the TCG/TPM support is one of the few
(only?) pieces of code that does actual MMIO during PEI that is bitten
by not having the address marked as shared/unencrypted.
In SEC, I think we have MMIO access too (LAPIC --
InitializeApicTimer()); why does that work?

Hmm... Is that because we're immediately in x2apic mode, and that means
CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit
decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic
support", 2015-11-30).) And, we have #VC handling in SEC too.
Missed this question in my earlier reply... LAPIC access has a dedicated
check in ValidateMmioMemory() to allow access in this case.

Thanks,
Tom


Anyway: I think the TPM (MMIO) access you see comes from this PEIM:

OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf

The driver uses the following library instance:

SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf

This library instance is what depends on "PcdTpmBaseAddress".

And it's not just that decrypting the TPM MMIO range in PlatformPei
"looks awkward", but I don't even see it immediately why PlatformPei is
guaranteed to be dispatched before Tcg2ConfigPei. The effective depex of
Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according to the
build report file. If Tcg2ConfigPei runs first, whatever we do in
PlatformPei is too late.

I also don't like that, with this patch, we'd decrypt the TPM range even
if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses
"PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero
default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is
set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex
too.)


(2) So, can you please try the following, in the
"OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module:
I'll take the input from each of your emails on this and see how that all
works. Thanks for the insight and knowledge!

Tom


diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
index 6776ec931ce0..0d0572b83599 100644
--- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
+++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf
@@ -20,13 +20,16 @@ [Defines]
ENTRY_POINT = Tcg2ConfigPeimEntryPoint

[Sources]
+ MemEncrypt.h
Tcg2ConfigPeim.c
Tpm12Support.h

[Sources.IA32, Sources.X64]
+ MemEncryptSev.c
Tpm12Support.c

[Sources.ARM, Sources.AARCH64]
+ MemEncryptNull.c
Tpm12SupportNull.c

[Packages]
@@ -43,6 +46,7 @@ [LibraryClasses]

[LibraryClasses.IA32, LibraryClasses.X64]
BaseLib
+ MemEncryptSevLib
Tpm12DeviceLib

[Guids]
@@ -56,6 +60,9 @@ [Ppis]
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES

+[Pcd.IA32, Pcd.X64]
+ gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES
+
[Depex.IA32, Depex.X64]
TRUE
In the "MemEncrypt.h" file, declare a function called
InternalTpmDecryptAddressRange(). The function definition in
"MemEncryptNull.c" should do nothing, while the one in "MemEncryptSev.c"
should check MemEncryptSevIsEnabled(), and then make the above-seen
MemEncryptSevClearPageEncMask() call.

The new InternalTpmDecryptAddressRange() function should be called from
Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().

(An alternative approach would be to call MemEncryptSevIsEnabled() and
MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also
on ARM / AARCH64. In addition to that, we'd have to implement a Null
instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null
instance in the ArmVirtPkg DSC files. But I don't like that: the library
*class* carries SEV in the name, which is inherently X64-specific, thus
I wouldn't even like the lib *class* to leak into ArmVirtPkg.)


(3) If the approach in (2) works, then please don't forget to update the
patch subject (it currently refers to PlatformPei).


(4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should
have type UINTN. The constant 0x5000 has type "int" (INT32); please cast
it to UINTN.

(In fact I would prefer a new macro for 0x5000, somewhere in the
"MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that
SecurityPkg already open-codes the 0x5000 constant in
"Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.)

Thanks
Laszlo


Lendacky, Thomas
 

On 4/22/21 3:39 AM, Laszlo Ersek wrote:
On 04/22/21 09:34, Laszlo Ersek wrote:

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
library's constructor is called before the OVMF Tcg2ConfigPei constructor.
The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
fails because the pages haven't been marked unencrypted yet by OVMF
Tcg2ConfigPei. Some debug output:

Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
*** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
*** DEBUG: Tpm2GetPtpInterface:425
*** DEBUG: Tpm2IsPtpPresence:51
MMIO using encrypted memory: FED40000
!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!

Thanks,
Tom

Sorry, another point:

(6) where we determine that no TPM is available:

//
// If no TPM2 was detected, we still need to install
// TpmInitializationDonePpi. Namely, Tcg2Pei will exit early upon seeing
// the default (all-bits-zero) contents of PcdTpmInstanceGuid, thus we have
// to install the PPI in its place, in order to unblock any dependent
// PEIMs.
//
Status = PeiServicesInstallPpi (&mTpmInitializationDonePpiList);

we should re-encrypt the address range, as if nothing had happened.

For this, we'll likely need a similarly polymorphic function called
InternalTpmEncryptAddressRange().

(

For some background on this particular branch of the code, please refer
to commit 6cf1880fb5b6 ("OvmfPkg: add customized Tcg2ConfigPei clone",
2018-03-09):

- Check the QEMU hardware for TPM2 availability only

- If found, set the dynamic PCD "PcdTpmInstanceGuid" to
&gEfiTpmDeviceInstanceTpm20DtpmGuid. This is what informs the rest of
the firmware about the TPM type.

- Install the gEfiTpmDeviceSelectedGuid PPI. This action permits the
PEI_CORE to dispatch the Tcg2Pei module, which consumes the above PCD.
In effect, the gEfiTpmDeviceSelectedGuid PPI serializes the setting
and the consumption of the "TPM type" PCD.

- If no TPM2 was found, install gPeiTpmInitializationDonePpiGuid.
(Normally this is performed by Tcg2Pei, but Tcg2Pei doesn't do it if
no TPM2 is available. So in that case our Tcg2ConfigPei must do it.)

)

Thanks
Laszlo


Laszlo Ersek
 

On 04/22/21 21:10, Tom Lendacky wrote:
On 4/22/21 3:39 AM, Laszlo Ersek wrote:
On 04/22/21 09:34, Laszlo Ersek wrote:

The new InternalTpmDecryptAddressRange() function should be called
from Tcg2ConfigPeimEntryPoint(), before the latter calls
InternalTpm12Detect(). Regarding error checking... if
InternalTpmDecryptAddressRange() fails, I think we can log an error
message, and hang with CpuDeadLoop().
Unfortunately, this method doesn't work. The OVMF Tcg2ConfigPei.inf file
uses the SecurityPkg Tpm2DeviceLib library. The SecurityPkg Tpm2DeviceLib
library's constructor is called before the OVMF Tcg2ConfigPei constructor.
The Tpm2DeviceLib constructor performs MMIO to the TPM base address and
fails because the pages haven't been marked unencrypted yet by OVMF
Tcg2ConfigPei. Some debug output:

Loading PEIM at 0x0007F793000 EntryPoint=0x0007F794E4F Tcg2ConfigPei.efi
*** DEBUG: InternalTpm2DeviceLibDTpmCommonConstructor:55
*** DEBUG: Tpm2GetPtpInterface:425
*** DEBUG: Tpm2IsPtpPresence:51
MMIO using encrypted memory: FED40000
!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!!
Thank you for checking this approach.

Let me re-review this patch from scratch.

Thanks
Laszlo