Skip to content

Commit

Permalink
Rename mPageAttributesInitialized to mGcdSyncComplete and Clarify Intent
Browse files Browse the repository at this point in the history
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
  • Loading branch information
TaylorBeebe authored and os-d committed Jun 27, 2024
1 parent 43c04c7 commit 92406b9
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 14 deletions.
2 changes: 1 addition & 1 deletion MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
6 changes: 3 additions & 3 deletions MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -527,7 +527,7 @@ GetPermissionAttributeForMemoryType (
Attributes |= EFI_MEMORY_XP;
}

if ((MemoryType == EfiConventionalMemory) && gDxeMps.FreeMemoryReadProtected && mPageAttributesInitialized) {
if ((MemoryType == EfiConventionalMemory) && gDxeMps.FreeMemoryReadProtected) {
Attributes |= EFI_MEMORY_RP;
}

Expand Down Expand Up @@ -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;
}

Expand Down
18 changes: 8 additions & 10 deletions MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down Expand Up @@ -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)) {
Expand All @@ -1457,7 +1456,6 @@ SetAccessAttributesInMemoryMap (
MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, *DescriptorSize);
}

mPageAttributesInitialized = FALSE;
return EFI_SUCCESS;
}

Expand Down Expand Up @@ -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 ();

Expand Down

0 comments on commit 92406b9

Please sign in to comment.