From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web12.11466.1583943778010396430 for ; Wed, 11 Mar 2020 09:22:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OPSxgGL+; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583943777; 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=p7hBkJh1RIbeWC/BE5pgeDG58y21eS6+WrGTEcFlDOg=; b=OPSxgGL+laJlp14GU9vT6mnTclWwdPZoVZKUbAyPERYgJtf5FER9dIt7dQLKKUt9SJn/CE YvgPgW6EIZnf3JOz+0uV2XobKvZl9OcCASbMAQZSXVQXNLdxC+5kZRkJXpZw2NSlMV3atY +GFjWu8LxsAJAI1DHgVCVkszuaXdmug= 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-377-lgh2aCP7O1iQbla7sgCiGQ-1; Wed, 11 Mar 2020 12:22:51 -0400 X-MC-Unique: lgh2aCP7O1iQbla7sgCiGQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D09C1DB62; Wed, 11 Mar 2020 16:22:49 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.119.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CAA58AC4D; Wed, 11 Mar 2020 16:22:48 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation To: Leif Lindholm , devel@edk2.groups.io Cc: Ard Biesheuvel , Jordan Justen , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20200310222739.26717-1-lersek@redhat.com> <20200310222739.26717-6-lersek@redhat.com> <20200311160040.GS23627@bivouac.eciton.net> From: "Laszlo Ersek" Message-ID: Date: Wed, 11 Mar 2020 17:22:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200311160040.GS23627@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/11/20 17:00, Leif Lindholm wrote: > On Tue, Mar 10, 2020 at 23:27:39 +0100, Laszlo Ersek wrote: >> * In the Intel whitepaper: >> >> --v-- >> A Tour Beyond BIOS -- Secure SMM Communication >> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Security-W= hite-Papers >> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_B= eyond_BIOS_Secure_SMM_Communication.pdf >> --^-- >> >> bullet#3 in section "Assumption and Recommendation", and bullet#4 in "C= all >> for action", recommend enabling the (adaptive) Memory Type Information >> feature. >> >> * In the Intel whitepaper: >> >> --v-- >> A Tour Beyond BIOS -- Memory Map and Practices in UEFI BIOS >> >> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-pape= rs >> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_B= eyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf >> --^-- >> >> figure#6 describes the Memory Type Information feature in detail; namel= y >> as a feedback loop between the Platform PEIM, the DXE IPL PEIM, the DXE >> Core, and BDS. >> >> Implement the missing PlatformPei functionality in OvmfPkg, for fulfill= ing >> the Secure SMM Communication recommendation. >> >> In the longer term, OVMF should install the WSMT ACPI table, and this >> patch contributes to that. >> >> Notes: >> >> - the step in figure#6 where the UEFI variable is copied into the HOB i= s >> covered by the DXE IPL PEIM, in the DxeLoadCore() function, >> >> - "PcdResetOnMemoryTypeInformationChange" must be reverted to the DEC >> default TRUE value, because both whitepapers indicate that BDS needs = to >> reset the system if the Memory Type Information changes. >> >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Philippe Mathieu-Daud=C3=A9 >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D386 >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/OvmfPkgIa32.dsc | 2 + >> OvmfPkg/OvmfPkgIa32X64.dsc | 2 + >> OvmfPkg/OvmfPkgX64.dsc | 2 + >> OvmfPkg/PlatformPei/PlatformPei.inf | 2 + >> OvmfPkg/PlatformPei/Platform.h | 5 + >> OvmfPkg/PlatformPei/MemTypeInfo.c | 147 ++++++++++++++++++++ >> OvmfPkg/PlatformPei/Platform.c | 23 +-- >> 7 files changed, 161 insertions(+), 22 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >> index 4c9ff419c462..02ca17db8b2a 100644 >> --- a/OvmfPkg/OvmfPkgIa32.dsc >> +++ b/OvmfPkg/OvmfPkgIa32.dsc >> @@ -448,7 +448,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) =3D=3D FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange= |FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >> index 56681e9e4580..d08cf558c6aa 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >> @@ -452,7 +452,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) =3D=3D FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange= |FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 0e74b6f4d26c..b2dccc40a865 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -452,7 +452,9 @@ [PcdsFeatureFlag] >> >> [PcdsFixedAtBuild] >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1 >> +!if $(SMM_REQUIRE) =3D=3D FALSE >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange= |FALSE >> +!endif >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 >> !if ($(FD_SIZE_IN_KB) =3D=3D 1024) || ($(FD_SIZE_IN_KB) =3D=3D 2048) >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000 >> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/= PlatformPei.inf >> index c51a6176aa2e..8531c63995c1 100644 >> --- a/OvmfPkg/PlatformPei/PlatformPei.inf >> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf >> @@ -30,6 +30,7 @@ [Sources] >> FeatureControl.c >> Fv.c >> MemDetect.c >> + MemTypeInfo.c >> Platform.c >> Platform.h >> Xen.c >> @@ -112,6 +113,7 @@ [FeaturePcd] >> [Ppis] >> gEfiPeiMasterBootModePpiGuid >> gEfiPeiMpServicesPpiGuid >> + gEfiPeiReadOnlyVariable2PpiGuid >> >> [Depex] >> TRUE >> diff --git a/OvmfPkg/PlatformPei/Platform.h b/OvmfPkg/PlatformPei/Platf= orm.h >> index 43f20f067f22..0484ec9e6b4c 100644 >> --- a/OvmfPkg/PlatformPei/Platform.h >> +++ b/OvmfPkg/PlatformPei/Platform.h >> @@ -82,6 +82,11 @@ PeiFvInitialization ( >> VOID >> ); >> >> +VOID >> +MemTypeInfoInitialization ( >> + VOID >> + ); >> + >> VOID >> InstallFeatureControlCallback ( >> VOID >> diff --git a/OvmfPkg/PlatformPei/MemTypeInfo.c b/OvmfPkg/PlatformPei/Me= mTypeInfo.c >> new file mode 100644 >> index 000000000000..c709236a457a >> --- /dev/null >> +++ b/OvmfPkg/PlatformPei/MemTypeInfo.c >> @@ -0,0 +1,147 @@ >> +/** @file >> + Produce a default memory type information HOB unless we can determin= e, from >> + the existence of the "MemoryTypeInformation" variable, that the DXE = IPL PEIM >> + will produce the HOB. >> + >> + Copyright (C) 2017-2020, Red Hat, Inc. >> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "Platform.h" >> + >> +STATIC EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] =3D= { >> + { EfiACPIMemoryNVS, 0x004 }, >> + { EfiACPIReclaimMemory, 0x008 }, >> + { EfiReservedMemoryType, 0x004 }, >> + { EfiRuntimeServicesData, 0x024 }, >> + { EfiRuntimeServicesCode, 0x030 }, >> + { EfiBootServicesCode, 0x180 }, >> + { EfiBootServicesData, 0xF00 }, >> + { EfiMaxMemoryType, 0x000 } >> +}; > > Could you add a comment as to where these page counts come from? > Oh, right, they're just moved across from OvmfPkg/PlatformPei/Platform.c= . > > It *would* be nice if that could be cleaned up at the same time... Sorry, I don't understand -- what kind of cleanup do you have in mind precisely? The table is not copied, but moved from the original place, so I'm unsure what's left in "Platform.c" to clean up. Regarding the origin of the table, it's ancient: - 49ba9447c92d ("Add initial version of Open Virtual Machine Firmware (OVMF) platform.", 2009-05-27) - 991d95636264 ("Update OVMF Platform PEI to declare IO and MMIO resources. Update default memory type information to reduce EFI Memory Map fragmentation.", 2010-07-16) - 55cdb67acb3d ("OVMF: Support greater than 2GB of memory", 2010-10-13) (BTW I have a patch as a separate work item in the queue for this -- I'm going to remove the EfiBootServicesCode / EfiBootServicesData entries from this table, per Jiewen's recommendation / explanation in the "WSMT bits" thread: https://edk2.groups.io/g/devel/message/55712 http://mid.mail-archive.com/a5e71131-65dc-8b85-481a-d35011512987@redhat.co= m But that's a separate patch, for later.) Thank you! Laszlo > > / > Leif > >> + >> +STATIC >> +VOID >> +BuildMemTypeInfoHob ( >> + VOID >> + ) >> +{ >> + BuildGuidDataHob ( >> + &gEfiMemoryTypeInformationGuid, >> + mDefaultMemoryTypeInformation, >> + sizeof mDefaultMemoryTypeInformation >> + ); >> + DEBUG ((DEBUG_INFO, "%a: default memory type information HOB built\n= ", >> + __FUNCTION__)); >> +} >> + >> +/** >> + Notification function called when EFI_PEI_READ_ONLY_VARIABLE2_PPI be= comes >> + available. >> + >> + @param[in] PeiServices Indirect reference to the PEI Services T= able. >> + @param[in] NotifyDescriptor Address of the notification descriptor d= ata >> + structure. >> + @param[in] Ppi Address of the PPI that was installed. >> + >> + @return Status of the notification. The status code returned from t= his >> + function is ignored. >> +**/ >> +STATIC >> +EFI_STATUS >> +EFIAPI >> +OnReadOnlyVariable2Available ( >> + IN EFI_PEI_SERVICES **PeiServices, >> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor, >> + IN VOID *Ppi >> + ) >> +{ >> + EFI_PEI_READ_ONLY_VARIABLE2_PPI *ReadOnlyVariable2; >> + UINTN DataSize; >> + EFI_STATUS Status; >> + >> + DEBUG ((DEBUG_VERBOSE, "%a\n", __FUNCTION__)); >> + >> + // >> + // Check if the "MemoryTypeInformation" variable exists, in the >> + // gEfiMemoryTypeInformationGuid namespace. >> + // >> + ReadOnlyVariable2 =3D Ppi; >> + DataSize =3D 0; >> + Status =3D ReadOnlyVariable2->GetVariable ( >> + ReadOnlyVariable2, >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_N= AME, >> + &gEfiMemoryTypeInformationGuid, >> + NULL, >> + &DataSize, >> + NULL >> + ); >> + switch (Status) { >> + case EFI_BUFFER_TOO_SMALL: >> + // >> + // The variable exists; the DXE IPL PEIM will build the HOB from i= t. >> + // >> + break; >> + case EFI_NOT_FOUND: >> + // >> + // The variable does not exist; install the default memory type in= formation >> + // HOB. >> + // >> + BuildMemTypeInfoHob (); >> + break; >> + default: >> + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNC= TION__, >> + Status)); >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + break; >> + } >> + >> + return EFI_SUCCESS; >> +} >> + >> +// >> +// Notification object for registering the callback, for when >> +// EFI_PEI_READ_ONLY_VARIABLE2_PPI becomes available. >> +// >> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mReadOnlyVariable2Notify =3D { >> + (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | >> + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), // Flags >> + &gEfiPeiReadOnlyVariable2PpiGuid, // Guid >> + OnReadOnlyVariable2Available // Notify >> +}; >> + >> +VOID >> +MemTypeInfoInitialization ( >> + VOID >> + ) >> +{ >> + EFI_STATUS Status; >> + >> + if (!FeaturePcdGet (PcdSmmSmramRequire)) { >> + // >> + // EFI_PEI_READ_ONLY_VARIABLE2_PPI will never be available; instal= l >> + // the default memory type information HOB right away. >> + // >> + BuildMemTypeInfoHob (); >> + return; >> + } >> + >> + Status =3D PeiServicesNotifyPpi (&mReadOnlyVariable2Notify); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_ERROR, "%a: failed to set up R/O Variable 2 callback= : %r\n", >> + __FUNCTION__, Status)); >> + ASSERT (FALSE); >> + CpuDeadLoop (); >> + } >> +} >> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platf= orm.c >> index 473af102783a..587ca68fc210 100644 >> --- a/OvmfPkg/PlatformPei/Platform.c >> +++ b/OvmfPkg/PlatformPei/Platform.c >> @@ -28,7 +28,6 @@ >> #include >> #include >> #include >> -#include >> #include >> #include >> #include >> @@ -39,18 +38,6 @@ >> #include "Platform.h" >> #include "Cmos.h" >> >> -EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] =3D { >> - { EfiACPIMemoryNVS, 0x004 }, >> - { EfiACPIReclaimMemory, 0x008 }, >> - { EfiReservedMemoryType, 0x004 }, >> - { EfiRuntimeServicesData, 0x024 }, >> - { EfiRuntimeServicesCode, 0x030 }, >> - { EfiBootServicesCode, 0x180 }, >> - { EfiBootServicesData, 0xF00 }, >> - { EfiMaxMemoryType, 0x000 } >> -}; >> - >> - >> EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] =3D { >> { >> EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST= , >> @@ -162,15 +149,6 @@ MemMapInitialization ( >> PciIoBase =3D 0xC000; >> PciIoSize =3D 0x4000; >> >> - // >> - // Create Memory Type Information HOB >> - // >> - BuildGuidDataHob ( >> - &gEfiMemoryTypeInformationGuid, >> - mDefaultMemoryTypeInformation, >> - sizeof(mDefaultMemoryTypeInformation) >> - ); >> - >> // >> // Video memory + Legacy BIOS region >> // >> @@ -811,6 +789,7 @@ InitializePlatform ( >> ReserveEmuVariableNvStore (); >> } >> PeiFvInitialization (); >> + MemTypeInfoInitialization (); >> MemMapInitialization (); >> NoexecDxeInitialization (); >> } >> -- >> 2.19.1.3.g30247aa5d201 >> >> >>=20 >> >