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


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

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