Re: [edk2] [PATCH] MdeModulePkg/PiSmmCore: MemoryAttributeTable need keep non-PE record.

2016-12-06 Thread Fan, Jeff 
Reviewed-by: Jeff Fan 

-Original Message-
From: Yao, Jiewen 
Sent: Thursday, December 01, 2016 4:23 PM
To: edk2-devel@lists.01.org
Cc: Fan, Jeff; Kinney, Michael D; Laszlo Ersek
Subject: [PATCH] MdeModulePkg/PiSmmCore: MemoryAttributeTable need keep non-PE 
record.

Current memory attribute table implementation will only mark PE code to be 
EfiRuntimeServicesCode, and mark rest to be EfiRuntimeServicesData.

However, there might be a case that a SMM code wants to allocate 
EfiRuntimeServicesCode explicitly to let page table protect this region to be 
read only. It is unsupported.

This patch enhances the current solution so that MemoryAttributeTable does not 
touch non PE image record.
Only the PE image region is forced to be EfiRuntimeServicesCode for code and 
EfiRuntimeServicesData for data.

Cc: Jeff Fan 
Cc: Michael D Kinney 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
---
 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 91 +++-
 1 file changed, 51 insertions(+), 40 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c 
b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
index 3a5a2c8..a6ab830 100644
--- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
@@ -268,15 +268,19 @@ EnforceMemoryMapAttribute (
   MemoryMapEntry = MemoryMap;
   MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + 
MemoryMapSize);
   while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
-switch (MemoryMapEntry->Type) {
-case EfiRuntimeServicesCode:
-  MemoryMapEntry->Attribute |= EFI_MEMORY_RO;
-  break;
-case EfiRuntimeServicesData:
-  MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
-  break;
+if (MemoryMapEntry->Attribute != 0) {
+  // It is PE image, the attribute is already set.
+} else {
+  switch (MemoryMapEntry->Type) {
+  case EfiRuntimeServicesCode:
+MemoryMapEntry->Attribute = EFI_MEMORY_RO;
+break;
+  case EfiRuntimeServicesData:
+  default:
+MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
+break;
+  }
 }
-
 MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
   }
 
@@ -358,6 +362,21 @@ SetNewRecord (
   PhysicalEnd = TempRecord.PhysicalStart + 
EfiPagesToSize(TempRecord.NumberOfPages);
   NewRecordCount = 0;
 
+  //
+  // Always create a new entry for non-PE image record  //  if 
+ (ImageRecord->ImageBase > TempRecord.PhysicalStart) {
+NewRecord->Type = TempRecord.Type;
+NewRecord->PhysicalStart = TempRecord.PhysicalStart;
+NewRecord->VirtualStart  = 0;
+NewRecord->NumberOfPages = EfiSizeToPages(ImageRecord->ImageBase - 
TempRecord.PhysicalStart);
+NewRecord->Attribute = TempRecord.Attribute;
+NewRecord = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
+NewRecordCount ++;
+TempRecord.PhysicalStart = ImageRecord->ImageBase;
+TempRecord.NumberOfPages = EfiSizeToPages(PhysicalEnd - 
+ TempRecord.PhysicalStart);  }
+
   ImageRecordCodeSectionList = >CodeSegmentList;
 
   ImageRecordCodeSectionLink = ImageRecordCodeSectionList->ForwardLink;
@@ -452,14 +471,10 @@ GetMaxSplitRecordCount (
 if (ImageRecord == NULL) {
   break;
 }
-SplitRecordCount += (2 * ImageRecord->CodeSegmentCount + 1);
+SplitRecordCount += (2 * ImageRecord->CodeSegmentCount + 2);
 PhysicalStart = ImageRecord->ImageBase + ImageRecord->ImageSize;
   } while ((ImageRecord != NULL) && (PhysicalStart < PhysicalEnd));
 
-  if (SplitRecordCount != 0) {
-SplitRecordCount--;
-  }
-
   return SplitRecordCount;
 }
 
@@ -516,28 +531,16 @@ SplitRecord (
   //
   // No more image covered by this range, stop
   //
-  if ((PhysicalEnd > PhysicalStart) && (ImageRecord != NULL)) {
+  if (PhysicalEnd > PhysicalStart) {
 //
-// If this is still address in this record, need record.
+// Always create a new entry for non-PE image record
 //
-NewRecord = PREVIOUS_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
-if (NewRecord->Type == EfiRuntimeServicesData) {
-  //
-  // Last record is DATA, just merge it.
-  //
-  NewRecord->NumberOfPages = EfiSizeToPages(PhysicalEnd - 
NewRecord->PhysicalStart);
-} else {
-  //
-  // Last record is CODE, create a new DATA entry.
-  //
-  NewRecord = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
-  NewRecord->Type = EfiRuntimeServicesData;
-  NewRecord->PhysicalStart = TempRecord.PhysicalStart;
-  NewRecord->VirtualStart  = 0;
-  NewRecord->NumberOfPages = TempRecord.NumberOfPages;
-  NewRecord->Attribute = TempRecord.Attribute | EFI_MEMORY_XP;
-  TotalNewRecordCount ++;

[edk2] [PATCH] MdeModulePkg/PiSmmCore: MemoryAttributeTable need keep non-PE record.

2016-12-01 Thread Jiewen Yao 
Current memory attribute table implementation will only mark PE code
to be EfiRuntimeServicesCode, and mark rest to be EfiRuntimeServicesData.

However, there might be a case that a SMM code wants to allocate
EfiRuntimeServicesCode explicitly to let page table protect this region
to be read only. It is unsupported.

This patch enhances the current solution so that MemoryAttributeTable
does not touch non PE image record.
Only the PE image region is forced to be EfiRuntimeServicesCode for
code and EfiRuntimeServicesData for data.

Cc: Jeff Fan 
Cc: Michael D Kinney 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
---
 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 91 +++-
 1 file changed, 51 insertions(+), 40 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c 
b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
index 3a5a2c8..a6ab830 100644
--- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
@@ -268,15 +268,19 @@ EnforceMemoryMapAttribute (
   MemoryMapEntry = MemoryMap;
   MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + 
MemoryMapSize);
   while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
-switch (MemoryMapEntry->Type) {
-case EfiRuntimeServicesCode:
-  MemoryMapEntry->Attribute |= EFI_MEMORY_RO;
-  break;
-case EfiRuntimeServicesData:
-  MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
-  break;
+if (MemoryMapEntry->Attribute != 0) {
+  // It is PE image, the attribute is already set.
+} else {
+  switch (MemoryMapEntry->Type) {
+  case EfiRuntimeServicesCode:
+MemoryMapEntry->Attribute = EFI_MEMORY_RO;
+break;
+  case EfiRuntimeServicesData:
+  default:
+MemoryMapEntry->Attribute |= EFI_MEMORY_XP;
+break;
+  }
 }
-
 MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
   }
 
@@ -358,6 +362,21 @@ SetNewRecord (
   PhysicalEnd = TempRecord.PhysicalStart + 
EfiPagesToSize(TempRecord.NumberOfPages);
   NewRecordCount = 0;
 
+  //
+  // Always create a new entry for non-PE image record
+  //
+  if (ImageRecord->ImageBase > TempRecord.PhysicalStart) {
+NewRecord->Type = TempRecord.Type;
+NewRecord->PhysicalStart = TempRecord.PhysicalStart;
+NewRecord->VirtualStart  = 0;
+NewRecord->NumberOfPages = EfiSizeToPages(ImageRecord->ImageBase - 
TempRecord.PhysicalStart);
+NewRecord->Attribute = TempRecord.Attribute;
+NewRecord = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
+NewRecordCount ++;
+TempRecord.PhysicalStart = ImageRecord->ImageBase;
+TempRecord.NumberOfPages = EfiSizeToPages(PhysicalEnd - 
TempRecord.PhysicalStart);
+  }
+
   ImageRecordCodeSectionList = >CodeSegmentList;
 
   ImageRecordCodeSectionLink = ImageRecordCodeSectionList->ForwardLink;
@@ -452,14 +471,10 @@ GetMaxSplitRecordCount (
 if (ImageRecord == NULL) {
   break;
 }
-SplitRecordCount += (2 * ImageRecord->CodeSegmentCount + 1);
+SplitRecordCount += (2 * ImageRecord->CodeSegmentCount + 2);
 PhysicalStart = ImageRecord->ImageBase + ImageRecord->ImageSize;
   } while ((ImageRecord != NULL) && (PhysicalStart < PhysicalEnd));
 
-  if (SplitRecordCount != 0) {
-SplitRecordCount--;
-  }
-
   return SplitRecordCount;
 }
 
@@ -516,28 +531,16 @@ SplitRecord (
   //
   // No more image covered by this range, stop
   //
-  if ((PhysicalEnd > PhysicalStart) && (ImageRecord != NULL)) {
+  if (PhysicalEnd > PhysicalStart) {
 //
-// If this is still address in this record, need record.
+// Always create a new entry for non-PE image record
 //
-NewRecord = PREVIOUS_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
-if (NewRecord->Type == EfiRuntimeServicesData) {
-  //
-  // Last record is DATA, just merge it.
-  //
-  NewRecord->NumberOfPages = EfiSizeToPages(PhysicalEnd - 
NewRecord->PhysicalStart);
-} else {
-  //
-  // Last record is CODE, create a new DATA entry.
-  //
-  NewRecord = NEXT_MEMORY_DESCRIPTOR (NewRecord, DescriptorSize);
-  NewRecord->Type = EfiRuntimeServicesData;
-  NewRecord->PhysicalStart = TempRecord.PhysicalStart;
-  NewRecord->VirtualStart  = 0;
-  NewRecord->NumberOfPages = TempRecord.NumberOfPages;
-  NewRecord->Attribute = TempRecord.Attribute | EFI_MEMORY_XP;
-  TotalNewRecordCount ++;
-}
+NewRecord->Type = TempRecord.Type;
+NewRecord->PhysicalStart = TempRecord.PhysicalStart;
+NewRecord->VirtualStart  = 0;
+NewRecord->NumberOfPages = TempRecord.NumberOfPages;
+NewRecord->Attribute = TempRecord.Attribute;
+