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.web11.9078.1619076938843836600 for ; Thu, 22 Apr 2021 00:35:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WFwkperj; 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=1619076938; 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=8+w9zESlEh71txiChH0b40uNmLfqhJjtBOWx7Ic3axk=; b=WFwkperjjGj0Q7at71VK+v1gnMvadnHnRaLpRymf1kDQnAU8O8wgaPaStOo10OPhDNc8H+ BQT5WbnbkgFZkBesIlnbI1x5czT6a8nZ3k0yObbU/P4nfvczjLn16dUS5m9XbLp+/0i1q6 NhCDzCgqgJOPWGfQ0gASFYQUR+WWfUk= 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-547-maysYUu0OUmyUTNMeFMbNg-1; Thu, 22 Apr 2021 03:34:59 -0400 X-MC-Unique: maysYUu0OUmyUTNMeFMbNg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 12422501EC; Thu, 22 Apr 2021 07:34:57 +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 A3A5962688; Thu, 22 Apr 2021 07:34:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/3] OvmfPkg/PlatformPei: Mark TPM MMIO range as unencrypted for SEV 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> From: "Laszlo Ersek" Message-ID: <08f723a5-9883-7785-91c0-9e5627836288@redhat.com> Date: Thu, 22 Apr 2021 09:34:53 +0200 MIME-Version: 1.0 In-Reply-To: <007e59ea-3933-7b93-afff-4023f3111558@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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/21/21 01:13, Lendacky, Thomas wrote: > On 4/20/21 5:54 PM, Lendacky, Thomas via groups.io wrote: >> From: Tom Lendacky >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345 >> >> 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) The subject says SEV, not SEV-ES, and the code in the patch too suggests SEV, not SEV-ES. If that's correct, can you please update the commit message? >> >> 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 >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Brijesh Singh >> Cc: James Bottomley >> Cc: Jiewen Yao >> Cc: Min Xu >> Signed-off-by: Tom Lendacky >> --- >> 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) { >> + RETURN_STATUS DecryptStatus; >> + >> + DecryptStatus = MemEncryptSevClearPageEncMask ( >> + 0, >> + TpmBaseAddress, >> + EFI_SIZE_TO_PAGES (0x5000), >> + FALSE >> + ); >> + >> + ASSERT_RETURN_ERROR (DecryptStatus); >> + } >> + > > Laszlo, I'm not sure if this is the best way to approach this. It is > simple and straight forward and the TCG/TPM support is one of the few > (only?) pieces of code that does actual MMIO during PEI that is bitten > by not having the address marked as shared/unencrypted. In SEC, I think we have MMIO access too (LAPIC -- InitializeApicTimer()); why does that work? Hmm... Is that because we're immediately in x2apic mode, and that means CPUID plus MSR accesses, and not MMIO? (I'm reminded of commit decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic support", 2015-11-30).) And, we have #VC handling in SEC too. 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.) (3) If the approach in (2) works, then please don't forget to update the patch subject (it currently refers to PlatformPei). (4) The argument of the EFI_SIZE_TO_PAGES() function-like macro should have type UINTN. The constant 0x5000 has type "int" (INT32); please cast it to UINTN. (In fact I would prefer a new macro for 0x5000, somewhere in the "MdePkg/Include/IndustryStandard/Tpm*.h" files; but I can see that SecurityPkg already open-codes the 0x5000 constant in "Tcg/Tcg2Acpi/Tpm.asl" and "Tcg/TcgSmm/Tpm.asl", so meh.) Thanks Laszlo