From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by mx.groups.io with SMTP id smtpd.web12.10915.1583942444823640546 for ; Wed, 11 Mar 2020 09:00:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=C0btJgjK; spf=pass (domain: nuviainc.com, ip: 209.85.128.54, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f54.google.com with SMTP id e26so2729810wme.5 for ; Wed, 11 Mar 2020 09:00:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=7tmcW8v+bWfg2x9SKRetVb/pQ5vt7onPomyhFalYobM=; b=C0btJgjKVAdv7DRT1Jc5XRvRTSyLuAwZtDWDuV+NVDNHzo+O1SI8a0GivgqoCpxFh5 ltoVPBZWoGo4zrXwWEq4lvBqYjWtqZpsZ+Mkc3/Ev0JE68krZWL5LE8JlMd7ROrESxBf NDmY9mVCzoA8sZ5yxxvSGQzelLzbQAalSMa8fXswrWkyBwEA+uWox/kcUb0MNHpVHUOl j2erApWPO7SFGWc+1ns5k/VS8wkq6rud92L7hlCrBLww8FS5dZEaIt27rjjNL7s+8lQn SnT0KTGD7WsrTpz8G5dP0X0L+Pw8fOPStnAjgkkUr9Nl7B1M1ZjPKVDl/LSdaE+wuskL 3nfw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7tmcW8v+bWfg2x9SKRetVb/pQ5vt7onPomyhFalYobM=; b=X2/7YthAP79eRzZEv7dEUhSp2D8PN9a7OlBY4+5huqFQ3wH0wlk9bVqIc6JaAWirov yEvZsYorqJzegdaAdOCLmzW4WN0xW1g09UUX7U2T1i5rn411GVBTjQx3fN7Ma4Augk3G y5x4wxpAclZqD9sV6xywf/WD5W/Ze3JjTYXC+OUAMChbSGOgOwzBdQ2C/Ac/5M9LSFu9 qgSG/C5ntybLIJVUaF3+il2yec18OuvsDlBEXl3G9VfXAiI4pJ31SaMPWOUw7sOmpWoE lPdoCoMHz/O47pPWhv4G1+PUKjZ+y3A0SmzUbyXcjkYPhcsc8apnoYhmyPAGvJdKeEKW haXA== X-Gm-Message-State: ANhLgQ0RZEXmZ5cehG+pBem4anMUbqrwBZfWXKbRbGZ/Lc73DB+rtzGO lD5h9FdUnnlj5OeSkDysTVl61aP8TsDyVhlmiOXkbHo5CMBnoZXDN701QOiJFH71l2DVAkP7jhd 6Og5qwV4tRfv+JqJovMK6muy6mCnVQyKMk+5kREvf/Ey1GGd9E6GYvw899NvjSes= X-Google-Smtp-Source: ADFU+vuk4R+DWiY8iMMIF+uUS5jIq7S3jOKNf6T8YmrKYTUrcxZJSEQ1uSkqaz7F8a0qyjZKGOPxqw== X-Received: by 2002:a1c:a102:: with SMTP id k2mr4233685wme.125.1583942442626; Wed, 11 Mar 2020 09:00:42 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v16sm54651792wrp.84.2020.03.11.09.00.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Mar 2020 09:00:41 -0700 (PDT) Date: Wed, 11 Mar 2020 16:00:40 +0000 From: "Leif Lindholm" To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Jordan Justen , Philippe =?iso-8859-1?Q?Mathieu-Daud=E9?= Subject: Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation Message-ID: <20200311160040.GS23627@bivouac.eciton.net> References: <20200310222739.26717-1-lersek@redhat.com> <20200310222739.26717-6-lersek@redhat.com> MIME-Version: 1.0 In-Reply-To: <20200310222739.26717-6-lersek@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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-White-Papers > https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Secure_SMM_Communication.pdf > --^-- > > bullet#3 in section "Assumption and Recommendation", and bullet#4 in "Call > 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-papers > https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf > --^-- > > figure#6 describes the Memory Type Information feature in detail; namely > 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 fulfilling > 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 is > 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é > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386 > 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) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 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) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 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) == FALSE > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE > +!endif > gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10 > !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 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/Platform.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/MemTypeInfo.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 determine, 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[] = { > + { 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... / 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 becomes > + available. > + > + @param[in] PeiServices Indirect reference to the PEI Services Table. > + @param[in] NotifyDescriptor Address of the notification descriptor data > + structure. > + @param[in] Ppi Address of the PPI that was installed. > + > + @return Status of the notification. The status code returned from this > + 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 = Ppi; > + DataSize = 0; > + Status = ReadOnlyVariable2->GetVariable ( > + ReadOnlyVariable2, > + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > + &gEfiMemoryTypeInformationGuid, > + NULL, > + &DataSize, > + NULL > + ); > + switch (Status) { > + case EFI_BUFFER_TOO_SMALL: > + // > + // The variable exists; the DXE IPL PEIM will build the HOB from it. > + // > + break; > + case EFI_NOT_FOUND: > + // > + // The variable does not exist; install the default memory type information > + // HOB. > + // > + BuildMemTypeInfoHob (); > + break; > + default: > + DEBUG ((DEBUG_ERROR, "%a: unexpected: GetVariable(): %r\n", __FUNCTION__, > + 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 = { > + (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; install > + // the default memory type information HOB right away. > + // > + BuildMemTypeInfoHob (); > + return; > + } > + > + Status = 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/Platform.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[] = { > - { EfiACPIMemoryNVS, 0x004 }, > - { EfiACPIReclaimMemory, 0x008 }, > - { EfiReservedMemoryType, 0x004 }, > - { EfiRuntimeServicesData, 0x024 }, > - { EfiRuntimeServicesCode, 0x030 }, > - { EfiBootServicesCode, 0x180 }, > - { EfiBootServicesData, 0xF00 }, > - { EfiMaxMemoryType, 0x000 } > -}; > - > - > EFI_PEI_PPI_DESCRIPTOR mPpiBootMode[] = { > { > EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > @@ -162,15 +149,6 @@ MemMapInitialization ( > PciIoBase = 0xC000; > PciIoSize = 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 > > > >