Re: [edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature
Star,
I think the CoreGetMemorySpaceDescriptor() can get the memory capabilities. So
we can remove those hard-coded ones. In addition, since CoreAddRange() doesn't
touch mGcdMemorySpaceMap, CoreAcquireGcdMemoryLock and
CoreReleaseGcdMemoryLock are not necessary to protect CoreAddRange(). I'll
drop them as well.
Regards,
Jian
> -Original Message-
> From: Zeng, Star
> Sent: Thursday, October 25, 2018 11:37 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Ni, Ruiyu
> ; Yao, Jiewen ; Laszlo Ersek
> ; Zeng, Star
> Subject: Re: [edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory
> guard feature
>
> On 2018/10/24 13:26, Jian J Wang wrote:
> >> v3 changes:
> >> a. Merge from #4 and #5 of v2 patch
> >> b. Coding style cleanup
> >
> > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
> > which is illegal access to memory which has been freed. The principle
> > behind is similar to heap guard feature, that is we'll turn all pool
>
> Since we also regard UAF part of heap guard feature, better to use
> "pool/page heap guard feature" instead of "heap guard feature" here.
>
> I quoted a piece of code at below and have question that why it uses
> hard code Attribute parameter?
>
> + CoreAddRange (
> +EfiConventionalMemory,
> +StartAddress,
> +EndAddress,
> +EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT |
> EFI_MEMORY_WB
> +);
>
> Thanks,
> Star
>
> > memory allocation to page allocation and mark them to be not-present
> > once they are freed. The freed memory block will not be added back into
> > memory pool.
> >
> > This means that, once a page is allocated and freed, it cannot be
> > re-allocated. This will bring an issue, which is that there's
> > risk that memory space will be used out. To address it, the memory
> > service add logic to put part (at most 64 pages a time) of freed pages
> > back into page pool, so that the memory service can still have memory
> > to allocate, when all memory space have been allocated once. This is
> > called memory promotion. The promoted pages are always from the eldest
> > pages which haven been freed.
> >
> > This feature brings another problem is that memory map descriptors will
> > be increased enormously (200+ -> 2000+). One of change in this patch
> > is to update MergeMemoryMap() in file PropertiesTable.c to allow merge
> > freed pages back into the memory map. Now the number can stay at around
> > 510.
> >
> > Cc: Star Zeng
> > Cc: Michael D Kinney
> > Cc: Jiewen Yao
> > Cc: Ruiyu Ni
> > Cc: Laszlo Ersek
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang
> > ---
> > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409
> +-
> > MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++-
> > MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++-
> > MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +-
> > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
> > MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +-
> > 6 files changed, 524 insertions(+), 34 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > index 663f969c0d..449a022658 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > @@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
> mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
> > GLOBAL_REMOVE_IF_UNREFERENCED UINTN
> mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
> > = GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
> >
> > +//
> > +// Used for promoting freed but not used pages.
> > +//
> > +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS
> mLastPromotedPage = BASE_4GB;
> > +
> > /**
> > Set corresponding bits in bitmap table to 1 according to the address.
> >
> > @@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
> >
> > @return An integer containing the guarded memory bitmap.
> > **/
> > -UINTN
> > +UINT64
> > GetGuardedMemoryBits (
> > IN EFI_PHYSICAL_ADDRESSAddress,
> > IN UINTN NumberOfPages
> > @@ -387,7 +392,7 @@ GetGuardedMemoryBits (
> > {
> > UINT64*BitMap;
> > UINTN Bits;
> > - UINTN Result;
> > + UINT64Result;
> > U
Re: [edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature
Star,
Regards,
Jian
> -Original Message-
> From: Zeng, Star
> Sent: Thursday, October 25, 2018 11:37 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Ni, Ruiyu
> ; Yao, Jiewen ; Laszlo Ersek
> ; Zeng, Star
> Subject: Re: [edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory
> guard feature
>
> On 2018/10/24 13:26, Jian J Wang wrote:
> >> v3 changes:
> >> a. Merge from #4 and #5 of v2 patch
> >> b. Coding style cleanup
> >
> > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
> > which is illegal access to memory which has been freed. The principle
> > behind is similar to heap guard feature, that is we'll turn all pool
>
> Since we also regard UAF part of heap guard feature, better to use
> "pool/page heap guard feature" instead of "heap guard feature" here.
>
You're right. I'll change it.
> I quoted a piece of code at below and have question that why it uses
> hard code Attribute parameter?
>
> + CoreAddRange (
> +EfiConventionalMemory,
> +StartAddress,
> +EndAddress,
> +EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT |
> EFI_MEMORY_WB
> +);
>
Because I don't know the actual attributes at that time so I think it'd be
safer to add all supported ones.
> Thanks,
> Star
>
> > memory allocation to page allocation and mark them to be not-present
> > once they are freed. The freed memory block will not be added back into
> > memory pool.
> >
> > This means that, once a page is allocated and freed, it cannot be
> > re-allocated. This will bring an issue, which is that there's
> > risk that memory space will be used out. To address it, the memory
> > service add logic to put part (at most 64 pages a time) of freed pages
> > back into page pool, so that the memory service can still have memory
> > to allocate, when all memory space have been allocated once. This is
> > called memory promotion. The promoted pages are always from the eldest
> > pages which haven been freed.
> >
> > This feature brings another problem is that memory map descriptors will
> > be increased enormously (200+ -> 2000+). One of change in this patch
> > is to update MergeMemoryMap() in file PropertiesTable.c to allow merge
> > freed pages back into the memory map. Now the number can stay at around
> > 510.
> >
> > Cc: Star Zeng
> > Cc: Michael D Kinney
> > Cc: Jiewen Yao
> > Cc: Ruiyu Ni
> > Cc: Laszlo Ersek
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang
> > ---
> > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409
> +-
> > MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++-
> > MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++-
> > MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +-
> > MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
> > MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +-
> > 6 files changed, 524 insertions(+), 34 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > index 663f969c0d..449a022658 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
> > @@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
> mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
> > GLOBAL_REMOVE_IF_UNREFERENCED UINTN
> mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
> > = GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
> >
> > +//
> > +// Used for promoting freed but not used pages.
> > +//
> > +GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS
> mLastPromotedPage = BASE_4GB;
> > +
> > /**
> > Set corresponding bits in bitmap table to 1 according to the address.
> >
> > @@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
> >
> > @return An integer containing the guarded memory bitmap.
> > **/
> > -UINTN
> > +UINT64
> > GetGuardedMemoryBits (
> > IN EFI_PHYSICAL_ADDRESSAddress,
> > IN UINTN NumberOfPages
> > @@ -387,7 +392,7 @@ GetGuardedMemoryBits (
> > {
> > UINT64*BitMap;
> > UINTN Bits;
> > - UINTN Result;
> > + UINT64Result;
> > UINTN Shift;
> > UINTN BitsToUnitEnd;
> >
> > @@ -660,15 +665,16 @@ IsPageTypeToGua
Re: [edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature
On 2018/10/24 13:26, Jian J Wang wrote:
v3 changes:
a. Merge from #4 and #5 of v2 patch
b. Coding style cleanup
Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
Since we also regard UAF part of heap guard feature, better to use
"pool/page heap guard feature" instead of "heap guard feature" here.
I quoted a piece of code at below and have question that why it uses
hard code Attribute parameter?
+ CoreAddRange (
+EfiConventionalMemory,
+StartAddress,
+EndAddress,
+EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB
+);
Thanks,
Star
memory allocation to page allocation and mark them to be not-present
once they are freed. The freed memory block will not be added back into
memory pool.
This means that, once a page is allocated and freed, it cannot be
re-allocated. This will bring an issue, which is that there's
risk that memory space will be used out. To address it, the memory
service add logic to put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages which haven been freed.
This feature brings another problem is that memory map descriptors will
be increased enormously (200+ -> 2000+). One of change in this patch
is to update MergeMemoryMap() in file PropertiesTable.c to allow merge
freed pages back into the memory map. Now the number can stay at around
510.
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +-
MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++-
MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++-
MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +-
6 files changed, 524 insertions(+), 34 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 663f969c0d..449a022658 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
= GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
+//
+// Used for promoting freed but not used pages.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS mLastPromotedPage =
BASE_4GB;
+
/**
Set corresponding bits in bitmap table to 1 according to the address.
@@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
@return An integer containing the guarded memory bitmap.
**/
-UINTN
+UINT64
GetGuardedMemoryBits (
IN EFI_PHYSICAL_ADDRESSAddress,
IN UINTN NumberOfPages
@@ -387,7 +392,7 @@ GetGuardedMemoryBits (
{
UINT64*BitMap;
UINTN Bits;
- UINTN Result;
+ UINT64Result;
UINTN Shift;
UINTN BitsToUnitEnd;
@@ -660,15 +665,16 @@ IsPageTypeToGuard (
/**
Check to see if the heap guard is enabled for page and/or pool allocation.
+ @param[in] GuardType Specify the sub-type(s) of Heap Guard.
+
@return TRUE/FALSE.
**/
BOOLEAN
IsHeapGuardEnabled (
- VOID
+ UINT8 GuardType
)
{
- return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
- GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+ return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages, GuardType);
}
/**
@@ -1203,6 +1209,380 @@ SetAllGuardPages (
}
}
+/**
+ Find the address of top-most guarded free page.
+
+ @param[out] AddressStart address of top-most guarded free page.
+
+ @return VOID.
+**/
+VOID
+GetLastGuardedFreePageAddress (
+ OUT EFI_PHYSICAL_ADDRESS *Address
+ )
+{
+ EFI_PHYSICAL_ADDRESSAddressGranularity;
+ EFI_PHYSICAL_ADDRESSBaseAddress;
+ UINTN Level;
+ UINT64 Map;
+ INTNIndex;
+
+ ASSERT (mMapLevel >= 1);
+
+ BaseAddress = 0;
+ Map = mGuardedMemoryMap;
+ for (Level = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+ Level < GUARDED_HEAP_MAP_TABLE_DEPTH;
+ ++Level) {
+AddressGranularity = LShiftU64 (1, mLevelShift[Level]);
+
+//
+// Find the non-NULL entry at largest index.
+//
+for (Index = (INTN)mLevelMask[Level]; Index >= 0 ; --Index) {
+ if (((UINT64 *)(UINTN)Map)[Index] != 0) {
+BaseAddress += MultU64
[edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature
> v3 changes:
> a. Merge from #4 and #5 of v2 patch
> b. Coding style cleanup
Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
memory allocation to page allocation and mark them to be not-present
once they are freed. The freed memory block will not be added back into
memory pool.
This means that, once a page is allocated and freed, it cannot be
re-allocated. This will bring an issue, which is that there's
risk that memory space will be used out. To address it, the memory
service add logic to put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages which haven been freed.
This feature brings another problem is that memory map descriptors will
be increased enormously (200+ -> 2000+). One of change in this patch
is to update MergeMemoryMap() in file PropertiesTable.c to allow merge
freed pages back into the memory map. Now the number can stay at around
510.
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +-
MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++-
MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++-
MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +-
6 files changed, 524 insertions(+), 34 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 663f969c0d..449a022658 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
= GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
+//
+// Used for promoting freed but not used pages.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS mLastPromotedPage =
BASE_4GB;
+
/**
Set corresponding bits in bitmap table to 1 according to the address.
@@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
@return An integer containing the guarded memory bitmap.
**/
-UINTN
+UINT64
GetGuardedMemoryBits (
IN EFI_PHYSICAL_ADDRESSAddress,
IN UINTN NumberOfPages
@@ -387,7 +392,7 @@ GetGuardedMemoryBits (
{
UINT64*BitMap;
UINTN Bits;
- UINTN Result;
+ UINT64Result;
UINTN Shift;
UINTN BitsToUnitEnd;
@@ -660,15 +665,16 @@ IsPageTypeToGuard (
/**
Check to see if the heap guard is enabled for page and/or pool allocation.
+ @param[in] GuardType Specify the sub-type(s) of Heap Guard.
+
@return TRUE/FALSE.
**/
BOOLEAN
IsHeapGuardEnabled (
- VOID
+ UINT8 GuardType
)
{
- return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
- GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+ return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages, GuardType);
}
/**
@@ -1203,6 +1209,380 @@ SetAllGuardPages (
}
}
+/**
+ Find the address of top-most guarded free page.
+
+ @param[out] AddressStart address of top-most guarded free page.
+
+ @return VOID.
+**/
+VOID
+GetLastGuardedFreePageAddress (
+ OUT EFI_PHYSICAL_ADDRESS *Address
+ )
+{
+ EFI_PHYSICAL_ADDRESSAddressGranularity;
+ EFI_PHYSICAL_ADDRESSBaseAddress;
+ UINTN Level;
+ UINT64 Map;
+ INTNIndex;
+
+ ASSERT (mMapLevel >= 1);
+
+ BaseAddress = 0;
+ Map = mGuardedMemoryMap;
+ for (Level = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+ Level < GUARDED_HEAP_MAP_TABLE_DEPTH;
+ ++Level) {
+AddressGranularity = LShiftU64 (1, mLevelShift[Level]);
+
+//
+// Find the non-NULL entry at largest index.
+//
+for (Index = (INTN)mLevelMask[Level]; Index >= 0 ; --Index) {
+ if (((UINT64 *)(UINTN)Map)[Index] != 0) {
+BaseAddress += MultU64x32 (AddressGranularity, (UINT32)Index);
+Map = ((UINT64 *)(UINTN)Map)[Index];
+break;
+ }
+}
+ }
+
+ //
+ // Find the non-zero MSB then get the page address.
+ //
+ while (Map != 0) {
+Map = RShiftU64 (Map, 1);
+BaseAddress += EFI_PAGES_TO_SIZE (1);
+ }
+
+ *Address = BaseAddress;
+}
+
+/**
+ Record freed pages.
+
+ @param[in] BaseAddress Base address of just freed pages.
+ @param[in] Pages Number of freed pages.
+
+ @return VOID.
+**/
+VOID
+MarkFreedPages (
+
