[edk2-devel] [PATCH v2 4/4] OvmfPkg/Tcg2ConfigPei: Mark TPM MMIO range as unencrypted for SEV-ES
Lendacky, Thomas
thomas.lendacky at amd.com
Wed Apr 28 19:43:09 UTC 2021
On 4/28/21 12:51 PM, Laszlo Ersek via groups.io wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3345&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=G1GwQc6sZqRuNHWC5vbdb78gCOl4YkAq%2BHi0F0ceucg%3D&reserved=0
>>
>> 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.
Will do.
>
>
>>
>> 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.
Ok, I'll split that out.
>
>
> (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).
I'll do it as 5 patches.
>
>
>>
>> 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.
We were having problems with sending patches via git-format-patch and
git-send-email and our email system. Likely some left over .gitconfig
entries that are causing the problem. I'll double check everything.
>
>
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3354&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HPF7LTIV17CwZ%2BCFehXpnPb%2BtQCgpFPvLsVqfNj9HBI%3D&reserved=0>.
>
>>
>> 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.)
Ok, I was going with the alphabetical placement. I'll switch it up.
>
>
>> 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".
Will do.
>
>
>> +#
>> +# 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Draft-Specification&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AXuQkvUSwLjEyZiwivQQaUwTaY7Mo0wLSHUf8QKNLC8%3D&reserved=0
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftianocore-docs.github.io%2Fedk2-InfSpecification%2Fdraft%2F&data=04%7C01%7Cthomas.lendacky%40amd.com%7C3c65ebfe044e4f3eb5b808d90a6e5455%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637552291252644310%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YUHs6g5aMPWBjCNjcZPKnTSEs2gBazDX094nqj9qpnE%3D&reserved=0
>
> plus INF_VERSION no longer requires a binary-only (hex-only) format. So
> please just write "1.29".
Will do.
>
>
>> + 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?
I don't think so. Let me double check it.
>
>
>> +
>> +[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".
>
Will do.
>
>> +
>> + 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.
Ok, let me check on that and fix as appropriate.
>
>
>> +
>> +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.
Will do.
>
>
> (12) PcdLib is missing from both the [LibraryClasses] section and the
> #include directives.
Right, I'll update that.
>
>
>> + 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.
Ok, I'll change that. I though that was ok now since PatchCheck.py didn't
complain.
>
>
> (14) Please report errors with DEBUG_ERROR.
Yup, will change.
Thanks,
Tom
>
>
>> + 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 (#74576): https://edk2.groups.io/g/devel/message/74576
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