[edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV
Lendacky, Thomas
thomas.lendacky at amd.com
Mon Apr 26 14:21:24 UTC 2021
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 at 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%7Cc8a12e7c1e6a4282963508d908abe333%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637550356650588901%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3SeLVc3SscAozGX62BSAyWseujfwxzm6qIB%2Fh8ALyq0%3D&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 at redhat.com>
>>>>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>>>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>>>>>> Cc: James Bottomley <jejb at linux.ibm.com>
>>>>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>>>>> Cc: Min Xu <min.m.xu at intel.com>
>>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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
>>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74421): https://edk2.groups.io/g/devel/message/74421
Mute This Topic: https://groups.io/mt/82247968/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list