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

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

On Wed, Mar 11, 2020 at 17:22:47 +0100, Laszlo Ersek wrote:
> >> +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...
> 
> 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.

Not left to clean up there, but something to consider addressing when
moving it here. Yes, it's just a move, so we could argue it doesn't
need fixing - but it's a struct with a bunch of live-coded numerical
values completely without explanation.

"I'd rather not" is an acceptable answer, but I figured I should point
it out.

/
    Leif

> 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.com
> 
> 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 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
> >>
> >>
> >> 
> >>
> >
>