Re: [edk2-devel] [PATCH 5/5] OvmfPkg: improve SMM comms security with adaptive MemoryTypeInformation

["Leif Lindholm" <leif@nuviainc.com>]

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 <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=386
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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 <Guid/MemoryTypeInformation.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Ppi/ReadOnlyVariable2.h>
> +#include <Uefi/UefiMultiPhase.h>
> +
> +#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 <Library/QemuFwCfgLib.h>
>  #include <Library/QemuFwCfgS3Lib.h>
>  #include <Library/ResourcePublicationLib.h>
> -#include <Guid/MemoryTypeInformation.h>
>  #include <Ppi/MasterBootMode.h>
>  #include <IndustryStandard/I440FxPiix4.h>
>  #include <IndustryStandard/Pci22.h>
> @@ -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
> 
> 
> 
>