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.lenda...@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 <ler...@redhat.com> >>>>>> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >>>>>> Cc: Jordan Justen <jordan.l.jus...@intel.com> >>>>>> Cc: Brijesh Singh <brijesh.si...@amd.com> >>>>>> Cc: James Bottomley <j...@linux.ibm.com> >>>>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>>>> Cc: Min Xu <min.m...@intel.com> >>>>>> Signed-off-by: Tom Lendacky <thomas.lenda...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
