public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Fan, Jeff" <jeff.fan@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH] MdeModulePkg/PiSmmCore: MemoryAttributeTable need keep non-PE record.
Date: Wed, 7 Dec 2016 05:27:47 +0000	[thread overview]
Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2F5B2C@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <1480580607-19928-2-git-send-email-jiewen.yao@intel.com>

Reviewed-by: Jeff Fan <jeff.fan@intel.com>

-----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 <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>
---
 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 = &ImageRecord->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;
+        TotalNewRecordCount ++;
       }
       break;
     }
@@ -580,6 +583,8 @@ SplitRecord (
    ==>
    +---------------+
    | Record X      |
+   +---------------+
+   | Record RtCode |
    +---------------+ ----
    | Record RtData |     |
    +---------------+     |
@@ -587,12 +592,16 @@ SplitRecord (
    +---------------+     |
    | Record RtData |     |
    +---------------+ ----
+   | Record RtCode |
+   +---------------+ ----
    | Record RtData |     |
    +---------------+     |
    | Record RtCode |     |-> PE/COFF2
    +---------------+     |
    | Record RtData |     |
    +---------------+ ----
+   | Record RtCode |
+   +---------------+
    | Record Y      |
    +---------------+
 
@@ -622,7 +631,7 @@ SplitTable (
   UINTN       TotalSplitRecordCount;
   UINTN       AdditionalRecordCount;
 
-  AdditionalRecordCount = (2 * mImagePropertiesPrivateData.CodeSegmentCountMax + 1) * mImagePropertiesPrivateData.ImageRecordCount;
+  AdditionalRecordCount = (2 * 
+ mImagePropertiesPrivateData.CodeSegmentCountMax + 2) * 
+ mImagePropertiesPrivateData.ImageRecordCount;
 
   TotalSplitRecordCount = 0;
   //
@@ -648,11 +657,13 @@ SplitTable (
     //
     // Adjust IndexNew according to real split.
     //
-    CopyMem (
-      ((UINT8 *)MemoryMap + (IndexNew + MaxSplitRecordCount - RealSplitRecordCount) * DescriptorSize),
-      ((UINT8 *)MemoryMap + IndexNew * DescriptorSize),
-      RealSplitRecordCount * DescriptorSize
-      );
+    if (MaxSplitRecordCount != RealSplitRecordCount) {
+      CopyMem (
+        ((UINT8 *)MemoryMap + (IndexNew + MaxSplitRecordCount - RealSplitRecordCount) * DescriptorSize),
+        ((UINT8 *)MemoryMap + IndexNew * DescriptorSize),
+        (RealSplitRecordCount + 1) * DescriptorSize
+        );
+    }
     IndexNew = IndexNew + MaxSplitRecordCount - RealSplitRecordCount;
     TotalSplitRecordCount += RealSplitRecordCount;
     IndexNew --;
@@ -744,7 +755,7 @@ SmmCoreGetMemoryMapMemoryAttributesTable (
     return EFI_INVALID_PARAMETER;
   }
 
-  AdditionalRecordCount = (2 * mImagePropertiesPrivateData.CodeSegmentCountMax + 1) * mImagePropertiesPrivateData.ImageRecordCount;
+  AdditionalRecordCount = (2 * 
+ mImagePropertiesPrivateData.CodeSegmentCountMax + 2) * 
+ mImagePropertiesPrivateData.ImageRecordCount;
 
   OldMemoryMapSize = *MemoryMapSize;
   Status = SmmCoreGetMemoryMap (MemoryMapSize, MemoryMap, MapKey, DescriptorSize, DescriptorVersion);
--
2.7.4.windows.1



  reply	other threads:[~2016-12-07  5:28 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-01  8:23 [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType Jiewen Yao
2016-12-01  8:23 ` [PATCH] MdeModulePkg/PiSmmCore: MemoryAttributeTable need keep non-PE record Jiewen Yao
2016-12-07  5:27   ` Fan, Jeff [this message]
2016-12-01  8:23 ` [PATCH] MdeModulePkg/PiSmmCore; Use DEBUG_WARN for non 4k aligned image Jiewen Yao
2016-12-07  2:59   ` Fan, Jeff
2016-12-01  8:23 ` [PATCH] MdeModulePkg/PiSmmCore: use EfiPagesToSize to prevent build error Jiewen Yao
2016-12-07  3:02   ` Fan, Jeff
2016-12-01 21:51 ` [PATCH] MdeModulePkg/PiSmmCore: AllocatePool should use MemoryType Laszlo Ersek
2016-12-02  2:03   ` Yao, Jiewen
2016-12-02  9:44     ` Laszlo Ersek
2016-12-07  1:59 ` Fan, Jeff

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=542CF652F8836A4AB8DBFAAD40ED192A4A2F5B2C@shsmsx102.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox