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

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

On 4/26/21 7:07 AM, Laszlo Ersek wrote:
> On 04/23/21 22:02, Tom Lendacky wrote:
>> On 4/23/21 12:41 PM, Tom Lendacky wrote:
>>> 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%7Cc8a12e7c1e6a4282963508d908abe333%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637550356650588901%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3SeLVc3SscAozGX62BSAyWseujfwxzm6qIB%2Fh8ALyq0%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.
>>
>> So one thing I failed to do before submitting my previous patch was to
>> complete my testing against the IA32 and X64 combination build. In this
>> build, PEI is built as Ia32, and MemEncryptSevClearPageEncMask() will
>> return UNSUPPORTED causing an ASSERT (since I check the return code). So
>> there are a few options:
>>
>> 1. SEV works with the current encrypted mapping, it is only the SEV-ES
>>    support that fails because of the ValidateMmioMemory() check. I can do
>>    the mapping change just for SEV-ES since it is X64 only. This works,
>>    because MemEncryptSevClearPageEncMask() will not return UNSUPPORTED
>>    when running in 64-bit.
> 
> Can we really say "SEV works" though? Because, even using an X64 PEI
> phase, and enabling only SEV (not SEV-ES), TPM access will be broken in
> the PEI phase. Is my understanding correct?

Because the memory range is marked as MMIO, we'll take a nested page fault
(NPF). The GPA passed as part of the NPF does not include the c-bit. So we
do in fact work properly with a TPM in SEV. SEV-ES would also work
properly if the mitigation for accessing an encrypted address was removed
from the #VC handler. It is only this added mitigation to protect MMIO
that results in an issue with the TPM in PEI.

> 
> I think the behavior you currently see is actually what we want, we
> should double down on it -- if MemEncryptSevClearPageEncMask() fails,
> report an explicit DEBUG_ERROR, and call CpuDeadLoop(). If the firmware
> is built with TPM_ENABLE, and SEV is active, then an IA32 PEI phase is
> simply unusable. Silently pretending that the TPM is not there, even
> though it may have been configured on the QEMU command line, we just
> failed to communicate with it, is not a good idea, IMO.

However, because the c-bit is not part of the NPF, we do communicate
successfully with the TPM.

So we could actually do following:
 - For IA32:
   - Remove the Depex on gOvmfTpmMmioAccessiblePpiGuid
   - Do not add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf

 - For X64:
   - Add the Depex on gOvmfTpmMmioAccessiblePpiGuid
   - Add OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf

That might be confusing, though. So we could just do option #3 below.

Thanks,
Tom

> 
> This is somewhat similar IMO to the S3Verification() function in
> "OvmfPkg/PlatformPei/Platform.c".
> 
> TPM_ENABLE, SEV, IA32 PEI phase: pick any two.
> 
> Thanks,
> Laszlo
> 
>>
>> 2. Call MemEncryptSevClearPageEncMask() for SEV or SEV-ES, but don't check
>>    the return status.
>>
>> 3. Create Ia32 and X64 versions of internal functions, where the Ia32
>>    version simply returns SUCCESS because it can't do anything and the X64
>>    version calls MemEncryptSevClearPageEncMask(), allowing the main code
>>    to ASSERT on any errors.
>>
>> I'm leaning towards #1, because this is an SEV-ES only issue. Thoughts?
>>
>> Thanks,
>> Tom
>>
>>>
>>> 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
>>>>
>>
>