Re: [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Jiewen,

Some comments included below.

I see you have added SortMemoryMap() from MdeModulePkg\Core\Dxe\Misc\PropertiesTable.c.
That same file has a function called MergeMemoryMap() that produces a smaller 
version of the memory map merging memory map entries together.  If you added 
a modified version of that function that merges the contiguous regions with 
the memory types this patch wants to set as not present, then you could have a
simpler loop on the smaller merged memory to set the policy and the check for
forbidden address would loop on a smaller number of memory map entries.

Best regards,

Mike

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jiewen Yao
> Sent: Wednesday, December 7, 2016 4:54 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Fan, Jeff <jeff.fan@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
> 
> This patch sets the normal OS buffer EfiLoaderCode/Data,
> EfiBootServicesCode/Data, EfiConventionalMemory, EfiACPIReclaimMemory
> to be not present after SmmReadyToLock.
> 
> To access these region in OS runtime phase is not a good solution.
> 
> Previously, we did similar check in SmmMemLib to help SMI handler
> do the check. But if SMI handler forgets the check, it can still
> access these OS region and bring risk.
> 
> So here we enforce the policy to prevent it happening.
> 
> Cc: Jeff Fan <jeff.fan@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |   7 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         |  23 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  29 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 248 ++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            |   7 +
>  5 files changed, 302 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index ba79477..c1f4b7e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -149,6 +149,13 @@ SmiPFHandler (
>        );
>        CpuDeadLoop ();
>      }
> +    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
> +      DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%x)!\n",
> PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
> +      );
> +      CpuDeadLoop ();
> +    }
>    }
> 
>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 4bef60a..fc7714a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -504,6 +504,11 @@ SmmReadyToLockEventNotify (
>    GetAcpiCpuData ();
> 
>    //
> +  // Cache a copy of UEFI memory map before we start profiling feature.
> +  //
> +  GetUefiMemoryMap ();
> +
> +  //
>    // Set SMM ready to lock flag and return
>    //
>    mSmmReadyToLock = TRUE;
> @@ -1154,17 +1159,6 @@ ConfigSmmCodeAccessCheck (
>  }
> 
>  /**
> -  Set code region to be read only and data region to be execute disable.
> -**/
> -VOID
> -SetRegionAttributes (
> -  VOID
> -  )
> -{
> -  SetMemMapAttributes ();
> -}
> -
> -/**
>    This API provides a way to allocate memory for page table.
> 
>    This API can be called more once to allocate memory for page tables.
> @@ -1320,7 +1314,12 @@ PerformRemainingTasks (
>      //
>      // Mark critical region to be read-only in page table
>      //
> -    SetRegionAttributes ();
> +    SetMemMapAttributes ();
> +
> +    //
> +    // For outside SMRAM, we only map SMM communication buffer or MMIO.
> +    //
> +    SetUefiMemMapAttributes ();
> 
>      //
>      // Set page table itself to be read-only
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 9160fa8..69c54fb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -839,6 +839,35 @@ SetMemMapAttributes (
>    );
> 
>  /**
> +  This function sets UEFI memory attribute according to UEFI memory map.
> +**/
> +VOID
> +SetUefiMemMapAttributes (
> +  VOID
> +  );
> +
> +/**
> +  Return if the Address is forbidden as SMM communication buffer.
> +
> +  @param[in] Address the address to be checked
> +
> +  @return TRUE  The address is forbidden as SMM communication buffer.
> +  @return FALSE The address is allowed as SMM communication buffer.
> +**/
> +BOOLEAN
> +IsSmmCommBufferForbiddenAddress (
> +  IN UINT64  Address
> +  );
> +
> +/**
> +  This function caches the UEFI memory map information.
> +**/
> +VOID
> +GetUefiMemoryMap (
> +  VOID
> +  );
> +
> +/**
>    This function sets memory attribute for page table.
>  **/
>  VOID
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 588aa27..9942c43 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -16,6 +16,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS
> OR IMPLIED.
>  #define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>    ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size)))
> 
> +EFI_MEMORY_DESCRIPTOR *mUefiMemoryMap;
> +UINTN                 mUefiMemoryMapSize;
> +UINTN                 mUefiDescriptorSize;
> +
>  PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>    {Page4K,  SIZE_4KB, PAGING_4K_ADDRESS_MASK_64},
>    {Page2M,  SIZE_2MB, PAGING_2M_ADDRESS_MASK_64},
> @@ -823,3 +827,247 @@ SetMemMapAttributes (
> 
>    return ;
>  }
> +
> +/**
> +  Sort memory map entries based upon PhysicalStart, from low to high.
> +
> +  @param  MemoryMap              A pointer to the buffer in which firmware places
> +                                 the current memory map.
> +  @param  MemoryMapSize          Size, in bytes, of the MemoryMap buffer.
> +  @param  DescriptorSize         Size, in bytes, of an individual
> EFI_MEMORY_DESCRIPTOR.
> +**/
> +STATIC
> +VOID
> +SortMemoryMap (
> +  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
> +  IN UINTN                      MemoryMapSize,
> +  IN UINTN                      DescriptorSize
> +  )
> +{
> +  EFI_MEMORY_DESCRIPTOR       *MemoryMapEntry;
> +  EFI_MEMORY_DESCRIPTOR       *NextMemoryMapEntry;
> +  EFI_MEMORY_DESCRIPTOR       *MemoryMapEnd;
> +  EFI_MEMORY_DESCRIPTOR       TempMemoryMap;
> +
> +  MemoryMapEntry = MemoryMap;
> +  NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> +  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
> +  while (MemoryMapEntry < MemoryMapEnd) {
> +    while (NextMemoryMapEntry < MemoryMapEnd) {
> +      if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry->PhysicalStart) {
> +        CopyMem (&TempMemoryMap, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
> +        CopyMem (MemoryMapEntry, NextMemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
> +        CopyMem (NextMemoryMapEntry, &TempMemoryMap, sizeof(EFI_MEMORY_DESCRIPTOR));
> +      }
> +
> +      NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry,
> DescriptorSize);
> +    }
> +
> +    MemoryMapEntry      = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> +    NextMemoryMapEntry  = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> +  }
> +
> +  return ;

Remove return statement at end of function.

> +}
> +
> +/**
> +  This function caches the UEFI memory map information.
> +**/
> +VOID
> +GetUefiMemoryMap (
> +  VOID
> +  )
> +{
> +  EFI_STATUS            Status;
> +  UINTN                 MapKey;
> +  UINT32                DescriptorVersion;
> +  EFI_MEMORY_DESCRIPTOR *MemoryMap;
> +
> +  DEBUG ((DEBUG_INFO, "GetUefiMemoryMap\n"));
> +
> +  mUefiMemoryMapSize = 0;
> +  MemoryMap = NULL;
> +  Status = gBS->GetMemoryMap (
> +                  &mUefiMemoryMapSize,
> +                  MemoryMap,
> +                  &MapKey,
> +                  &mUefiDescriptorSize,
> +                  &DescriptorVersion
> +                  );
> +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> +
> +  do {
> +    Status = gBS->AllocatePool (EfiBootServicesData, mUefiMemoryMapSize, (VOID
> **)&MemoryMap);
> +    ASSERT (MemoryMap != NULL);
> +    if (MemoryMap == NULL) {
> +      return ;
> +    }
> +
> +    Status = gBS->GetMemoryMap (
> +                    &mUefiMemoryMapSize,
> +                    MemoryMap,
> +                    &MapKey,
> +                    &mUefiDescriptorSize,
> +                    &DescriptorVersion
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      gBS->FreePool (MemoryMap);
> +      MemoryMap = NULL;
> +    }
> +  } while (Status == EFI_BUFFER_TOO_SMALL);
> +
> +  SortMemoryMap (MemoryMap, mUefiMemoryMapSize, mUefiDescriptorSize);
> +
> +  mUefiMemoryMap = AllocateCopyPool (mUefiMemoryMapSize, MemoryMap);

Why do you do another allocation here?  Couldn't you use AllocatePool/FreePool
library functions from SMM memory with mUefiMemoryMap in the logic above and
remove the use of the MemoryMap local variable?

> +  ASSERT (mUefiMemoryMap != NULL);
> +
> +  gBS->FreePool (MemoryMap);
> +
> +  return ;

Remove return statement at end of function.

> +}
> +
> +/**
> +  This function sets UEFI memory attribute according to UEFI memory map.
> +
> +  The normal memory region is marked as not present, such as
> +  EfiLoaderCode/Data, EfiBootServicesCode/Data, EfiConventionalMemory,
> +  EfiACPIReclaimMemory.
> +**/
> +VOID
> +SetUefiMemMapAttributes (
> +  VOID
> +  )
> +{
> +  EFI_MEMORY_DESCRIPTOR *MemoryMap;
> +  UINTN                 MemoryMapEntryCount;
> +  UINTN                 Index;
> +  UINT64                ProtectedMemoryBase;
> +  UINT64                ProtectedMemorySize;
> +
> +  DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
> +
> +  if (mUefiMemoryMap == NULL) {
> +    DEBUG ((DEBUG_INFO, "UefiMemoryMap - NULL\n"));
> +    return ;
> +  }
> +
> +  ProtectedMemoryBase = 0;
> +  ProtectedMemorySize = 0;
> +  MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> +  MemoryMap = mUefiMemoryMap;
> +  for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> +    switch (MemoryMap->Type) {
> +    case EfiLoaderCode:
> +    case EfiLoaderData:
> +    case EfiBootServicesCode:
> +    case EfiBootServicesData:
> +    case EfiConventionalMemory:
> +    case EfiUnusableMemory:
> +    case EfiACPIReclaimMemory:
> +      if (ProtectedMemoryBase + ProtectedMemorySize == MemoryMap->PhysicalStart) {
> +        //
> +        // This record can be merged to previous record
> +        //
> +        ProtectedMemorySize += EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages);
> +      } else {
> +        //
> +        // This record can not be merged to previous record.
> +        // We need set attribute
> +        //
> +        if (ProtectedMemorySize != 0) {
> +          DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n",
> ProtectedMemoryBase, ProtectedMemoryBase + ProtectedMemorySize));
> +          SmmSetMemoryAttributes (
> +            ProtectedMemoryBase,
> +            ProtectedMemorySize,
> +            EFI_MEMORY_RP
> +            );
> +        }
> +        //
> +        // then set a new ProtectedMemoryBase
> +        //
> +        ProtectedMemoryBase = MemoryMap->PhysicalStart;
> +        ProtectedMemorySize = EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages);
> +      }
> +      break;
> +    default:
> +      //
> +      // This record can not be merged to previous record.
> +      // We need set attribute
> +      //
> +      if (ProtectedMemorySize != 0) {
> +        DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n",
> ProtectedMemoryBase, ProtectedMemoryBase + ProtectedMemorySize));
> +        SmmSetMemoryAttributes (
> +          ProtectedMemoryBase,
> +          ProtectedMemorySize,
> +          EFI_MEMORY_RP
> +          );
> +      }
> +      //
> +      // then set a new ProtectedMemoryBase
> +      //
> +      ProtectedMemoryBase = MemoryMap->PhysicalStart +
> EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages);
> +      ProtectedMemorySize = 0;
> +      break;
> +    }
> +    MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
> +  }
> +
> +  //
> +  // We need set attribute for last item.
> +  //
> +  if (ProtectedMemorySize != 0) {
> +    DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n",
> ProtectedMemoryBase, ProtectedMemoryBase + ProtectedMemorySize));
> +    SmmSetMemoryAttributes (
> +      ProtectedMemoryBase,
> +      ProtectedMemorySize,
> +      EFI_MEMORY_RP
> +      );
> +  }
> +
> +  //
> +  // Do free mUefiMemoryMap�� it will be checked in
> IsSmmCommBufferForbiddenAddress().
> +  //

Is there code missing here to free mUefiMemoryMap, or is this an extra 
comment that should be removed?

> +
> +  return ;

Remove return statement at end of function.

> +}
> +
> +/**
> +  Return if the Address is forbidden as SMM communication buffer.
> +
> +  @param[in] Address the address to be checked
> +
> +  @return TRUE  The address is forbidden as SMM communication buffer.
> +  @return FALSE The address is allowed as SMM communication buffer.
> +**/
> +BOOLEAN
> +IsSmmCommBufferForbiddenAddress (
> +  IN UINT64  Address
> +  )
> +{
> +  EFI_MEMORY_DESCRIPTOR *MemoryMap;
> +  UINTN                 MemoryMapEntryCount;
> +  UINTN                 Index;
> +
> +  MemoryMap = mUefiMemoryMap;
> +  MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
> +  for (Index = 0; Index < MemoryMapEntryCount; Index++) {
> +    switch (MemoryMap->Type) {
> +    case EfiLoaderCode:
> +    case EfiLoaderData:
> +    case EfiBootServicesCode:
> +    case EfiBootServicesData:
> +    case EfiConventionalMemory:
> +    case EfiUnusableMemory:
> +    case EfiACPIReclaimMemory:
> +      if ((Address >= MemoryMap->PhysicalStart) &&
> +          (Address < MemoryMap->PhysicalStart + EFI_PAGES_TO_SIZE((UINTN)MemoryMap-
> >NumberOfPages)) ) {
> +        return TRUE;
> +      }
> +      break;
> +    default:
> +      break;
> +    }
> +    MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
> +  }
> +  return FALSE;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 7237e57..17b2f4c 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -867,6 +867,13 @@ SmiPFHandler (
>        );
>        CpuDeadLoop ();
>      }
> +    if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
> +      DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n",
> PFAddress));
> +      DEBUG_CODE (
> +        DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
> +      );
> +      CpuDeadLoop ();
> +    }
>    }
> 
>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
> --
> 2.7.4.windows.1