From 92406b9172f3dce4ebe3a574ac6408c14908730b Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Tue, 30 Apr 2024 16:37:53 -0700 Subject: [PATCH] Rename mPageAttributesInitialized to mGcdSyncComplete and Clarify Intent Description mPageAttributesInitialized was used to signal that the memory protection initialization routine was complete so EFI_MEMORY_RP can be safely set on free memory. However, the blocker to setting EFI_MEMORY_RP on free memory is not the initialization of the page attributes, but the completion of the GCD sync. This change renames mPageAttributesInitialized to mGcdSyncComplete to better reflect the intent of the variable. Additionally, it makes more sense to not change what GetPermissionAttributeForMemoryType() returns based on the value of mGcdSyncComplete. Instead, it should always return the permission attributes which should be applied to the memory type but ApplyMemoryProtectionPolicy() should bail if the GCD sync has not been completed. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... How This Was Tested Tested by running the DxePagingAuditTestApp on Q35 and SBSA platforms. Additionally tested by booting to Windows on Q35. Integration Instructions N/A --- MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 2 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 6 +++--- .../Core/Dxe/Misc/MemoryProtectionSupport.c | 18 ++++++++---------- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c index 76517310878..ff64ff27ea2 100644 --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c @@ -16,7 +16,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent // GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN mOnGuarding = FALSE; -extern BOOLEAN mPageAttributesInitialized; // MU_CHANGE +extern BOOLEAN mGcdSyncComplete; // MU_CHANGE // // Pointer to table tracking the Guarded memory with bitmap, in which '1' diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index a1a49d2348c..9d9f3a5947e 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c @@ -70,7 +70,7 @@ UINT32 mImageProtectionPolicy; extern LIST_ENTRY mGcdMemorySpaceMap; EFI_MEMORY_ATTRIBUTE_PROTOCOL *mMemoryAttribute = NULL; // MU_CHANGE -extern BOOLEAN mPageAttributesInitialized; // MU_CHANGE +extern BOOLEAN mGcdSyncComplete; // MU_CHANGE STATIC LIST_ENTRY mProtectedImageRecordList = INITIALIZE_LIST_HEAD_VARIABLE (mProtectedImageRecordList); // MU_CHANGE: Initialize at compile time @@ -527,7 +527,7 @@ GetPermissionAttributeForMemoryType ( Attributes |= EFI_MEMORY_XP; } - if ((MemoryType == EfiConventionalMemory) && gDxeMps.FreeMemoryReadProtected && mPageAttributesInitialized) { + if ((MemoryType == EfiConventionalMemory) && gDxeMps.FreeMemoryReadProtected) { Attributes |= EFI_MEMORY_RP; } @@ -1167,7 +1167,7 @@ ApplyMemoryProtectionPolicy ( // MU_CHANGE: Because GCD sync and memory protection initialization // ordering is reversed, check if the initialization routine // has run before allowing this function to execute. - if ((gCpu == NULL) || !mPageAttributesInitialized) { + if ((gCpu == NULL) || !mGcdSyncComplete) { return EFI_SUCCESS; } diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c index f843abe9930..c35ae720399 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c @@ -29,7 +29,7 @@ BOOLEAN mEnhancedMemoryProtectionActive = TRUE; EFI_MEMORY_ATTRIBUTE_PROTOCOL *mMemoryAttributeProtocol = NULL; UINT8 *mBitmapGlobal = NULL; LIST_ENTRY **mArrayOfListEntryPointers = NULL; -BOOLEAN mPageAttributesInitialized = FALSE; +BOOLEAN mGcdSyncComplete = FALSE; #define IS_BITMAP_INDEX_SET(Bitmap, Index) ((((UINT8*)Bitmap)[Index / 8] & (1 << (Index % 8))) != 0 ? TRUE : FALSE) #define SET_BITMAP_INDEX(Bitmap, Index) (((UINT8*)Bitmap)[Index / 8] |= (1 << (Index % 8))) @@ -1443,9 +1443,8 @@ SetAccessAttributesInMemoryMap ( return EFI_INVALID_PARAMETER; } - mPageAttributesInitialized = TRUE; - MemoryMapEntry = MemoryMap; - MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + *MemoryMapSize); + MemoryMapEntry = MemoryMap; + MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + *MemoryMapSize); while (MemoryMapEntry < MemoryMapEnd) { if (!IS_BITMAP_INDEX_SET (Bitmap, Index)) { @@ -1457,7 +1456,6 @@ SetAccessAttributesInMemoryMap ( MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, *DescriptorSize); } - mPageAttributesInitialized = FALSE; return EFI_SUCCESS; } @@ -2752,14 +2750,14 @@ InitializePageAttributesCallback ( EFI_STATUS Status; EFI_EVENT DisableNullDetectionEvent; + // Inidcates that the GCD sync process has been completed. This BOOLEAN + // must set this to TRUE so calls to ApplyMemoryProtectionPolicy() are not + // blocked. This BOOLEAN also allows guard pages to be set. + mGcdSyncComplete = TRUE; + // Initialize paging attributes InitializePageAttributesForMemoryProtectionPolicy (); - // Must set this to TRUE so calls to ApplyMemoryProtectionPolicy() are not - // blocked. This BOOLEAN also allows guard pages to be set and EFI_MEMORY_RP - // to be applied to memory being freed. - mPageAttributesInitialized = TRUE; - // Set all the guard pages HeapGuardCpuArchProtocolNotify ();