On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:
On 2/1/2018 9:17 AM, Wang, Jian J wrote:
You're right. Using a mask or separating the API into two
(SetMemoryAttributes/ClearMemoryAttributes)
is much better and can avoid many potential issues.
Regards,
Jian
For now the patch is good enough to leave NULL pointer detection
feature enabled.
Reviewed-by: Ruiyu Ni <ruiyu...@intel.com>
-----Original Message-----
From: Ni, Ruiyu
Sent: Tuesday, January 30, 2018 12:38 PM
To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.z...@intel.com>; Dong, Eric
<eric.d...@intel.com>; Yao,
Jiewen <jiewen....@intel.com>
Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between
NX and
NULL detection
On 1/29/2018 7:09 PM, Jian J Wang wrote:
If enabled, NX memory protection feature will mark all free memory as
NX (non-executable), including page 0. This will overwrite the
attributes
of page 0 if NULL pointer detection feature is also enabled and then
compromise the functionality of it. The solution is skipping the NX
attributes setting to page 0 if NULL pointer detection feature is
enabled.
Cc: Star Zeng <star.z...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Ruiyu Ni <ruiyu...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 862593f562..150167bf66 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
Attributes = GetPermissionAttributeForMemoryType
(MemoryMapEntry-
Type);
if (Attributes != 0) {
- SetUefiImageMemoryAttributes (
- MemoryMapEntry->PhysicalStart,
- LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
- Attributes);
+ if (MemoryMapEntry->PhysicalStart == 0 &&
+ PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
+ //
+ // Skip page 0 if NULL pointer detection is enabled to
avoid attributes
+ // overwritten.
+ //
By the way, could you please add an assertion here?
ASSERT (MemoryMapEntry->NumberOfPages != 0);
+ SetUefiImageMemoryAttributes (
+ MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
+ LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
EFI_PAGE_SHIFT),
+ Attributes);
+ } else {
+ SetUefiImageMemoryAttributes (
+ MemoryMapEntry->PhysicalStart,
+ LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
+ Attributes);
+ }
}
MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
DescriptorSize);
}
Does this bug expose an API-level issue?
SetUefiImageMemoryAttributes () should also accept a Mask value?
--
Thanks,
Ray
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
