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.126.1619632320170589471 for ; Wed, 28 Apr 2021 10:52:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=K5o7o12e; 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=1619632319; 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=GMJacAQQmoxYIYW5MhpWprLv+pz+3VwQ1ZCTX3nrWY0=; b=K5o7o12efEMtXiwcxL4twA1MvUxRD/1hRctaTynpaPPgPATnUgMaJehyvlMdPYC3++1VlK jFkUFoGS6+AorJewmAJzQLN2yNsiE5+avKiAMxMxO+ooqJqDF0it01loL+H/2rX4G1sREW CM1XyIa8O2NQlSnfKdORvevbPS6br/w= 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-155-KSp0PWN3OGS8xnPwGREROA-1; Wed, 28 Apr 2021 13:51:55 -0400 X-MC-Unique: KSp0PWN3OGS8xnPwGREROA-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 84454107ACE4; Wed, 28 Apr 2021 17:51:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-188.ams2.redhat.com [10.36.114.188]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7DFC75C1C2; Wed, 28 Apr 2021 17:51:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Joerg Roedel , Borislav Petkov , Ard Biesheuvel , Jordan Justen , Brijesh Singh , Erdem Aktas , James Bottomley , Jiewen Yao , Min Xu , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Stefan Berger References: <00ff47c80f180b5b9054890de0ce5e1975fe2b1f.1619540470.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <6807464e-823e-3a16-cf1c-24f612a43936@redhat.com> Date: Wed, 28 Apr 2021 19:51:49 +0200 MIME-Version: 1.0 In-Reply-To: <00ff47c80f180b5b9054890de0ce5e1975fe2b1f.1619540470.git.thomas.lendacky@amd.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: 8bit I'm going to ask for v3 after all: On 04/27/21 18:21, Lendacky, Thomas wrote: > From: Tom Lendacky > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3345 > > During PEI, the MMIO range for the TPM is marked as encrypted when running > as an SEV guest. While this isn't an issue for an SEV guest because of > the way the nested page fault is handled, it does result in an SEV-ES > guest terminating because of a mitigation check in the #VC handler to > prevent MMIO to an encrypted address. For an SEV-ES guest, this range > must be marked as unencrypted. > > Create a new x86 PEIM for TPM support that will map the TPM MMIO range as > unencrypted when SEV-ES is active. The gOvmfTpmMmioAccessiblePpiGuid PPI > will be unconditionally installed before exiting. The PEIM will exit with > the EFI_ABORTED status so that the PEIM does not stay resident. (1) Please spell out that the new PEIM will depend on the installation of the permanent PEI RAM, by PlatformPei -- the reason being that, in case page table splitting proves necessary for clearing the C-bit, the new page table(s) should be allocated from permanent PEI RAM. > > The OVMF Tcg2Config PEIM will add the gOvmfTpmMmioAccessiblePpiGuid as a > Depex for IA32 and X64 builds so that the MMIO range is properly mapped > for SEV-ES before the Tcg2Config PEIM is loaded. (2) The Tcg2Config depex change should be a separate patch -- the last patch in the series. That covers both the Tcg2Config hunk in the patch body, and this commit message paragraph right here. (3) While at it: those parts of this patch that are *not* related to the Tcg2Config PEIM can be squashed into the previous patch -- if you prefer that. I'm certainly happy with three separate patches though: for the DEC file, for TpmMmioSevDecryptPei + the DSC/FDF files, and finally the Tcg2Config PEIM. So in total the series should include 4 or 5 patches (up to you). > > Update all OVMF Ia32 and X64 build packages to include this new PEIM. > > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Brijesh Singh > Cc: Erdem Aktas > Cc: James Bottomley > Cc: Jiewen Yao > Cc: Min Xu > Cc: Marc-Andr?? Lureau (4) Marc-André's name is garbled here too. The following git config options are related: - For encoding non-ASCII characters in git commits, the "i18n.commitencoding" knob is relevant. Almost universally, this should be "UTF-8" (assuming your text editor used for composing commit messages produces UTF-8-encoded files). - For formatting commits to patch emails, "i18n.logOutputEncoding" matters. This should *always* be "UTF-8", when git-format-patch is invoked. > Cc: Stefan Berger > Signed-off-by: Tom Lendacky > --- > OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/AmdSev/AmdSevX64.fdf | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 1 + > OvmfPkg/OvmfPkgX64.fdf | 1 + > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf | 2 +- > OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf | 40 +++++++++++ > OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c | 76 ++++++++++++++++++++ > 11 files changed, 125 insertions(+), 1 deletion(-) Right, skipping Bhyve is justified, per your previous report / . > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc > index cdb29d53142d..5a5246c64bf7 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.dsc > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc > @@ -627,6 +627,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > (5) Functionally correct, but it reads more nicely (from a logical dependency POV) if we place the new PEIM first. (Please apply to the rest of the DSC files, and the FDF files too.) > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 1730b6558b5c..a33c14c673a0 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -707,6 +707,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 78a559da0d0b..a4ff7ed44705 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -720,6 +720,7 @@ [Components.IA32] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index a7d747f6b4ab..3fb56b3f9ff9 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -719,6 +719,7 @@ [Components] > > !if $(TPM_ENABLE) == TRUE > OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > + OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > SecurityPkg/Tcg/TcgPei/TcgPei.inf > SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf { > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf > index c0098502aa90..ab58a9c0b4da 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.fdf > +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf > @@ -148,6 +148,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index f400c845b9c9..fc0ae1f280df 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -163,6 +163,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index d055552fd09f..306fc5a9b60d 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -163,6 +163,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index d519f8532822..22c8664427d6 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -175,6 +175,7 @@ [FV.PEIFV] > > !if $(TPM_ENABLE) == TRUE > INF OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +INF OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > INF SecurityPkg/Tcg/TcgPei/TcgPei.inf > INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf > !endif > diff --git a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > index 6776ec931ce0..39d1deeed16b 100644 > --- a/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > +++ b/OvmfPkg/Tcg/Tcg2Config/Tcg2ConfigPei.inf > @@ -57,7 +57,7 @@ [Pcd] > gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid ## PRODUCES > > [Depex.IA32, Depex.X64] > - TRUE > + gOvmfTpmMmioAccessiblePpiGuid > > [Depex.ARM, Depex.AARCH64] > gOvmfTpmDiscoveredPpiGuid > diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > new file mode 100644 > index 000000000000..926113b8ffb0 > --- /dev/null > +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf > @@ -0,0 +1,40 @@ > +## @file > +# Map TPM MMIO range unencrypted when SEV is active (6) Please add another sentence here: "Install gOvmfTpmMmioAccessiblePpiGuid unconditionally". > +# > +# Copyright (C) 2021, Advanced Micro Devices, Inc. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION = 0x00010005 (7) The latest INF spec version is 1.29: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Draft-Specification https://tianocore-docs.github.io/edk2-InfSpecification/draft/ plus INF_VERSION no longer requires a binary-only (hex-only) format. So please just write "1.29". > + BASE_NAME = TpmMmioSevDecryptPei > + FILE_GUID = F12F698A-E506-4A1B-B32E-6920E55DA1C4 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + ENTRY_POINT = TpmMmioSevDecryptPeimEntryPoint > + > +[Sources] > + TpmMmioSevDecryptPeim.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + SecurityPkg/SecurityPkg.dec (8) Is MdeModulePkg necessary? > + > +[LibraryClasses] > + BaseLib > + DebugLib > + MemEncryptSevLib > + PeimEntryPoint > + PeiServicesLib > + > +[Ppis] > + gOvmfTpmMmioAccessiblePpiGuid ## PRODUCES > + > +[FixedPcd] > + gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress ## CONSUMES > + > +[Depex] > + gEfiPeiMemoryDiscoveredPpiGuid > diff --git a/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c > new file mode 100644 > index 000000000000..dd1f1a80b5b0 > --- /dev/null > +++ b/OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPeim.c > @@ -0,0 +1,76 @@ > +/** @file > + Map TPM MMIO range unencrypted when SEV is active (9) Same request as (6) -- please add another sentence: "Install gOvmfTpmMmioAccessiblePpiGuid unconditionally". > + > + Copyright (C) 2021, Advanced Micro Devices, Inc. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > + > +#include > + > +#include > +#include > +#include (10) This Library #include list does not match the [LibraryClasses] section of the INF file (the PeimEntryPoint class apart, which should indeed only be in the INF file). In other words, BaseLib appears superfluous in the INF file. > + > +STATIC CONST EFI_PEI_PPI_DESCRIPTOR mTpmMmioRangeAccessible = { > + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gOvmfTpmMmioAccessiblePpiGuid, > + NULL > +}; > + > +/** > + The entry point for TPM MMIO range mapping driver. > + > + @param[in] FileHandle Handle of the file being invoked. > + @param[in] PeiServices Describes the list of possible PEI Services. > + > + @retval EFI_ABORTED No need to keep this PEIM resident > +**/ > +EFI_STATUS > +EFIAPI > +TpmMmioSevDecryptPeimEntryPoint ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + RETURN_STATUS DecryptStatus; > + EFI_STATUS Status; > + > + DEBUG ((DEBUG_INFO, "%a\n", __FUNCTION__)); > + > + // > + // 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. > + // Lovely comment, thanks! > + if (MemEncryptSevEsIsEnabled ()) { > + DEBUG ((DEBUG_INFO, "%a: mapping TPM MMIO address range unencrypted\n", __FUNCTION__)); > + > + DecryptStatus = MemEncryptSevClearPageEncMask ( > + 0, > + PcdGet64 (PcdTpmBaseAddress), (11) The INF file says [FixedPcd], so it would be cleanest to say FixedPcdGet64() here. (12) PcdLib is missing from both the [LibraryClasses] section and the #include directives. > + EFI_SIZE_TO_PAGES ((UINTN) 0x5000), > + FALSE > + ); > + > + if (RETURN_ERROR (DecryptStatus)) { > + DEBUG ((DEBUG_INFO, "%a: failed to map TPM MMIO address range unencrypted\n", __FUNCTION__)); (13) Overlong line. (14) Please report errors with DEBUG_ERROR. > + ASSERT_RETURN_ERROR (DecryptStatus); > + } > + } > + > + // > + // MMIO range available > + // > + Status = PeiServicesInstallPpi (&mTpmMmioRangeAccessible); > + ASSERT_EFI_ERROR (Status); > + > + return EFI_ABORTED; > +} > Thanks! Laszlo