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 9:21 AM, Tom Lendacky wrote:
> 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>

...

>>>>>
>>>>> 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.

So I'm thinking that I can have TpmMmioSevDecryptPeim.c do this:

  //
  // If SEV or SEV-ES is active, MMIO succeeds against an encrypted physical
  // address because the nested page fault (NPF) that occurs on access does not
  // include the encryption bit in the guest physical address provided to the
  // hypervisor.
  //
  // However, if SEV-ES is active, before performing the actual MMIO, an
  // additional MMIO mitigation check is performed in the #VC handler to ensure
  // that MMIO is being done to an unencrypted address. To prevent guest
  // termination in this scenario, mark the range unencrypted ahead of access.
  //
  if (MemEncryptSevEsIsEnabled ()) {
    // Do MemEncryptSevClearPageEncMask() ...
  }

Let me submit the next version with this and see what you think.

Thanks,
Tom

> 
>>
>> 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
>>>>>
>>>
>>