public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
@ 2017-08-24  3:20 Star Zeng
  2017-08-24  8:51 ` Yao, Jiewen
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Star Zeng @ 2017-08-24  3:20 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Laszlo Ersek, Eric Dong

https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports
memory protection crash in PiSmmCpuDxeSmm, Ia32 build with
RAM above 4GB (of which 2GB are placed in 64-bit address).
It is because UEFI builds identity mapping page tables,
>4G address is not supported at Ia32 build.

This patch is to get the PhysicalAddressBits that is used
to build in PageTbl.c(Ia32/X64), and use it to check whether
the address is supported or not in ConvertMemoryPageAttributes().

With this patch, the debug messages will be like below.
UefiMemory protection: 0x0 - 0x9F000 Success
UefiMemory protection: 0x100000 - 0x807000 Success
UefiMemory protection: 0x808000 - 0x810000 Success
UefiMemory protection: 0x818000 - 0x820000 Success
UefiMemory protection: 0x1510000 - 0x7B798000 Success
UefiMemory protection: 0x7B79B000 - 0x7E538000 Success
UefiMemory protection: 0x7E539000 - 0x7E545000 Success
UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success
UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success
UefiMemory protection: 0x7F703000 - 0x7F70B000 Success
UefiMemory protection: 0x7F70F000 - 0x7F778000 Success
UefiMemory protection: 0x100000000 - 0x180000000 Unsupported

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
Reported-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 32ce5958c59c..e88b42d73343 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 #include "PiSmmCpuDxeSmm.h"
 
+UINT8                               mPhysicalAddressBits;
+
 /**
   Create PageTable for SMM use.
 
@@ -36,6 +38,8 @@ SmmInitPageTable (
   //
   InitializeSpinLock (mPFLock);
 
+  mPhysicalAddressBits = 32;
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     //
     // Set own Page Fault entry instead of the default one, because SMM Profile
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index dbce9ec520fe..1cf85c1481a7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
 extern SPIN_LOCK                           *mMemoryMappedLock;
 extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
 extern UINTN                               mSmmCpuSmramRangeCount;
+extern UINT8                               mPhysicalAddressBits;
 
 //
 // Copy of the PcdPteMemoryEncryptionAddressOrMask
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index a535389c26ce..3ad5256f1e03 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1,6 +1,6 @@
 /** @file
 
-Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
   PAGE_ATTRIBUTE                    SplitAttribute;
   RETURN_STATUS                     Status;
   BOOLEAN                           IsEntryModified;
+  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
 
   ASSERT (Attributes != 0);
   ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
@@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
     return RETURN_INVALID_PARAMETER;
   }
 
+  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, mPhysicalAddressBits) - 1);
+  if (BaseAddress > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if (Length > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
+    return RETURN_UNSUPPORTED;
+  }
+
 //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
 
   if (IsSplitted != NULL) {
@@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
   VOID
   )
 {
+  EFI_STATUS            Status;
   EFI_MEMORY_DESCRIPTOR *MemoryMap;
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
@@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
   MemoryMap = mUefiMemoryMap;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
     if (IsUefiPageNotPresent(MemoryMap)) {
-      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
-      SmmSetMemoryAttributes (
+      Status = SmmSetMemoryAttributes (
+                 MemoryMap->PhysicalStart,
+                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+                 EFI_MEMORY_RP
+                 );
+      DEBUG ((
+        DEBUG_INFO,
+        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
         MemoryMap->PhysicalStart,
-        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
-        EFI_MEMORY_RP
-        );
+        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+        Status
+        ));
     }
     MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
   }
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-24  3:20 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash Star Zeng
@ 2017-08-24  8:51 ` Yao, Jiewen
  2017-08-24  9:02   ` Zeng, Star
  2017-08-24 10:21 ` Laszlo Ersek
  2017-08-25  9:49 ` Laszlo Ersek
  2 siblings, 1 reply; 11+ messages in thread
From: Yao, Jiewen @ 2017-08-24  8:51 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Dong, Eric, Zeng, Star

Reviewed-by: Jiewen.yao@intel.com

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star Zeng
Sent: Thursday, August 24, 2017 11:20 AM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports
memory protection crash in PiSmmCpuDxeSmm, Ia32 build with
RAM above 4GB (of which 2GB are placed in 64-bit address).
It is because UEFI builds identity mapping page tables,
>4G address is not supported at Ia32 build.

This patch is to get the PhysicalAddressBits that is used
to build in PageTbl.c(Ia32/X64), and use it to check whether
the address is supported or not in ConvertMemoryPageAttributes().

With this patch, the debug messages will be like below.
UefiMemory protection: 0x0 - 0x9F000 Success
UefiMemory protection: 0x100000 - 0x807000 Success
UefiMemory protection: 0x808000 - 0x810000 Success
UefiMemory protection: 0x818000 - 0x820000 Success
UefiMemory protection: 0x1510000 - 0x7B798000 Success
UefiMemory protection: 0x7B79B000 - 0x7E538000 Success
UefiMemory protection: 0x7E539000 - 0x7E545000 Success
UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success
UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success
UefiMemory protection: 0x7F703000 - 0x7F70B000 Success
UefiMemory protection: 0x7F70F000 - 0x7F778000 Success
UefiMemory protection: 0x100000000 - 0x180000000 Unsupported

Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Reported-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 32ce5958c59c..e88b42d73343 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

 #include "PiSmmCpuDxeSmm.h"

+UINT8                               mPhysicalAddressBits;
+
 /**
   Create PageTable for SMM use.

@@ -36,6 +38,8 @@ SmmInitPageTable (
   //
   InitializeSpinLock (mPFLock);

+  mPhysicalAddressBits = 32;
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     //
     // Set own Page Fault entry instead of the default one, because SMM Profile
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index dbce9ec520fe..1cf85c1481a7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
 extern SPIN_LOCK                           *mMemoryMappedLock;
 extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
 extern UINTN                               mSmmCpuSmramRangeCount;
+extern UINT8                               mPhysicalAddressBits;

 //
 // Copy of the PcdPteMemoryEncryptionAddressOrMask
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index a535389c26ce..3ad5256f1e03 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1,6 +1,6 @@
 /** @file

-Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
   PAGE_ATTRIBUTE                    SplitAttribute;
   RETURN_STATUS                     Status;
   BOOLEAN                           IsEntryModified;
+  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;

   ASSERT (Attributes != 0);
   ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
@@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
     return RETURN_INVALID_PARAMETER;
   }

+  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, mPhysicalAddressBits) - 1);
+  if (BaseAddress > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if (Length > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
+    return RETURN_UNSUPPORTED;
+  }
+
 //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));

   if (IsSplitted != NULL) {
@@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
   VOID
   )
 {
+  EFI_STATUS            Status;
   EFI_MEMORY_DESCRIPTOR *MemoryMap;
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
@@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
   MemoryMap = mUefiMemoryMap;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
     if (IsUefiPageNotPresent(MemoryMap)) {
-      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
-      SmmSetMemoryAttributes (
+      Status = SmmSetMemoryAttributes (
+                 MemoryMap->PhysicalStart,
+                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+                 EFI_MEMORY_RP
+                 );
+      DEBUG ((
+        DEBUG_INFO,
+        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
         MemoryMap->PhysicalStart,
-        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
-        EFI_MEMORY_RP
-        );
+        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+        Status
+        ));
     }
     MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
   }
--
2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-24  8:51 ` Yao, Jiewen
@ 2017-08-24  9:02   ` Zeng, Star
  0 siblings, 0 replies; 11+ messages in thread
From: Zeng, Star @ 2017-08-24  9:02 UTC (permalink / raw)
  To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Dong, Eric, Zeng, Star

Thanks for the Reviewed-by.
Sorry I did not check the whole thread at https://lists.01.org/pipermail/edk2-devel/2017-July/012283.html that has the discussion about the validity of BASE 0 + LENGTH 4G (Case1) and BASE 4G + LENGTH 0 (Case2).

Current code has filtered Case2.
  @retval RETURN_INVALID_PARAMETER Length is zero.
  if (Length == 0) {
    return RETURN_INVALID_PARAMETER;
  }

If we agree Case1 needs to be considered, I am ok to update the patch accordingly. :)


Thanks,
Star
From: Yao, Jiewen
Sent: Thursday, August 24, 2017 4:52 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

Reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Star Zeng
Sent: Thursday, August 24, 2017 11:20 AM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports
memory protection crash in PiSmmCpuDxeSmm, Ia32 build with
RAM above 4GB (of which 2GB are placed in 64-bit address).
It is because UEFI builds identity mapping page tables,
>4G address is not supported at Ia32 build.

This patch is to get the PhysicalAddressBits that is used
to build in PageTbl.c(Ia32/X64), and use it to check whether
the address is supported or not in ConvertMemoryPageAttributes().

With this patch, the debug messages will be like below.
UefiMemory protection: 0x0 - 0x9F000 Success
UefiMemory protection: 0x100000 - 0x807000 Success
UefiMemory protection: 0x808000 - 0x810000 Success
UefiMemory protection: 0x818000 - 0x820000 Success
UefiMemory protection: 0x1510000 - 0x7B798000 Success
UefiMemory protection: 0x7B79B000 - 0x7E538000 Success
UefiMemory protection: 0x7E539000 - 0x7E545000 Success
UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success
UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success
UefiMemory protection: 0x7F703000 - 0x7F70B000 Success
UefiMemory protection: 0x7F70F000 - 0x7F778000 Success
UefiMemory protection: 0x100000000 - 0x180000000 Unsupported

Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Reported-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++-----
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 32ce5958c59c..e88b42d73343 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

 #include "PiSmmCpuDxeSmm.h"

+UINT8                               mPhysicalAddressBits;
+
 /**
   Create PageTable for SMM use.

@@ -36,6 +38,8 @@ SmmInitPageTable (
   //
   InitializeSpinLock (mPFLock);

+  mPhysicalAddressBits = 32;
+
   if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
     //
     // Set own Page Fault entry instead of the default one, because SMM Profile
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index dbce9ec520fe..1cf85c1481a7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
 extern SPIN_LOCK                           *mMemoryMappedLock;
 extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
 extern UINTN                               mSmmCpuSmramRangeCount;
+extern UINT8                               mPhysicalAddressBits;

 //
 // Copy of the PcdPteMemoryEncryptionAddressOrMask
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index a535389c26ce..3ad5256f1e03 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1,6 +1,6 @@
 /** @file

-Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
   PAGE_ATTRIBUTE                    SplitAttribute;
   RETURN_STATUS                     Status;
   BOOLEAN                           IsEntryModified;
+  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;

   ASSERT (Attributes != 0);
   ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
@@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
     return RETURN_INVALID_PARAMETER;
   }

+  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, mPhysicalAddressBits) - 1);
+  if (BaseAddress > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if (Length > MaximumSupportMemAddress) {
+    return RETURN_UNSUPPORTED;
+  }
+  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
+    return RETURN_UNSUPPORTED;
+  }
+
 //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));

   if (IsSplitted != NULL) {
@@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
   VOID
   )
 {
+  EFI_STATUS            Status;
   EFI_MEMORY_DESCRIPTOR *MemoryMap;
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
@@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
   MemoryMap = mUefiMemoryMap;
   for (Index = 0; Index < MemoryMapEntryCount; Index++) {
     if (IsUefiPageNotPresent(MemoryMap)) {
-      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
-      SmmSetMemoryAttributes (
+      Status = SmmSetMemoryAttributes (
+                 MemoryMap->PhysicalStart,
+                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+                 EFI_MEMORY_RP
+                 );
+      DEBUG ((
+        DEBUG_INFO,
+        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
         MemoryMap->PhysicalStart,
-        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
-        EFI_MEMORY_RP
-        );
+        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+        Status
+        ));
     }
     MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
   }
--
2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-24  3:20 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash Star Zeng
  2017-08-24  8:51 ` Yao, Jiewen
@ 2017-08-24 10:21 ` Laszlo Ersek
  2017-08-24 10:28   ` Zeng, Star
  2017-08-25  9:49 ` Laszlo Ersek
  2 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-08-24 10:21 UTC (permalink / raw)
  To: Star Zeng, edk2-devel; +Cc: Jiewen Yao, Eric Dong

Hi,

On 08/24/17 05:20, Star Zeng wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports
> memory protection crash in PiSmmCpuDxeSmm, Ia32 build with
> RAM above 4GB (of which 2GB are placed in 64-bit address).
> It is because UEFI builds identity mapping page tables,
>> 4G address is not supported at Ia32 build.
> 
> This patch is to get the PhysicalAddressBits that is used
> to build in PageTbl.c(Ia32/X64), and use it to check whether
> the address is supported or not in ConvertMemoryPageAttributes().
> 
> With this patch, the debug messages will be like below.
> UefiMemory protection: 0x0 - 0x9F000 Success
> UefiMemory protection: 0x100000 - 0x807000 Success
> UefiMemory protection: 0x808000 - 0x810000 Success
> UefiMemory protection: 0x818000 - 0x820000 Success
> UefiMemory protection: 0x1510000 - 0x7B798000 Success
> UefiMemory protection: 0x7B79B000 - 0x7E538000 Success
> UefiMemory protection: 0x7E539000 - 0x7E545000 Success
> UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success
> UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success
> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success
> UefiMemory protection: 0x7F70F000 - 0x7F778000 Success
> UefiMemory protection: 0x100000000 - 0x180000000 Unsupported
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++-----
>  3 files changed, 30 insertions(+), 6 deletions(-)

I might need a few days before I can test this. Please continue
reviewing / discussing the patch (and if v2 is necessary, please CC me);
just please give me a chance to test it before merging it. I hope I can
test it beginning of next week at the latest.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-24 10:21 ` Laszlo Ersek
@ 2017-08-24 10:28   ` Zeng, Star
  0 siblings, 0 replies; 11+ messages in thread
From: Zeng, Star @ 2017-08-24 10:28 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star

I did test on OVMF for the case by myself. :)
Your double confirm will be wonderful.
I will wait for your test result.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, August 24, 2017 6:21 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

Hi,

On 08/24/17 05:20, Star Zeng wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
> which 2GB are placed in 64-bit address).
> It is because UEFI builds identity mapping page tables,
>> 4G address is not supported at Ia32 build.
> 
> This patch is to get the PhysicalAddressBits that is used to build in 
> PageTbl.c(Ia32/X64), and use it to check whether the address is 
> supported or not in ConvertMemoryPageAttributes().
> 
> With this patch, the debug messages will be like below.
> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 0x810000 
> Success UefiMemory protection: 0x818000 - 0x820000 Success UefiMemory 
> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 - 
> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory 
> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
> 0x100000000 - 0x180000000 Unsupported
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 
> +++++++++++++++++-----
>  3 files changed, 30 insertions(+), 6 deletions(-)

I might need a few days before I can test this. Please continue reviewing / discussing the patch (and if v2 is necessary, please CC me); just please give me a chance to test it before merging it. I hope I can test it beginning of next week at the latest.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-24  3:20 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash Star Zeng
  2017-08-24  8:51 ` Yao, Jiewen
  2017-08-24 10:21 ` Laszlo Ersek
@ 2017-08-25  9:49 ` Laszlo Ersek
  2017-08-25  9:53   ` Zeng, Star
  2 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-08-25  9:49 UTC (permalink / raw)
  To: Star Zeng, edk2-devel; +Cc: Jiewen Yao, Eric Dong

Star,

On 08/24/17 05:20, Star Zeng wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports
> memory protection crash in PiSmmCpuDxeSmm, Ia32 build with
> RAM above 4GB (of which 2GB are placed in 64-bit address).
> It is because UEFI builds identity mapping page tables,
>> 4G address is not supported at Ia32 build.
> 
> This patch is to get the PhysicalAddressBits that is used
> to build in PageTbl.c(Ia32/X64), and use it to check whether
> the address is supported or not in ConvertMemoryPageAttributes().
> 
> With this patch, the debug messages will be like below.
> UefiMemory protection: 0x0 - 0x9F000 Success
> UefiMemory protection: 0x100000 - 0x807000 Success
> UefiMemory protection: 0x808000 - 0x810000 Success
> UefiMemory protection: 0x818000 - 0x820000 Success
> UefiMemory protection: 0x1510000 - 0x7B798000 Success
> UefiMemory protection: 0x7B79B000 - 0x7E538000 Success
> UefiMemory protection: 0x7E539000 - 0x7E545000 Success
> UefiMemory protection: 0x7E55A000 - 0x7E61F000 Success
> UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success
> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success
> UefiMemory protection: 0x7F70F000 - 0x7F778000 Success
> UefiMemory protection: 0x100000000 - 0x180000000 Unsupported
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 +++++++++++++++++-----
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 32ce5958c59c..e88b42d73343 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #include "PiSmmCpuDxeSmm.h"
>  
> +UINT8                               mPhysicalAddressBits;
> +
>  /**
>    Create PageTable for SMM use.
>  
> @@ -36,6 +38,8 @@ SmmInitPageTable (
>    //
>    InitializeSpinLock (mPFLock);
>  
> +  mPhysicalAddressBits = 32;
> +
>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>      //
>      // Set own Page Fault entry instead of the default one, because SMM Profile
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index dbce9ec520fe..1cf85c1481a7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>  extern SPIN_LOCK                           *mMemoryMappedLock;
>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>  extern UINTN                               mSmmCpuSmramRangeCount;
> +extern UINT8                               mPhysicalAddressBits;
>  
>  //
>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index a535389c26ce..3ad5256f1e03 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -1,6 +1,6 @@
>  /** @file
>  
> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>    PAGE_ATTRIBUTE                    SplitAttribute;
>    RETURN_STATUS                     Status;
>    BOOLEAN                           IsEntryModified;
> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>  
>    ASSERT (Attributes != 0);
>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
> @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>      return RETURN_INVALID_PARAMETER;
>    }
>  
> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 (1, mPhysicalAddressBits) - 1);
> +  if (BaseAddress > MaximumSupportMemAddress) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  if (Length > MaximumSupportMemAddress) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +
>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>  
>    if (IsSplitted != NULL) {
> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>    VOID
>    )
>  {
> +  EFI_STATUS            Status;
>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>    UINTN                 MemoryMapEntryCount;
>    UINTN                 Index;
> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>    MemoryMap = mUefiMemoryMap;
>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>      if (IsUefiPageNotPresent(MemoryMap)) {
> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
> -      SmmSetMemoryAttributes (
> +      Status = SmmSetMemoryAttributes (
> +                 MemoryMap->PhysicalStart,
> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> +                 EFI_MEMORY_RP
> +                 );
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>          MemoryMap->PhysicalStart,
> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> -        EFI_MEMORY_RP
> -        );
> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> +        Status
> +        ));
>      }
>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>    }
> 

before applying this patch for local testing, I figured I'd look it over
quickly.

I think that you missed adding the X64 changes to the commit, with "git
add". Because, the "mPhysicalAddressBits" variable is declared in common
code, it is also consumed in common code, but it is only defined (i.e.,
allocated) and set in Ia32 code. I believe that applying this exact
patch would prevent PiSmmCpuDxeSmm even from linking.

I think for X64 you likely have a change similar to the Ia32 one
(defining the variable and setting it to the actual physical address
bits, likely from the CPU HOB), but it's not part of the patch.

If I'm right and you decide to post v2, then I suggest another (very
small) improvement: I think the definition (=allocation) of
"mPhysicalAddressBits" could also be moved to common code; only the
assignments have to be architecture-specific.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-25  9:49 ` Laszlo Ersek
@ 2017-08-25  9:53   ` Zeng, Star
  2017-08-25 11:52     ` Laszlo Ersek
  0 siblings, 1 reply; 11+ messages in thread
From: Zeng, Star @ 2017-08-25  9:53 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star

Laszlo,

X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.

  mPhysicalAddressBits = CalculateMaximumSupportAddress ();


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Friday, August 25, 2017 5:50 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

Star,

On 08/24/17 05:20, Star Zeng wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
> which 2GB are placed in 64-bit address).
> It is because UEFI builds identity mapping page tables,
>> 4G address is not supported at Ia32 build.
> 
> This patch is to get the PhysicalAddressBits that is used to build in 
> PageTbl.c(Ia32/X64), and use it to check whether the address is 
> supported or not in ConvertMemoryPageAttributes().
> 
> With this patch, the debug messages will be like below.
> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 0x810000 
> Success UefiMemory protection: 0x818000 - 0x820000 Success UefiMemory 
> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 - 
> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory 
> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
> 0x100000000 - 0x180000000 Unsupported
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 
> +++++++++++++++++-----
>  3 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> index 32ce5958c59c..e88b42d73343 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #include "PiSmmCpuDxeSmm.h"
>  
> +UINT8                               mPhysicalAddressBits;
> +
>  /**
>    Create PageTable for SMM use.
>  
> @@ -36,6 +38,8 @@ SmmInitPageTable (
>    //
>    InitializeSpinLock (mPFLock);
>  
> +  mPhysicalAddressBits = 32;
> +
>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>      //
>      // Set own Page Fault entry instead of the default one, because 
> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index dbce9ec520fe..1cf85c1481a7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>  extern SPIN_LOCK                           *mMemoryMappedLock;
>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>  extern UINTN                               mSmmCpuSmramRangeCount;
> +extern UINT8                               mPhysicalAddressBits;
>  
>  //
>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index a535389c26ce..3ad5256f1e03 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -1,6 +1,6 @@
>  /** @file
>  
> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
> +reserved.<BR>
>  This program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License  which 
> accompanies this distribution.  The full text of the license may be 
> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>    PAGE_ATTRIBUTE                    SplitAttribute;
>    RETURN_STATUS                     Status;
>    BOOLEAN                           IsEntryModified;
> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>  
>    ASSERT (Attributes != 0);
>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | 
> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>      return RETURN_INVALID_PARAMETER;
>    }
>  
> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 
> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  if (Length > MaximumSupportMemAddress) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
> +    return RETURN_UNSUPPORTED;
> +  }
> +
>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>  
>    if (IsSplitted != NULL) {
> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>    VOID
>    )
>  {
> +  EFI_STATUS            Status;
>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>    UINTN                 MemoryMapEntryCount;
>    UINTN                 Index;
> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>    MemoryMap = mUefiMemoryMap;
>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>      if (IsUefiPageNotPresent(MemoryMap)) {
> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
> -      SmmSetMemoryAttributes (
> +      Status = SmmSetMemoryAttributes (
> +                 MemoryMap->PhysicalStart,
> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> +                 EFI_MEMORY_RP
> +                 );
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>          MemoryMap->PhysicalStart,
> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> -        EFI_MEMORY_RP
> -        );
> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
> +        Status
> +        ));
>      }
>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>    }
> 

before applying this patch for local testing, I figured I'd look it over quickly.

I think that you missed adding the X64 changes to the commit, with "git add". Because, the "mPhysicalAddressBits" variable is declared in common code, it is also consumed in common code, but it is only defined (i.e.,
allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.

I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.

If I'm right and you decide to post v2, then I suggest another (very
small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-25  9:53   ` Zeng, Star
@ 2017-08-25 11:52     ` Laszlo Ersek
  2017-08-26  2:24       ` Zeng, Star
  0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-08-25 11:52 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric

On 08/25/17 11:53, Zeng, Star wrote:
> Laszlo,
> 
> X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.
> 
>   mPhysicalAddressBits = CalculateMaximumSupportAddress ();

Thanks.

Do you think it would make sense to centralize the definition (i.e., the
allocation) of the mPhysicalAddressBits variable in this patch?

That is,
- instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c",
- you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c"
  (or another C source file that is built into both Ia32 and X64).

Thanks,
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Friday, August 25, 2017 5:50 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
> 
> Star,
> 
> On 08/24/17 05:20, Star Zeng wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
>> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
>> which 2GB are placed in 64-bit address).
>> It is because UEFI builds identity mapping page tables,
>>> 4G address is not supported at Ia32 build.
>>
>> This patch is to get the PhysicalAddressBits that is used to build in 
>> PageTbl.c(Ia32/X64), and use it to check whether the address is 
>> supported or not in ConvertMemoryPageAttributes().
>>
>> With this patch, the debug messages will be like below.
>> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
>> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 0x810000 
>> Success UefiMemory protection: 0x818000 - 0x820000 Success UefiMemory 
>> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
>> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 - 
>> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
>> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
>> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory 
>> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
>> 0x100000000 - 0x180000000 Unsupported
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31 
>> +++++++++++++++++-----
>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> index 32ce5958c59c..e88b42d73343 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  
>>  #include "PiSmmCpuDxeSmm.h"
>>  
>> +UINT8                               mPhysicalAddressBits;
>> +
>>  /**
>>    Create PageTable for SMM use.
>>  
>> @@ -36,6 +38,8 @@ SmmInitPageTable (
>>    //
>>    InitializeSpinLock (mPFLock);
>>  
>> +  mPhysicalAddressBits = 32;
>> +
>>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>>      //
>>      // Set own Page Fault entry instead of the default one, because 
>> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index dbce9ec520fe..1cf85c1481a7 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>>  extern SPIN_LOCK                           *mMemoryMappedLock;
>>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>>  extern UINTN                               mSmmCpuSmramRangeCount;
>> +extern UINT8                               mPhysicalAddressBits;
>>  
>>  //
>>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index a535389c26ce..3ad5256f1e03 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -1,6 +1,6 @@
>>  /** @file
>>  
>> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
>> +reserved.<BR>
>>  This program and the accompanying materials  are licensed and made 
>> available under the terms and conditions of the BSD License  which 
>> accompanies this distribution.  The full text of the license may be 
>> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>>    PAGE_ATTRIBUTE                    SplitAttribute;
>>    RETURN_STATUS                     Status;
>>    BOOLEAN                           IsEntryModified;
>> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>>  
>>    ASSERT (Attributes != 0);
>>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | 
>> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>>      return RETURN_INVALID_PARAMETER;
>>    }
>>  
>> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64 
>> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if (Length > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +
>>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
>> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>>  
>>    if (IsSplitted != NULL) {
>> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>>    VOID
>>    )
>>  {
>> +  EFI_STATUS            Status;
>>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>>    UINTN                 MemoryMapEntryCount;
>>    UINTN                 Index;
>> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>>    MemoryMap = mUefiMemoryMap;
>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>      if (IsUefiPageNotPresent(MemoryMap)) {
>> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
>> -      SmmSetMemoryAttributes (
>> +      Status = SmmSetMemoryAttributes (
>> +                 MemoryMap->PhysicalStart,
>> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +                 EFI_MEMORY_RP
>> +                 );
>> +      DEBUG ((
>> +        DEBUG_INFO,
>> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>>          MemoryMap->PhysicalStart,
>> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> -        EFI_MEMORY_RP
>> -        );
>> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +        Status
>> +        ));
>>      }
>>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>>    }
>>
> 
> before applying this patch for local testing, I figured I'd look it over quickly.
> 
> I think that you missed adding the X64 changes to the commit, with "git add". Because, the "mPhysicalAddressBits" variable is declared in common code, it is also consumed in common code, but it is only defined (i.e.,
> allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.
> 
> I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.
> 
> If I'm right and you decide to post v2, then I suggest another (very
> small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-25 11:52     ` Laszlo Ersek
@ 2017-08-26  2:24       ` Zeng, Star
  2017-08-27 17:10         ` Laszlo Ersek
  0 siblings, 1 reply; 11+ messages in thread
From: Zeng, Star @ 2017-08-26  2:24 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star

Laszlo,

I am ok to centralize the definition in the patch(V2 will cover it), how about PiSmmCpuDxeSmm.c?

Have you helped get the test result with the patch?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Friday, August 25, 2017 7:52 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

On 08/25/17 11:53, Zeng, Star wrote:
> Laszlo,
> 
> X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.
> 
>   mPhysicalAddressBits = CalculateMaximumSupportAddress ();

Thanks.

Do you think it would make sense to centralize the definition (i.e., the
allocation) of the mPhysicalAddressBits variable in this patch?

That is,
- instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c",
- you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c"
  (or another C source file that is built into both Ia32 and X64).

Thanks,
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Friday, August 25, 2017 5:50 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric 
> <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory 
> protection crash
> 
> Star,
> 
> On 08/24/17 05:20, Star Zeng wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
>> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
>> which 2GB are placed in 64-bit address).
>> It is because UEFI builds identity mapping page tables,
>>> 4G address is not supported at Ia32 build.
>>
>> This patch is to get the PhysicalAddressBits that is used to build in 
>> PageTbl.c(Ia32/X64), and use it to check whether the address is 
>> supported or not in ConvertMemoryPageAttributes().
>>
>> With this patch, the debug messages will be like below.
>> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
>> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 
>> 0x810000 Success UefiMemory protection: 0x818000 - 0x820000 Success 
>> UefiMemory
>> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
>> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 -
>> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
>> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
>> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory
>> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
>> 0x100000000 - 0x180000000 Unsupported
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31
>> +++++++++++++++++-----
>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> index 32ce5958c59c..e88b42d73343 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>  
>>  #include "PiSmmCpuDxeSmm.h"
>>  
>> +UINT8                               mPhysicalAddressBits;
>> +
>>  /**
>>    Create PageTable for SMM use.
>>  
>> @@ -36,6 +38,8 @@ SmmInitPageTable (
>>    //
>>    InitializeSpinLock (mPFLock);
>>  
>> +  mPhysicalAddressBits = 32;
>> +
>>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>>      //
>>      // Set own Page Fault entry instead of the default one, because 
>> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index dbce9ec520fe..1cf85c1481a7 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>>  extern SPIN_LOCK                           *mMemoryMappedLock;
>>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>>  extern UINTN                               mSmmCpuSmramRangeCount;
>> +extern UINT8                               mPhysicalAddressBits;
>>  
>>  //
>>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index a535389c26ce..3ad5256f1e03 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -1,6 +1,6 @@
>>  /** @file
>>  
>> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
>> +reserved.<BR>
>>  This program and the accompanying materials  are licensed and made 
>> available under the terms and conditions of the BSD License  which 
>> accompanies this distribution.  The full text of the license may be 
>> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>>    PAGE_ATTRIBUTE                    SplitAttribute;
>>    RETURN_STATUS                     Status;
>>    BOOLEAN                           IsEntryModified;
>> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>>  
>>    ASSERT (Attributes != 0);
>>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO |
>> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>>      return RETURN_INVALID_PARAMETER;
>>    }
>>  
>> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64
>> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if (Length > MaximumSupportMemAddress) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
>> +    return RETURN_UNSUPPORTED;
>> +  }
>> +
>>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
>> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>>  
>>    if (IsSplitted != NULL) {
>> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>>    VOID
>>    )
>>  {
>> +  EFI_STATUS            Status;
>>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>>    UINTN                 MemoryMapEntryCount;
>>    UINTN                 Index;
>> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>>    MemoryMap = mUefiMemoryMap;
>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>      if (IsUefiPageNotPresent(MemoryMap)) {
>> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
>> -      SmmSetMemoryAttributes (
>> +      Status = SmmSetMemoryAttributes (
>> +                 MemoryMap->PhysicalStart,
>> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +                 EFI_MEMORY_RP
>> +                 );
>> +      DEBUG ((
>> +        DEBUG_INFO,
>> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>>          MemoryMap->PhysicalStart,
>> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> -        EFI_MEMORY_RP
>> -        );
>> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>> +        Status
>> +        ));
>>      }
>>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>>    }
>>
> 
> before applying this patch for local testing, I figured I'd look it over quickly.
> 
> I think that you missed adding the X64 changes to the commit, with 
> "git add". Because, the "mPhysicalAddressBits" variable is declared in 
> common code, it is also consumed in common code, but it is only 
> defined (i.e.,
> allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.
> 
> I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.
> 
> If I'm right and you decide to post v2, then I suggest another (very
> small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-26  2:24       ` Zeng, Star
@ 2017-08-27 17:10         ` Laszlo Ersek
  2017-08-28  1:59           ` Zeng, Star
  0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2017-08-27 17:10 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric

On 08/26/17 04:24, Zeng, Star wrote:
> Laszlo,
> 
> I am ok to centralize the definition in the patch(V2 will cover it), how about PiSmmCpuDxeSmm.c?

Because I have now tested this patch (see below), at this point my
preference would be that you commit v1 as-is, and then post a separate
patch to centralize the variable. (I wouldn't like to re-test because of
the variable movement.)

I would have preferred to test v2 only, but the above works too I guess.

> Have you helped get the test result with the patch?

The crash is gone for the IA32 build, and I found no regressions in the
IA32X64 build.

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Friday, August 25, 2017 7:52 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
> 
> On 08/25/17 11:53, Zeng, Star wrote:
>> Laszlo,
>>
>> X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.
>>
>>   mPhysicalAddressBits = CalculateMaximumSupportAddress ();
> 
> Thanks.
> 
> Do you think it would make sense to centralize the definition (i.e., the
> allocation) of the mPhysicalAddressBits variable in this patch?
> 
> That is,
> - instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c",
> - you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c"
>   (or another C source file that is built into both Ia32 and X64).
> 
> Thanks,
> Laszlo
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Friday, August 25, 2017 5:50 PM
>> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric 
>> <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory 
>> protection crash
>>
>> Star,
>>
>> On 08/24/17 05:20, Star Zeng wrote:
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
>>> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB (of 
>>> which 2GB are placed in 64-bit address).
>>> It is because UEFI builds identity mapping page tables,
>>>> 4G address is not supported at Ia32 build.
>>>
>>> This patch is to get the PhysicalAddressBits that is used to build in 
>>> PageTbl.c(Ia32/X64), and use it to check whether the address is 
>>> supported or not in ConvertMemoryPageAttributes().
>>>
>>> With this patch, the debug messages will be like below.
>>> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
>>> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 - 
>>> 0x810000 Success UefiMemory protection: 0x818000 - 0x820000 Success 
>>> UefiMemory
>>> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
>>> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 -
>>> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
>>> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
>>> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory
>>> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
>>> 0x100000000 - 0x180000000 Unsupported
>>>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31
>>> +++++++++++++++++-----
>>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> index 32ce5958c59c..e88b42d73343 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>>  
>>>  #include "PiSmmCpuDxeSmm.h"
>>>  
>>> +UINT8                               mPhysicalAddressBits;
>>> +
>>>  /**
>>>    Create PageTable for SMM use.
>>>  
>>> @@ -36,6 +38,8 @@ SmmInitPageTable (
>>>    //
>>>    InitializeSpinLock (mPFLock);
>>>  
>>> +  mPhysicalAddressBits = 32;
>>> +
>>>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>>>      //
>>>      // Set own Page Fault entry instead of the default one, because 
>>> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> index dbce9ec520fe..1cf85c1481a7 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>>>  extern SPIN_LOCK                           *mMemoryMappedLock;
>>>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>>>  extern UINTN                               mSmmCpuSmramRangeCount;
>>> +extern UINT8                               mPhysicalAddressBits;
>>>  
>>>  //
>>>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> index a535389c26ce..3ad5256f1e03 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> @@ -1,6 +1,6 @@
>>>  /** @file
>>>  
>>> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
>>> +reserved.<BR>
>>>  This program and the accompanying materials  are licensed and made 
>>> available under the terms and conditions of the BSD License  which 
>>> accompanies this distribution.  The full text of the license may be 
>>> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>>>    PAGE_ATTRIBUTE                    SplitAttribute;
>>>    RETURN_STATUS                     Status;
>>>    BOOLEAN                           IsEntryModified;
>>> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>>>  
>>>    ASSERT (Attributes != 0);
>>>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO |
>>> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>>>      return RETURN_INVALID_PARAMETER;
>>>    }
>>>  
>>> +  MaximumSupportMemAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64
>>> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +  if (Length > MaximumSupportMemAddress) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +
>>>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
>>> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>>>  
>>>    if (IsSplitted != NULL) {
>>> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>>>    VOID
>>>    )
>>>  {
>>> +  EFI_STATUS            Status;
>>>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>>>    UINTN                 MemoryMapEntryCount;
>>>    UINTN                 Index;
>>> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>>>    MemoryMap = mUefiMemoryMap;
>>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>>      if (IsUefiPageNotPresent(MemoryMap)) {
>>> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
>>> -      SmmSetMemoryAttributes (
>>> +      Status = SmmSetMemoryAttributes (
>>> +                 MemoryMap->PhysicalStart,
>>> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> +                 EFI_MEMORY_RP
>>> +                 );
>>> +      DEBUG ((
>>> +        DEBUG_INFO,
>>> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>>>          MemoryMap->PhysicalStart,
>>> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> -        EFI_MEMORY_RP
>>> -        );
>>> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> +        Status
>>> +        ));
>>>      }
>>>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>>>    }
>>>
>>
>> before applying this patch for local testing, I figured I'd look it over quickly.
>>
>> I think that you missed adding the X64 changes to the commit, with 
>> "git add". Because, the "mPhysicalAddressBits" variable is declared in 
>> common code, it is also consumed in common code, but it is only 
>> defined (i.e.,
>> allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.
>>
>> I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.
>>
>> If I'm right and you decide to post v2, then I suggest another (very
>> small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash
  2017-08-27 17:10         ` Laszlo Ersek
@ 2017-08-28  1:59           ` Zeng, Star
  0 siblings, 0 replies; 11+ messages in thread
From: Zeng, Star @ 2017-08-28  1:59 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Dong, Eric, Zeng, Star

Thanks. :)
Pushed this patch at https://github.com/tianocore/edk2/commit/714c2603018a99a514c42c2b511c821f30ba9cdf.
And sent the patch at https://lists.01.org/pipermail/edk2-devel/2017-August/013768.html for the definition movement.

Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, August 28, 2017 1:10 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash

On 08/26/17 04:24, Zeng, Star wrote:
> Laszlo,
> 
> I am ok to centralize the definition in the patch(V2 will cover it), how about PiSmmCpuDxeSmm.c?

Because I have now tested this patch (see below), at this point my preference would be that you commit v1 as-is, and then post a separate patch to centralize the variable. (I wouldn't like to re-test because of the variable movement.)

I would have preferred to test v2 only, but the above works too I guess.

> Have you helped get the test result with the patch?

The crash is gone for the IA32 build, and I found no regressions in the
IA32X64 build.

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 25, 2017 7:52 PM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric 
> <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory 
> protection crash
> 
> On 08/25/17 11:53, Zeng, Star wrote:
>> Laszlo,
>>
>> X64 defined mPhysicalAddressBits already before the patch, and has the code below to assign it.
>>
>>   mPhysicalAddressBits = CalculateMaximumSupportAddress ();
> 
> Thanks.
> 
> Do you think it would make sense to centralize the definition (i.e., 
> the
> allocation) of the mPhysicalAddressBits variable in this patch?
> 
> That is,
> - instead of adding mPhysicalAddressBits to "Ia32/PageTbl.c",
> - you could move it from "X64/PageTbl.c" to "SmmCpuMemoryManagement.c"
>   (or another C source file that is built into both Ia32 and X64).
> 
> Thanks,
> Laszlo
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
>> Of Laszlo Ersek
>> Sent: Friday, August 25, 2017 5:50 PM
>> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric 
>> <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory 
>> protection crash
>>
>> Star,
>>
>> On 08/24/17 05:20, Star Zeng wrote:
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=624 reports memory 
>>> protection crash in PiSmmCpuDxeSmm, Ia32 build with RAM above 4GB 
>>> (of which 2GB are placed in 64-bit address).
>>> It is because UEFI builds identity mapping page tables,
>>>> 4G address is not supported at Ia32 build.
>>>
>>> This patch is to get the PhysicalAddressBits that is used to build 
>>> in PageTbl.c(Ia32/X64), and use it to check whether the address is 
>>> supported or not in ConvertMemoryPageAttributes().
>>>
>>> With this patch, the debug messages will be like below.
>>> UefiMemory protection: 0x0 - 0x9F000 Success UefiMemory protection: 
>>> 0x100000 - 0x807000 Success UefiMemory protection: 0x808000 -
>>> 0x810000 Success UefiMemory protection: 0x818000 - 0x820000 Success 
>>> UefiMemory
>>> protection: 0x1510000 - 0x7B798000 Success UefiMemory protection: 
>>> 0x7B79B000 - 0x7E538000 Success UefiMemory protection: 0x7E539000 -
>>> 0x7E545000 Success UefiMemory protection: 0x7E55A000 - 0x7E61F000 
>>> Success UefiMemory protection: 0x7E62B000 - 0x7F6AB000 Success 
>>> UefiMemory protection: 0x7F703000 - 0x7F70B000 Success UefiMemory
>>> protection: 0x7F70F000 - 0x7F778000 Success UefiMemory protection: 
>>> 0x100000000 - 0x180000000 Unsupported
>>>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Originally-suggested-by: Jiewen Yao <jiewen.yao@intel.com>
>>> Reported-by: Laszlo Ersek <lersek@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>>> ---
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |  4 +++
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  1 +
>>>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 31
>>> +++++++++++++++++-----
>>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> index 32ce5958c59c..e88b42d73343 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
>>> @@ -16,6 +16,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>>  
>>>  #include "PiSmmCpuDxeSmm.h"
>>>  
>>> +UINT8                               mPhysicalAddressBits;
>>> +
>>>  /**
>>>    Create PageTable for SMM use.
>>>  
>>> @@ -36,6 +38,8 @@ SmmInitPageTable (
>>>    //
>>>    InitializeSpinLock (mPFLock);
>>>  
>>> +  mPhysicalAddressBits = 32;
>>> +
>>>    if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
>>>      //
>>>      // Set own Page Fault entry instead of the default one, because 
>>> SMM Profile diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> index dbce9ec520fe..1cf85c1481a7 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>>> @@ -419,6 +419,7 @@ extern SPIN_LOCK                           *mConfigSmmCodeAccessCheckLock;
>>>  extern SPIN_LOCK                           *mMemoryMappedLock;
>>>  extern EFI_SMRAM_DESCRIPTOR                *mSmmCpuSmramRanges;
>>>  extern UINTN                               mSmmCpuSmramRangeCount;
>>> +extern UINT8                               mPhysicalAddressBits;
>>>  
>>>  //
>>>  // Copy of the PcdPteMemoryEncryptionAddressOrMask
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> index a535389c26ce..3ad5256f1e03 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>>> @@ -1,6 +1,6 @@
>>>  /** @file
>>>  
>>> -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) 2016 - 2017, Intel Corporation. All rights 
>>> +reserved.<BR>
>>>  This program and the accompanying materials  are licensed and made 
>>> available under the terms and conditions of the BSD License  which 
>>> accompanies this distribution.  The full text of the license may be 
>>> found at @@ -380,6 +380,7 @@ ConvertMemoryPageAttributes (
>>>    PAGE_ATTRIBUTE                    SplitAttribute;
>>>    RETURN_STATUS                     Status;
>>>    BOOLEAN                           IsEntryModified;
>>> +  EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>>>  
>>>    ASSERT (Attributes != 0);
>>>    ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO |
>>> EFI_MEMORY_XP)) == 0); @@ -391,6 +392,17 @@ ConvertMemoryPageAttributes (
>>>      return RETURN_INVALID_PARAMETER;
>>>    }
>>>  
>>> +  MaximumSupportMemAddress = 
>>> + (EFI_PHYSICAL_ADDRESS)(UINTN)(LShiftU64
>>> + (1, mPhysicalAddressBits) - 1);  if (BaseAddress > MaximumSupportMemAddress) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +  if (Length > MaximumSupportMemAddress) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +  if ((Length != 0) && (BaseAddress > MaximumSupportMemAddress - (Length - 1))) {
>>> +    return RETURN_UNSUPPORTED;
>>> +  }
>>> +
>>>  //  DEBUG ((DEBUG_ERROR, "ConvertMemoryPageAttributes(%x) - %016lx, 
>>> %016lx, %02lx\n", IsSet, BaseAddress, Length, Attributes));
>>>  
>>>    if (IsSplitted != NULL) {
>>> @@ -1037,6 +1049,7 @@ SetUefiMemMapAttributes (
>>>    VOID
>>>    )
>>>  {
>>> +  EFI_STATUS            Status;
>>>    EFI_MEMORY_DESCRIPTOR *MemoryMap;
>>>    UINTN                 MemoryMapEntryCount;
>>>    UINTN                 Index;
>>> @@ -1052,12 +1065,18 @@ SetUefiMemMapAttributes (
>>>    MemoryMap = mUefiMemoryMap;
>>>    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
>>>      if (IsUefiPageNotPresent(MemoryMap)) {
>>> -      DEBUG ((DEBUG_INFO, "UefiMemory protection: 0x%lx - 0x%lx\n", MemoryMap->PhysicalStart, MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)));
>>> -      SmmSetMemoryAttributes (
>>> +      Status = SmmSetMemoryAttributes (
>>> +                 MemoryMap->PhysicalStart,
>>> +                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> +                 EFI_MEMORY_RP
>>> +                 );
>>> +      DEBUG ((
>>> +        DEBUG_INFO,
>>> +        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
>>>          MemoryMap->PhysicalStart,
>>> -        EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> -        EFI_MEMORY_RP
>>> -        );
>>> +        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
>>> +        Status
>>> +        ));
>>>      }
>>>      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
>>>    }
>>>
>>
>> before applying this patch for local testing, I figured I'd look it over quickly.
>>
>> I think that you missed adding the X64 changes to the commit, with 
>> "git add". Because, the "mPhysicalAddressBits" variable is declared 
>> in common code, it is also consumed in common code, but it is only 
>> defined (i.e.,
>> allocated) and set in Ia32 code. I believe that applying this exact patch would prevent PiSmmCpuDxeSmm even from linking.
>>
>> I think for X64 you likely have a change similar to the Ia32 one (defining the variable and setting it to the actual physical address bits, likely from the CPU HOB), but it's not part of the patch.
>>
>> If I'm right and you decide to post v2, then I suggest another (very
>> small) improvement: I think the definition (=allocation) of "mPhysicalAddressBits" could also be moved to common code; only the assignments have to be architecture-specific.
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-08-28  1:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-24  3:20 [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Fix memory protection crash Star Zeng
2017-08-24  8:51 ` Yao, Jiewen
2017-08-24  9:02   ` Zeng, Star
2017-08-24 10:21 ` Laszlo Ersek
2017-08-24 10:28   ` Zeng, Star
2017-08-25  9:49 ` Laszlo Ersek
2017-08-25  9:53   ` Zeng, Star
2017-08-25 11:52     ` Laszlo Ersek
2017-08-26  2:24       ` Zeng, Star
2017-08-27 17:10         ` Laszlo Ersek
2017-08-28  1:59           ` Zeng, Star

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox