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

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

On 4/23/21 8:04 AM, Laszlo Ersek wrote:
> On 04/23/21 12:26, Laszlo Ersek wrote:
>> review#2 from scratch:
>>
>> On 04/21/21 00:54, Tom Lendacky 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%7Ca24cbb3f52404e466fe808d906585034%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547798659640707%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=o2rAydWG7jenRhqbZx3oDffZhlCUimJL9f9Tpmy4etk%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) As discussed, please update the commit message, for more clarify
>> about SEV vs. SEV-ES.
>>
>>>
>>> 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) {
>>
>> It's OK to keep this as a sanity check, yes.
>>
>>> +    RETURN_STATUS  DecryptStatus;
>>> +
>>> +    DecryptStatus = MemEncryptSevClearPageEncMask (
>>> +                      0,
>>> +                      TpmBaseAddress,
>>> +                      EFI_SIZE_TO_PAGES (0x5000),
>>
>> (2) Should be (UINTN)0x5000, as discussed earlier.
>>
>>> +                      FALSE
>>> +                      );
>>> +
>>> +    ASSERT_RETURN_ERROR (DecryptStatus);
>>
>> (3) So this is where the mess begins.
>>
>> The idea is to delay the dispatch of Tcg2ConfigPei until after
>> PlatformPei determines if SEV is active, and (in case SEV is active)
>> PlatformPei decrypts the MMIO range of the TPM.
>>
>> For this, we need to change the IA32 / X64 DEPEX of Tcg2ConfigPei, from
>> the current TRUE, to some PPI GUID.
>>
>> There are two choices for that PPI:
>>
>> (a) gEfiPeiMemoryDiscoveredPpiGuid
>>
>> Advantages:
>>
>> - no new PPI definition needed,
>>
>> - no new PPI installation needed,
>>
>> - OvmfPkg/Bhyve/PlatformPei needs no separate change
>>
>> Disadvantages:
>>
>> - total abuse of gEfiPeiMemoryDiscoveredPpiGuid
>>
>>
>> (b) gOvmfTpmMmioAccessiblePpiGuid
>>
>> Disadvantages:
>>
>> - this new GUID must be defined in "OvmfPkg.dec", in the [Ppis] section,
>> in a separate patch; its comment should say "this PPI signals that
>> accessing the MMIO range of the TPM is possible in the PEI phase,
>> regardless of memory encryption". The PPI definitions should be kept
>> alphabetically ordered.
>>
>> - OvmfPkg/PlatformPei must install this new PPI, with a NULL interface.
>> (See "mPpiBootMode" as a technical example.) OvmfPkg/PlatformPei must
>> install this new PPI either when the SEV check at the top of
>> AmdSevInitialize() fails, or when MemEncryptSevClearPageEncMask() succeeds.
>>
>> - OvmfPkg/Bhyve/PlatformPei must receive the same update, in a separate
>> patch. That's because "OvmfPkg/Bhyve/BhyveX64.fdf" includes the same
>> "Tcg2ConfigPei", but consumes "OvmfPkg/Bhyve/PlatformPei" rather than
>> "OvmfPkg/PlatformPei". Tcg2ConfigPei will receive the same
>> stricter-than-before depex, so something on the bhyve platform too must
>> produce the new PPI.
>>
>> Advantages:
>>
>> - more or less palatable as a concept, with the new PPI precisely
>> expressing the dependency we have.
>>
>>
>> In approach (b), the "OvmfPkg/Bhyve/PlatformPei" patch needs to be CC'd
>> to the Bhyve reviewers. If the Bhyve reviewers determine that such an
>> update is actually unnecessary, because on Bhyve, there is no TPM
>> support and/or no SEV support in fact, then *first* we have to create an
>> independent Bhyve cleanup series, that rips out the TPM and/or SEV
>> remnants from the OvmfPkg/Bhyve sub-tree.
>>
>>
>> I prefer approach (b). I'm sorry that it means extra work wrt. Bhyve,
>> but I strongly believe in keeping all platforms in the tree, and that
>> means we need to spend time on such changes.
>>
>> I'm not CC'ing Rebecca and Peter on this message -- we're deep into this
>> patch review thread, and they'd have no useful context. I suggest simply
>> including the "OvmfPkg/Bhyve/PlatformPei" patch in the next version of
>> this series, with a proper explanation in the blurb (patch#0) and on the
>> "OvmfPkg/Bhyve/PlatformPei" patch. That should give them enough context
>> to evaluate whether the change is necessary, or whether we should purge
>> the TPM and/or the SEV bits from Bhyve. You could also ask them just
>> this question in advance, in a separate email on the list (with
>> distilled context). Personally I'm unsure if the TPM and SEV bits
>> survived into Bhyve because those bits are actually put to use there, or
>> because the initial platform creation / cloning wasn't as minimal as it
>> could have been.
>>
>> Note that in case TPM makes sense on bhyve but SEV doesn't, then
>> "OvmfPkg/Bhyve/PlatformPei" will have to install the new PPI
>> unconditionally.
> 
> I've had a further idea on this.
> 
> You could add an entirely new PEIM just for this. The entry point
> function of the PEIM would check for SEV, decrypt the TPM range if SEV
> were active, and then install gOvmfTpmMmioAccessiblePpiGuid
> (unconditionally). The exit status of the PEIM would always be
> EFI_ABORTED, because there would be no need to keep the PEIM resident.
> 
> The new PEIM would have a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid, to
> make sure that potential page table splitting for the potential MMIO
> range decryption could be satisfied from permanent PEI RAM.
> 
> The new PEIM would be included in the DSC and FDF files of the usual
> three OVMF platforms, and in the Bhyve platform -- dependent on the
> TPM_ENABLE build flag.
> 
> There are several advantages to such a separate PEIM:
> 
> - For Bhyve, the update is minimal. Just include one line in each of the
> FDF and the DSC files. No need to customize an existent
> platform-specific PEIM, no code duplication between two PlatformPei modules.
> 
> - The new PEIM would depend on the TPM_ENABLE build flag, so it would
> only be included in the firmware binaries if and only if Tcg2ConfigPei
> were. No useless PPI installation would occur in the absence of TPM_ENABLE.
> 
> - No need to check PcdTpmBaseAddress for nullity in the new PEIM, before
> the decryption, as TPM_ENABLE guarantees (on IA32/X64) that the PCD
> already has the right value.
> 
> - The new logic would be properly ordered between PlatformPei and
> Tcg2ConfigPei, namely due to the use of two such PPI GUIDs in DEPEXes
> that actually make sense. PlatformPei -> TPM MMIO decryptor PEIM ordered
> via "memory discovered" (needed for potential page table splitting), TPM
> MMIO decryptor PEIM -> Tcg2ConfigPei ordered via "TPM MMIO decrypted".
> 
> You could place the new PEIM at:
> 
>   OvmfPkg/Tcg/TpmMmioSevDecryptPei
> 
> If you haven't lost your patience with me yet, I'd really appreciate if
> you could investigate this!
> 

So far, this appears to be working nicely. I'm new at the whole PEIM
thing, so hopefully I haven't missed anything. I should be submitting the
patches soon for review.

One thing I found is that the Bhyve package makes reference to the
OvmfPkg/Bhyve/Tcg directory, but that directory does not exist. So I don't
think that TPM enablement has been tested. I didn't update the Bhyve
support for that reason.

Thanks,
Tom

> Thanks!
> Laszlo
>