* [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
@ 2016-12-07 12:54 Jiewen Yao
2016-12-13 7:09 ` Fan, Jeff
2016-12-15 19:53 ` Kinney, Michael D
0 siblings, 2 replies; 4+ messages in thread
From: Jiewen Yao @ 2016-12-07 12:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jeff Fan, Michael D Kinney, Laszlo Ersek
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 12955 bytes --]
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 ;
+}
+
+/**
+ 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);
+ ASSERT (mUefiMemoryMap != NULL);
+
+ gBS->FreePool (MemoryMap);
+
+ return ;
+}
+
+/**
+ 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().
+ //
+
+ return ;
+}
+
+/**
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
2016-12-07 12:54 [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection Jiewen Yao
@ 2016-12-13 7:09 ` Fan, Jeff
2016-12-15 19:53 ` Kinney, Michael D
1 sibling, 0 replies; 4+ messages in thread
From: Fan, Jeff @ 2016-12-13 7:09 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Kinney, Michael D, Laszlo Ersek
Reviewed-by: Jeff Fan <jeff.fan@intel.com>
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jiewen Yao
Sent: Wednesday, December 07, 2016 8:54 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D; Laszlo Ersek; Fan, Jeff
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 ;
+}
+
+/**
+ 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);
+ ASSERT (mUefiMemoryMap != NULL);
+
+ gBS->FreePool (MemoryMap);
+
+ return ;
+}
+
+/**
+ 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().
+ //
+
+ return ;
+}
+
+/**
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
2016-12-07 12:54 [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection Jiewen Yao
2016-12-13 7:09 ` Fan, Jeff
@ 2016-12-15 19:53 ` Kinney, Michael D
2016-12-16 13:44 ` Yao, Jiewen
1 sibling, 1 reply; 4+ messages in thread
From: Kinney, Michael D @ 2016-12-15 19:53 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Laszlo Ersek, Fan, Jeff
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
2016-12-15 19:53 ` Kinney, Michael D
@ 2016-12-16 13:44 ` Yao, Jiewen
0 siblings, 0 replies; 4+ messages in thread
From: Yao, Jiewen @ 2016-12-16 13:44 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Fan, Jeff
Yes. Using MergeMemoryMap is a good idea.
I will send V2 patch.
From: Kinney, Michael D
Sent: Friday, December 16, 2016 3:54 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: RE: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection.
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<mailto:edk2-devel@lists.01.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>;
> Fan, Jeff <jeff.fan@intel.com<mailto: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<mailto:jeff.fan@intel.com>>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto: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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-16 13:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-07 12:54 [PATCH] UefiCpuPkg/PiSmmCpu: Add SMM Comm Buffer Paging Protection Jiewen Yao
2016-12-13 7:09 ` Fan, Jeff
2016-12-15 19:53 ` Kinney, Michael D
2016-12-16 13:44 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox