Sorry for the email title. This is not a series patch. And I missed one more issue addressed by this patch: c. NULL address handling due to allocation failure When allocation failure, normally a NULL will be returned. But Heap Guard code will still try to adjust the starting address of it, which will cause a non-NULL pointer returned.
Regards, Jian > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J > Wang > Sent: Monday, December 25, 2017 10:23 AM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > Zeng, Star <star.z...@intel.com> > Subject: [edk2] [PATCH 1/2] MdeModulePkg/Core: Fix heap guard issues > > Two issues addressed here: > > a. Make NX memory protection and heap guard to be compatible > The solution is to check PcdDxeNxMemoryProtectionPolicy in Heap Guard to see > if the free memory should be set to NX, and set the Guard page to NX before > it's freed back to memory pool. This can solve the issue which NX setting > would be overwritten by Heap Guard feature in certain configuration. > > b. Returned pool address was not 8-byte aligned sometimes > This happened only when BIT7 is not set in PcdHeapGuardPropertyMask. Since > 8-byte alignment is UEFI spec required, letting allocated pool adjacent to > tail guard page cannot be guaranteed. > > Cc: Star Zeng <star.z...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang <jian.j.w...@intel.com> > --- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 31 > ++++++++++++++++++++++----- > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 3 --- > MdeModulePkg/Core/PiSmmCore/HeapGuard.c | 14 ++++++++++-- > 3 files changed, 38 insertions(+), 10 deletions(-) > > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > index bee229f4c8..0f035043e1 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c > @@ -583,8 +583,7 @@ SetGuardPage ( > mOnGuarding = TRUE; > // > // Note: This might overwrite other attributes needed by other features, > - // such as memory protection (NX). Please make sure they are not enabled > - // at the same time. > + // such as NX memory protection. > // > gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, > EFI_MEMORY_RP); > mOnGuarding = FALSE; > @@ -605,6 +604,18 @@ UnsetGuardPage ( > IN EFI_PHYSICAL_ADDRESS BaseAddress > ) > { > + UINT64 Attributes; > + > + // > + // Once the Guard page is unset, it will be freed back to memory pool. NX > + // memory protection must be restored for this page if NX is enabled for > free > + // memory. > + // > + Attributes = 0; > + if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & (1 << > EfiConventionalMemory)) != 0) { > + Attributes |= EFI_MEMORY_XP; > + } > + > // > // Set flag to make sure allocating memory without GUARD for page table > // operation; otherwise infinite loops could be caused. > @@ -615,7 +626,7 @@ UnsetGuardPage ( > // such as memory protection (NX). Please make sure they are not enabled > // at the same time. > // > - gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, 0); > + gCpu->SetMemoryAttributes (gCpu, BaseAddress, EFI_PAGE_SIZE, Attributes); > mOnGuarding = FALSE; > } > > @@ -869,6 +880,15 @@ AdjustMemoryS ( > { > UINT64 Target; > > + // > + // UEFI spec requires that allocated pool must be 8-byte aligned. If it's > + // indicated to put the pool near the Tail Guard, we need extra bytes to > + // make sure alignment of the returned pool address. > + // > + if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) { > + SizeRequested = ALIGN_VALUE(SizeRequested, 8); > + } > + > Target = Start + Size - SizeRequested; > > // > @@ -1052,7 +1072,7 @@ AdjustPoolHeadA ( > IN UINTN Size > ) > { > - if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > + if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > // > // Pool head is put near the head Guard > // > @@ -1062,6 +1082,7 @@ AdjustPoolHeadA ( > // > // Pool head is put near the tail Guard > // > + Size = ALIGN_VALUE (Size, 8); > return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size); > } > > @@ -1077,7 +1098,7 @@ AdjustPoolHeadF ( > IN EFI_PHYSICAL_ADDRESS Memory > ) > { > - if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > + if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > // > // Pool head is put near the head Guard > // > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > index a74cfc137a..862593f562 100644 > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > @@ -1070,15 +1070,12 @@ CoreInitializeMemoryProtection ( > // - code regions should have no EFI_MEMORY_XP attribute > // - EfiConventionalMemory and EfiBootServicesData should use the > // same attribute > - // - heap guard should not be enabled for the same type of memory > // > ASSERT ((GetPermissionAttributeForMemoryType (EfiBootServicesCode) & > EFI_MEMORY_XP) == 0); > ASSERT ((GetPermissionAttributeForMemoryType (EfiRuntimeServicesCode) & > EFI_MEMORY_XP) == 0); > ASSERT ((GetPermissionAttributeForMemoryType (EfiLoaderCode) & > EFI_MEMORY_XP) == 0); > ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) == > GetPermissionAttributeForMemoryType (EfiConventionalMemory)); > - ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64 > (PcdHeapGuardPoolType)) == 0); > - ASSERT ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & PcdGet64 > (PcdHeapGuardPageType)) == 0); > > if (mImageProtectionPolicy != 0 || PcdGet64 > (PcdDxeNxMemoryProtectionPolicy) != 0) { > Status = CoreCreateEvent ( > diff --git a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > index 04fa1747a1..063a330b18 100644 > --- a/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > +++ b/MdeModulePkg/Core/PiSmmCore/HeapGuard.c > @@ -877,6 +877,15 @@ AdjustMemoryS ( > { > UINT64 Target; > > + // > + // UEFI spec requires that allocated pool must be 8-byte aligned. If it's > + // indicated to put the pool near the Tail Guard, we need extra bytes to > + // make sure alignment of the returned pool address. > + // > + if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) == 0) { > + SizeRequested = ALIGN_VALUE(SizeRequested, 8); > + } > + > Target = Start + Size - SizeRequested; > > // > @@ -1060,7 +1069,7 @@ AdjustPoolHeadA ( > IN UINTN Size > ) > { > - if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > + if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > // > // Pool head is put near the head Guard > // > @@ -1070,6 +1079,7 @@ AdjustPoolHeadA ( > // > // Pool head is put near the tail Guard > // > + Size = ALIGN_VALUE (Size, 8); > return (VOID *)(UINTN)(Memory + EFI_PAGES_TO_SIZE (NoPages) - Size); > } > > @@ -1085,7 +1095,7 @@ AdjustPoolHeadF ( > IN EFI_PHYSICAL_ADDRESS Memory > ) > { > - if ((PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > + if (Memory == 0 || (PcdGet8 (PcdHeapGuardPropertyMask) & BIT7) != 0) { > // > // Pool head is put near the head Guard > // > -- > 2.15.1.windows.2 > > _______________________________________________ > 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
