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

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

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