From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.9480.1619080284313448971 for ; Thu, 22 Apr 2021 01:31:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MQPiv9E5; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1619080283; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oz3/tGqtmsXC0wQFDX7C+HWbtnnUfqicjNeVpj5y2cM=; b=MQPiv9E5ol/jifXtX6u6tXAgEEiQtqMyzgohZTG7BopJNW6aZSFXpt0+guQVhN9k6yQYOr uhEfYREI9jHH2s1r7IhYEr/40WIwCsZHVTFRl/hSDQhdO49dq4uJ0evtIDjuzQuaegasd+ M5XTTlgXetsIVaJZIpsAlaZrjcHJDgI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-63-TiVI3SySMI2lLpRQlsNrkw-1; Thu, 22 Apr 2021 04:31:19 -0400 X-MC-Unique: TiVI3SySMI2lLpRQlsNrkw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D82A710054F6; Thu, 22 Apr 2021 08:31:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-151.ams2.redhat.com [10.36.112.151]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71A1B5C1B4; Thu, 22 Apr 2021 08:31:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV From: "Laszlo Ersek" To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Joerg Roedel , Borislav Petkov , Ard Biesheuvel , Jordan Justen , Brijesh Singh , James Bottomley , Jiewen Yao , Min Xu References: <1677B2EC90F30786.1355@groups.io> <007e59ea-3933-7b93-afff-4023f3111558@amd.com> <08f723a5-9883-7785-91c0-9e5627836288@redhat.com> Message-ID: <2ecb7ad4-3172-9124-6d4e-0ad01b79f665@redhat.com> Date: Thu, 22 Apr 2021 10:31:14 +0200 MIME-Version: 1.0 In-Reply-To: <08f723a5-9883-7785-91c0-9e5627836288@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/22/21 09:34, Laszlo Ersek wrote: > Anyway: I think the TPM (MMIO) access you see comes from this PEIM: > > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > > The driver uses the following library instance: > > SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf > > This library instance is what depends on "PcdTpmBaseAddress". > > And it's not just that decrypting the TPM MMIO range in PlatformPei > "looks awkward", but I don't even see it immediately why PlatformPei > is guaranteed to be dispatched before Tcg2ConfigPei. The effective > depex of Tcg2ConfigPei is just "gEfiPeiPcdPpiGuid" (on X64), according > to the build report file. If Tcg2ConfigPei runs first, whatever we do > in PlatformPei is too late. > > I also don't like that, with this patch, we'd decrypt the TPM range > even if OVMF weren't built with "-D TPM_ENABLE". Namely, OVMF uses > "PcdTpmBaseAddress" as fixed (not dynamic), inheriting the nonzero > default from "SecurityPkg.dec". (In ArmVirtQemu, PcdTpmBaseAddress is > set dynamically, which is why Tcg2ConfigPei has an ARM-specific depex > too.) > > > (2) So, can you please try the following, in the > "OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf" module: > >> diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> index 6776ec931ce0..0d0572b83599 100644 >> --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf >> @@ -20,13 +20,16 @@ [Defines] >> ENTRY_POINT = Tcg2ConfigPeimEntryPoint >> >> [Sources] >> + MemEncrypt.h >> Tcg2ConfigPeim.c >> Tpm12Support.h >> >> [Sources.IA32, Sources.X64] >> + MemEncryptSev.c >> Tpm12Support.c >> >> [Sources.ARM, Sources.AARCH64] >> + MemEncryptNull.c >> Tpm12SupportNull.c >> >> [Packages] >> @@ -43,6 +46,7 @@ [LibraryClasses] >> >> [LibraryClasses.IA32, LibraryClasses.X64] >> BaseLib >> + MemEncryptSevLib >> Tpm12DeviceLib >> >> [Guids] >> @@ -56,6 +60,9 @@ [Ppis] >> [Pcd] >> gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES >> >> +[Pcd.IA32, Pcd.X64] >> + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES >> + >> [Depex.IA32, Depex.X64] >> TRUE >> > > In the "MemEncrypt.h" file, declare a function called > InternalTpmDecryptAddressRange(). The function definition in > "MemEncryptNull.c" should do nothing, while the one in > "MemEncryptSev.c" should check MemEncryptSevIsEnabled(), and then make > the above-seen MemEncryptSevClearPageEncMask() call. > > The new InternalTpmDecryptAddressRange() function should be called > from Tcg2ConfigPeimEntryPoint(), before the latter calls > InternalTpm12Detect(). Regarding error checking... if > InternalTpmDecryptAddressRange() fails, I think we can log an error > message, and hang with CpuDeadLoop(). > > (An alternative approach would be to call MemEncryptSevIsEnabled() and > MemEncryptSevClearPageEncMask() regardless of architecture, i.e., also > on ARM / AARCH64. In addition to that, we'd have to implement a Null > instance of MemEncryptSevLib, and resolve MemEncryptSevLib to the Null > instance in the ArmVirtPkg DSC files. But I don't like that: the > library *class* carries SEV in the name, which is inherently > X64-specific, thus I wouldn't even like the lib *class* to leak into > ArmVirtPkg.) Here's another thing. Above, I mention that nothing guarantees that Tcg2ConfigPei runs before PlatformPei. That raises a problem even if we use approach (2). In approach (2), we massage page table entries, and ultimately use OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf for that. But that library instance can allocate full pages, in case page table splitting is needed (from 1GB to 2MB to 4KB). I can't tell off-hand if such page table splitting will actually occur when we decrypt the TPM MMIO address range -- but even if it does not, for whatever reason, I wouldn't like to rely on that particular reason. And I definitely don't want such page allocations to be satisfied from the temporary SEC/PEI heap, before we migrate to permanent PEI RAM. See how "PcdOvmfSecPeiTempRamBase" and "PcdOvmfSecPeiTempRamSize" are set in the FDF files, and see the OVMF log too: > Temp Stack : BaseAddress=0x818000 Length=0x8000 > Temp Heap : BaseAddress=0x810000 Length=0x8000 > Total temporary memory: 65536 bytes. > temporary memory stack ever used: 30128 bytes. > temporary memory heap used for HobList: 7208 bytes. > temporary memory heap occupied by memory pages: 0 bytes. What I'm saying is that we've probably been missing the following hunk for a long time now: > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > index 03a78c32df28..1b3808305415 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLib.inf > @@ -55,3 +55,6 @@ [FeaturePcd] > > [FixedPcd] > gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase > + > +[Depex] > + gEfiPeiMemoryDiscoveredPpiGuid In other words, whatever PEIM uses the PeiMemEncryptSevLib instance, should be delayed until PlatformPei installs the permanent PEI RAM, by inheriting a gEfiPeiMemoryDiscoveredPpiGuid depex from PeiMemEncryptSevLib. ... Unfortunately, this wouldn't work, because PlatformPei itself uses PeiMemEncryptSevLib [*], so we'd create a circular dependency. [*] first from commit 13b5d743c87a ("OvmfPkg/PlatformPei: Set memory encryption PCD when SEV is enabled", 2017-07-10), which called MemEncryptSevIsEnabled(), and then from commit 449a6e493418 ("OvmfPkg: Create GHCB pages for use during Pei and Dxe phase", 2020-08-17), which even called MemEncryptSevClearPageEncMask() -- but note that AmdSevInitialize() is called *after* PublishPeiMemory(), in PlatformPei! So, we can't add this "permanent PEI RAM" dependency to "PeiMemEncryptSevLib.inf" directly. Instead, as a work-around, we should add the dependency to "Tcg2ConfigPei". (5a) So ultimately, please update the "Tcg2ConfigPei.inf" file like this: > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > index 6776ec931ce0..6605b9bbaf91 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -20,13 +20,16 @@ [Defines] > ENTRY_POINT = Tcg2ConfigPeimEntryPoint > > [Sources] > + MemEncrypt.h > Tcg2ConfigPeim.c > Tpm12Support.h > > [Sources.IA32, Sources.X64] > + MemEncryptSev.c > Tpm12Support.c > > [Sources.ARM, Sources.AARCH64] > + MemEncryptNull.c > Tpm12SupportNull.c > > [Packages] > @@ -43,6 +46,7 @@ [LibraryClasses] > > [LibraryClasses.IA32, LibraryClasses.X64] > BaseLib > + MemEncryptSevLib > Tpm12DeviceLib > > [Guids] > @@ -56,8 +60,11 @@ [Ppis] > [Pcd] > gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES > > +[Pcd.IA32, Pcd.X64] > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## SOMETIMES_CONSUMES > + > [Depex.IA32, Depex.X64] > - TRUE > + gEfiPeiMemoryDiscoveredPpiGuid > > [Depex.ARM, Depex.AARCH64] > gOvmfTpmDiscoveredPpiGuid (5b) And in the commit message, please state that: We don't want PeiMemEncryptSevLib to allocate any pages, for potential page table splitting, from the temporary SEC/PEI heap. But we can't make PeiMemEncryptSevLib itself depend on gEfiPeiMemoryDiscoveredPpiGuid, because PlatformPei, which installs the permanent PEI RAM, consumes PeiMemEncryptSevLib too. Thus, restrict the DEPEX of Tcg2ConfigPei directly. --*-- ( Note that, in OVMF, the other PEIM that (indirectly) uses PeiMemEncryptSevLib is "UefiCpuPkg/CpuMpPei/CpuMpPei.inf". But, the effective initialization of that PEIM is already delayed until after the permanent PEI RAM is installed -- not with a depex, but with a notify callback. Also note that the library instance OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf doesn't support manipulating the page tables at all, and so it doesn't need to allocate memory for page table splitting either. So that's good. ) Thanks Laszlo