[edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES
Laszlo Ersek
lersek at redhat.com
Wed Apr 28 17:51:49 UTC 2021
I'm going to ask for v3 after all:
On 04/27/21 18:21, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky at amd.com>
>
> 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 <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: Erdem Aktas <erdemaktas at google.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>
> Cc: Marc-Andr?? Lureau <marcandre.lureau at redhat.com>
(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 <stefanb at linux.ibm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
> ---
> 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 /
<https://bugzilla.tianocore.org/show_bug.cgi?id=3354>.
>
> 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 {
> <LibraryClasses>
(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 {
> <LibraryClasses>
> 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 {
> <LibraryClasses>
> 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 {
> <LibraryClasses>
> 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 <PiPei.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/PeiServicesLib.h>
(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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74572): https://edk2.groups.io/g/devel/message/74572
Mute This Topic: https://groups.io/mt/82407869/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