From 144e3fe63244e0dfb14a6442dd969dd2757e844e Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Fri, 26 Apr 2024 13:28:43 -0700 Subject: [PATCH 1/5] Apply EFI_MEMORY_RP on Free Memory Description This PR makes the necessary changes to apply EFI_MEMORY_RP on EfiConventionalMemory and adds a memory protection policy to configure the setting. - [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, ... - [x] 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 DXE Paging Audit on Q35 and SBSA with various memory protection profiles. Integration Instructions Platforms which use pre-built binaries of Mu repos will need to rebuild them to sync the memory protection policy between all modules. --- MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 31 +- MdeModulePkg/Core/Dxe/Mem/Page.c | 59 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 528 +++++++++--------- .../Core/Dxe/Misc/MemoryProtectionSupport.c | 68 ++- .../Core/Dxe/Misc/MemoryProtectionSupport.h | 13 + .../Guid/DxeMemoryProtectionSettings.h | 30 +- .../Include/Guid/MmMemoryProtectionSettings.h | 4 +- .../DxeMemoryProtectionHobLib.c | 16 - MdeModulePkg/MdeModulePkg.dec | 4 + MdePkg/Include/Library/DebugLib.h | 9 +- MdePkg/Include/Protocol/Cpu.h | 7 + UefiCpuPkg/CpuDxe/CpuDxe.c | 9 + UefiCpuPkg/CpuDxe/CpuDxe.inf | 1 + 14 files changed, 463 insertions(+), 317 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf index 72ddc9a8ff..1608117dbe 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain.inf +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf @@ -183,6 +183,7 @@ gMemoryProtectionDebugProtocolGuid ## SOMETIMES_PRODUCES ## MS_CHANGE gEfiMemoryAttributeProtocolGuid ## CONSUMES ## MS_CHANGE gMemoryProtectionSpecialRegionProtocolGuid ## PRODUCES ## MU_CHANGE + gEdkiiGcdSyncCompleteProtocolGuid ## CONSUMES ## MU_CHANGE [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber ## SOMETIMES_CONSUMES diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c index 62d8005e97..df4a627529 100644 --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c @@ -16,6 +16,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent // GLOBAL_REMOVE_IF_UNREFERENCED BOOLEAN mOnGuarding = FALSE; +extern BOOLEAN mPageAttributesInitialized; // MU_CHANGE + // // Pointer to table tracking the Guarded memory with bitmap, in which '1' // is used to indicate memory guarded. '0' might be free memory or Guard @@ -252,6 +254,14 @@ FindGuardedMemoryMap ( ); ASSERT_EFI_ERROR (Status); ASSERT (MapMemory != 0); + // MU_CHANGE START: Apply Protection policy to the allocated memory + ApplyMemoryProtectionPolicy ( + EfiConventionalMemory, + EfiBootServicesData, + MapMemory, + ALIGN_VALUE (Size, EFI_PAGE_SIZE) + ); + // MU_CHANGE END SetMem ((VOID *)(UINTN)MapMemory, Size, 0); @@ -283,6 +293,14 @@ FindGuardedMemoryMap ( ); ASSERT_EFI_ERROR (Status); ASSERT (MapMemory != 0); + // MU_CHANGE START: Apply Protection policy to the allocated memory + ApplyMemoryProtectionPolicy ( + EfiConventionalMemory, + EfiBootServicesData, + MapMemory, + ALIGN_VALUE (Size, EFI_PAGE_SIZE) + ); + // MU_CHANGE END SetMem ((VOID *)(UINTN)MapMemory, Size, 0); *GuardMap = MapMemory; @@ -560,6 +578,13 @@ UnsetGuardPage ( Attributes |= EFI_MEMORY_XP; } + // MU_CHANGE START: Add support for RP on free mem + if (gDxeMps.FreeMemoryReadProtected) { + Attributes |= EFI_MEMORY_RP; + } + + // MU_CHANGE END + // // Set flag to make sure allocating memory without GUARD for page table // operation; otherwise infinite loops could be caused. @@ -690,10 +715,10 @@ IsHeapGuardEnabled ( UINT8 GuardType ) { - // MU_CHANGE START Update to work with memory protection settings HOB + // MU_CHANGE START: Update to work with memory protection settings HOB, + // remove freed memory guard. if ((GuardType & GUARD_HEAP_TYPE_PAGE && gDxeMps.HeapGuardPolicy.Fields.UefiPageGuard) || - (GuardType & GUARD_HEAP_TYPE_POOL && gDxeMps.HeapGuardPolicy.Fields.UefiPoolGuard) || - (GuardType & GUARD_HEAP_TYPE_FREED && gDxeMps.HeapGuardPolicy.Fields.UefiFreedMemoryGuard)) + (GuardType & GUARD_HEAP_TYPE_POOL && gDxeMps.HeapGuardPolicy.Fields.UefiPoolGuard)) { return TRUE; } diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c index a2ee3606ad..90be0425c1 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Page.c +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c @@ -288,6 +288,15 @@ AllocateMemoryMapEntry ( DEFAULT_PAGE_ALLOCATION_GRANULARITY, FALSE ); + // MU_CHANGE START: The above call to CoreAllocatePoolPages() sidesteps the application of the + // memory protection policy so apply it here to avoid a potential page fault + ApplyMemoryProtectionPolicy ( + EfiConventionalMemory, + EfiBootServicesData, + (EFI_PHYSICAL_ADDRESS)(UINTN)FreeDescriptorEntries, + DEFAULT_PAGE_ALLOCATION_GRANULARITY + ); + // MU_CHANGE END if (FreeDescriptorEntries != NULL) { // // Enque the free memmory map entries into the list @@ -947,6 +956,21 @@ CoreConvertPagesEx ( Entry = NULL; } + // MU_CHANGE [BEGIN] + // The below call may allocate pages which, if we're freeing memory (implied by + // the new type being EfiConventionalMemory), could cause the memory we're currently + // freeing to be allocated before we're done freeing it if CoreFreeMemoryMapStack() + // is called after AddRange(). So, if we are freeing, let's free the memory map + // stack before adding memory we're converting to the free list. + if (ChangingType && (NewType == EfiConventionalMemory)) { + // + // Move any map descriptor stack to general pool + // + CoreFreeMemoryMapStack (); + } + + // MU_CHANGE [END] + // // Add our new range in. Don't do this for freed pages if freed-memory // guard is enabled. @@ -973,10 +997,20 @@ CoreConvertPagesEx ( } } - // - // Move any map descriptor stack to general pool - // - CoreFreeMemoryMapStack (); + // MU_CHANGE [BEGIN] + // The below call may allocate pages which, if we're allocating memory (implied by + // the new type not being EfiConventionalMemory), could cause the range we're currently + // converting to also be allocated in the below call. To avoid this case, we should + // call CoreFreeMemoryMapStack() after we've called AddRange() to mark this memory + // as allocated. + if (!ChangingType || (ChangingType && (NewType != EfiConventionalMemory))) { + // + // Move any map descriptor stack to general pool + // + CoreFreeMemoryMapStack (); + } + + // MU_CHANGE [END] // // Bump the starting address, and convert the next range @@ -1578,23 +1612,6 @@ CoreInternalFreePages ( UINTN Alignment; BOOLEAN IsGuarded; - // MU_CHANGE Start: Unprotect page(s) before free if the memory will be cleared on free - UINT64 Attributes; - - if (DebugClearMemoryEnabled () && (mMemoryAttributeProtocol != NULL)) { - Status = mMemoryAttributeProtocol->GetMemoryAttributes (mMemoryAttributeProtocol, Memory, EFI_PAGES_TO_SIZE (NumberOfPages), &Attributes); - - if ((Attributes & EFI_MEMORY_RO) || (Attributes & EFI_MEMORY_RP) || (Status == EFI_NO_MAPPING)) { - Status = ClearAccessAttributesFromMemoryRange (Memory, EFI_PAGES_TO_SIZE (NumberOfPages)); - - if (EFI_ERROR (Status) && (Status != EFI_NOT_READY)) { - DEBUG ((DEBUG_WARN, "%a - Unable to clear attributes from memory at base: 0x%llx\n", __FUNCTION__, Memory)); - } - } - } - - // MU_CHANGE End - // // Free the range // diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index f5a12bfde8..6d11433e3c 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c @@ -79,6 +79,9 @@ STATIC MEMORY_PROTECTION_DEBUG_PROTOCOL mMemoryProtectionDebug = IsGuardPage, GetImageList }; + +BOOLEAN mPageAttributesInitialized = FALSE; + // MS_CHANGE - END /** @@ -504,7 +507,8 @@ GetPermissionAttributeForMemoryType ( IN EFI_MEMORY_TYPE MemoryType ) { - // MU_CHANGE START Update to use memory protection settings HOB + // MU_CHANGE START: Update to use memory protection settings HOB, + // add support for RP on free memory. // UINT64 TestBit; // if ((UINT32)MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) { @@ -521,15 +525,23 @@ GetPermissionAttributeForMemoryType ( // return 0; // } + UINT64 Attributes = 0; + // Handle code allocations according to the NX_COMPAT DLL flag. If the flag is // set, the image should update the attributes of code type allocates when it's ready to execute them. if (!IsEnhancedMemoryProtectionActive ()) { return 0; - } else if (GetDxeMemoryTypeSettingFromBitfield (MemoryType, gDxeMps.NxProtectionPolicy)) { - return EFI_MEMORY_XP; } - return 0; + if (GetDxeMemoryTypeSettingFromBitfield (MemoryType, gDxeMps.NxProtectionPolicy)) { + Attributes |= EFI_MEMORY_XP; + } + + if ((MemoryType == EfiConventionalMemory) && gDxeMps.FreeMemoryReadProtected && mPageAttributesInitialized) { + Attributes |= EFI_MEMORY_RP; + } + + return Attributes; // MU_CHANGE END } @@ -639,214 +651,216 @@ MergeMemoryMapForProtectionPolicy ( return; } -/** - Remove exec permissions from all regions whose type is identified by - the Dxe NX Protection Policy. // MU_CHANGE -**/ -STATIC -VOID -InitializeDxeNxMemoryProtectionPolicy ( - VOID - ) -{ - UINTN MemoryMapSize; - UINTN MapKey; - UINTN DescriptorSize; - UINT32 DescriptorVersion; - EFI_MEMORY_DESCRIPTOR *MemoryMap; - EFI_MEMORY_DESCRIPTOR *MemoryMapEntry; - EFI_MEMORY_DESCRIPTOR *MemoryMapEnd; - EFI_STATUS Status; - UINT64 Attributes; - LIST_ENTRY *Link; - EFI_GCD_MAP_ENTRY *Entry; - EFI_PEI_HOB_POINTERS Hob; - EFI_HOB_MEMORY_ALLOCATION *MemoryHob; - EFI_PHYSICAL_ADDRESS StackBase; - - // - // Get the EFI memory map. - // - MemoryMapSize = 0; - MemoryMap = NULL; - - Status = gBS->GetMemoryMap ( - &MemoryMapSize, - MemoryMap, - &MapKey, - &DescriptorSize, - &DescriptorVersion - ); - ASSERT (Status == EFI_BUFFER_TOO_SMALL); - do { - MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize); - // MU_CHANGE [BEGIN] - CodeQL change - if (MemoryMap == NULL) { - ASSERT (MemoryMap != NULL); - return; - } - - // MU_CHANGE [END] - CodeQL change - Status = gBS->GetMemoryMap ( - &MemoryMapSize, - MemoryMap, - &MapKey, - &DescriptorSize, - &DescriptorVersion - ); - if (EFI_ERROR (Status)) { - FreePool (MemoryMap); - } - } while (Status == EFI_BUFFER_TOO_SMALL); - - ASSERT_EFI_ERROR (Status); - - StackBase = 0; - // MU_CHANGE START Update to use memory protection settings HOB - // if (PcdGetBool (PcdCpuStackGuard)) { - if (gDxeMps.CpuStackGuard) { - // MU_CHANGE END - // - // Get the base of stack from Hob. - // - Hob.Raw = GetHobList (); - while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw)) != NULL) { - MemoryHob = Hob.MemoryAllocation; - if (CompareGuid (&gEfiHobMemoryAllocStackGuid, &MemoryHob->AllocDescriptor.Name)) { - DEBUG (( - DEBUG_INFO, - "%a: StackBase = 0x%016lx StackSize = 0x%016lx\n", - __func__, - MemoryHob->AllocDescriptor.MemoryBaseAddress, - MemoryHob->AllocDescriptor.MemoryLength - )); - - StackBase = MemoryHob->AllocDescriptor.MemoryBaseAddress; - // - // Ensure the base of the stack is page-size aligned. - // - ASSERT ((StackBase & EFI_PAGE_MASK) == 0); - break; - } - - Hob.Raw = GET_NEXT_HOB (Hob); - } - - // - // Ensure the base of stack can be found from Hob when stack guard is - // enabled. - // - ASSERT (StackBase != 0); - } - - DEBUG (( - DEBUG_INFO, - "%a: applying strict permissions to active memory regions\n", - __func__ - )); - - MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize, DescriptorSize); - - MemoryMapEntry = MemoryMap; - MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize); - while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) { - Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type); - if (Attributes != 0) { - SetUefiImageMemoryAttributes ( - MemoryMapEntry->PhysicalStart, - LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT), - Attributes - ); - - // - // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer detection is - // enabled. - // - // MU_CHANGE START: We Enable NULL detection via the function EnableNullDetection - // if (MemoryMapEntry->PhysicalStart == 0 && - // // PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { - // ASSERT (MemoryMapEntry->NumberOfPages > 0); - // SetUefiImageMemoryAttributes ( - // 0, - // EFI_PAGES_TO_SIZE (1), - // EFI_MEMORY_RP | Attributes); - // } - // MU_CHANGE END - - // - // Add EFI_MEMORY_RP attribute for the first page of the stack if stack - // guard is enabled. - // - if ((StackBase != 0) && - ((StackBase >= MemoryMapEntry->PhysicalStart) && - (StackBase < MemoryMapEntry->PhysicalStart + - LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT))) && - // MU_CHANGE START Update to use memory protection settings HOB - // PcdGetBool (PcdCpuStackGuard)) { - gDxeMps.CpuStackGuard) - { - // MU_CHANGE END - SetUefiImageMemoryAttributes ( - StackBase, - EFI_PAGES_TO_SIZE (1), - EFI_MEMORY_RP | Attributes - ); - } - } - - MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize); - } - - FreePool (MemoryMap); - - // - // Apply the policy for RAM regions that we know are present and - // accessible, but have not been added to the UEFI memory map (yet). - // - if (GetPermissionAttributeForMemoryType (EfiConventionalMemory) != 0) { - DEBUG (( - DEBUG_INFO, - "%a: applying strict permissions to inactive memory regions\n", - __func__ - )); - - CoreAcquireGcdMemoryLock (); - - Link = mGcdMemorySpaceMap.ForwardLink; - while (Link != &mGcdMemorySpaceMap) { - Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); - - if ((Entry->GcdMemoryType == EfiGcdMemoryTypeReserved) && - (Entry->EndAddress < MAX_ADDRESS) && - ((Entry->Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) == - (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED))) - { - Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) | - (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK); - - DEBUG (( - DEBUG_INFO, - "Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n", - Entry->BaseAddress, - Entry->EndAddress - Entry->BaseAddress + 1, - Attributes - )); - - ASSERT (gCpu != NULL); - gCpu->SetMemoryAttributes ( - gCpu, - Entry->BaseAddress, - Entry->EndAddress - Entry->BaseAddress + 1, - Attributes - ); - } - - Link = Link->ForwardLink; - } - - CoreReleaseGcdMemoryLock (); - } -} +// MU_CHANGE START: Comment out unused function +// /** +// Remove exec permissions from all regions whose type is identified by +// the Dxe NX Protection Policy. // MU_CHANGE +// **/ +// STATIC +// VOID +// InitializeDxeNxMemoryProtectionPolicy ( +// VOID +// ) +// { +// UINTN MemoryMapSize; +// UINTN MapKey; +// UINTN DescriptorSize; +// UINT32 DescriptorVersion; +// EFI_MEMORY_DESCRIPTOR *MemoryMap; +// EFI_MEMORY_DESCRIPTOR *MemoryMapEntry; +// EFI_MEMORY_DESCRIPTOR *MemoryMapEnd; +// EFI_STATUS Status; +// UINT64 Attributes; +// LIST_ENTRY *Link; +// EFI_GCD_MAP_ENTRY *Entry; +// EFI_PEI_HOB_POINTERS Hob; +// EFI_HOB_MEMORY_ALLOCATION *MemoryHob; +// EFI_PHYSICAL_ADDRESS StackBase; + +// // +// // Get the EFI memory map. +// // +// MemoryMapSize = 0; +// MemoryMap = NULL; + +// Status = gBS->GetMemoryMap ( +// &MemoryMapSize, +// MemoryMap, +// &MapKey, +// &DescriptorSize, +// &DescriptorVersion +// ); +// ASSERT (Status == EFI_BUFFER_TOO_SMALL); +// do { +// MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize); +// // MU_CHANGE [BEGIN] - CodeQL change +// if (MemoryMap == NULL) { +// ASSERT (MemoryMap != NULL); +// return; +// } + +// // MU_CHANGE [END] - CodeQL change +// Status = gBS->GetMemoryMap ( +// &MemoryMapSize, +// MemoryMap, +// &MapKey, +// &DescriptorSize, +// &DescriptorVersion +// ); +// if (EFI_ERROR (Status)) { +// FreePool (MemoryMap); +// } +// } while (Status == EFI_BUFFER_TOO_SMALL); + +// ASSERT_EFI_ERROR (Status); + +// StackBase = 0; +// // MU_CHANGE START Update to use memory protection settings HOB +// // if (PcdGetBool (PcdCpuStackGuard)) { +// if (gDxeMps.CpuStackGuard) { +// // MU_CHANGE END +// // +// // Get the base of stack from Hob. +// // +// Hob.Raw = GetHobList (); +// while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw)) != NULL) { +// MemoryHob = Hob.MemoryAllocation; +// if (CompareGuid (&gEfiHobMemoryAllocStackGuid, &MemoryHob->AllocDescriptor.Name)) { +// DEBUG (( +// DEBUG_INFO, +// "%a: StackBase = 0x%016lx StackSize = 0x%016lx\n", +// __func__, +// MemoryHob->AllocDescriptor.MemoryBaseAddress, +// MemoryHob->AllocDescriptor.MemoryLength +// )); + +// StackBase = MemoryHob->AllocDescriptor.MemoryBaseAddress; +// // +// // Ensure the base of the stack is page-size aligned. +// // +// ASSERT ((StackBase & EFI_PAGE_MASK) == 0); +// break; +// } + +// Hob.Raw = GET_NEXT_HOB (Hob); +// } + +// // +// // Ensure the base of stack can be found from Hob when stack guard is +// // enabled. +// // +// ASSERT (StackBase != 0); +// } + +// DEBUG (( +// DEBUG_INFO, +// "%a: applying strict permissions to active memory regions\n", +// __func__ +// )); + +// MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize, DescriptorSize); + +// MemoryMapEntry = MemoryMap; +// MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize); +// while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) { +// Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type); +// if (Attributes != 0) { +// SetUefiImageMemoryAttributes ( +// MemoryMapEntry->PhysicalStart, +// LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT), +// Attributes +// ); + +// // +// // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer detection is +// // enabled. +// // +// // MU_CHANGE START: We Enable NULL detection via the function EnableNullDetection +// // if (MemoryMapEntry->PhysicalStart == 0 && +// // // PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) { +// // ASSERT (MemoryMapEntry->NumberOfPages > 0); +// // SetUefiImageMemoryAttributes ( +// // 0, +// // EFI_PAGES_TO_SIZE (1), +// // EFI_MEMORY_RP | Attributes); +// // } +// // MU_CHANGE END + +// // +// // Add EFI_MEMORY_RP attribute for the first page of the stack if stack +// // guard is enabled. +// // +// if ((StackBase != 0) && +// ((StackBase >= MemoryMapEntry->PhysicalStart) && +// (StackBase < MemoryMapEntry->PhysicalStart + +// LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT))) && +// // MU_CHANGE START Update to use memory protection settings HOB +// // PcdGetBool (PcdCpuStackGuard)) { +// gDxeMps.CpuStackGuard) +// { +// // MU_CHANGE END +// SetUefiImageMemoryAttributes ( +// StackBase, +// EFI_PAGES_TO_SIZE (1), +// EFI_MEMORY_RP | Attributes +// ); +// } +// } + +// MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize); +// } + +// FreePool (MemoryMap); + +// // +// // Apply the policy for RAM regions that we know are present and +// // accessible, but have not been added to the UEFI memory map (yet). +// // +// if (GetPermissionAttributeForMemoryType (EfiConventionalMemory) != 0) { +// DEBUG (( +// DEBUG_INFO, +// "%a: applying strict permissions to inactive memory regions\n", +// __func__ +// )); + +// CoreAcquireGcdMemoryLock (); + +// Link = mGcdMemorySpaceMap.ForwardLink; +// while (Link != &mGcdMemorySpaceMap) { +// Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE); + +// if ((Entry->GcdMemoryType == EfiGcdMemoryTypeReserved) && +// (Entry->EndAddress < MAX_ADDRESS) && +// ((Entry->Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) == +// (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED))) +// { +// Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) | +// (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK); + +// DEBUG (( +// DEBUG_INFO, +// "Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n", +// Entry->BaseAddress, +// Entry->EndAddress - Entry->BaseAddress + 1, +// Attributes +// )); + +// ASSERT (gCpu != NULL); +// gCpu->SetMemoryAttributes ( +// gCpu, +// Entry->BaseAddress, +// Entry->EndAddress - Entry->BaseAddress + 1, +// Attributes +// ); +// } + +// Link = Link->ForwardLink; +// } + +// CoreReleaseGcdMemoryLock (); +// } +// } +// MU_CHANGE END /** A notification for CPU_ARCH protocol. @@ -879,12 +893,11 @@ MemoryProtectionCpuArchProtocolNotify ( // // Apply the memory protection policy on non-BScode/RTcode regions. // - // MU_CHANGE START Update to use memory protection settings HOB + // MU_CHANGE START: This function is now called after the GCD sync process has completed // if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) { - if (gDxeMps.NxProtectionPolicy.Data) { - // MU_CHANGE END - InitializeDxeNxMemoryProtectionPolicy (); - } + // InitializeDxeNxMemoryProtectionPolicy (); + // } + // MU_CHANGE END // // Call notify function meant for Heap Guard. @@ -1104,14 +1117,15 @@ CoreInitializeMemoryProtection ( ) { EFI_STATUS Status; - EFI_EVENT Event; - EFI_EVENT DisableNullDetectionEvent; - EFI_EVENT EnableNullDetectionEvent; // MU_CHANGE - EFI_EVENT MemoryAttributeProtocolEvent; // MU_CHANGE - VOID *Registration; - - // mImageProtectionPolicy = gDxeMps.ImageProtectionPolicy; // MU_CHANGE - + // MU_CHANGE START: Update to use memory protection settings HOB, + // add support for RP on free memory. + EFI_EVENT DisableNullDetectionEvent; + EFI_EVENT EnableNullDetectionEvent; + EFI_EVENT MemoryAttributeProtocolEvent; + VOID *Registration; + + // mImageProtectionPolicy = gDxeMps.ImageProtectionPolicy; + // MU_CHANGE END InitializeListHead (&mProtectedImageRecordList); // @@ -1120,37 +1134,37 @@ CoreInitializeMemoryProtection ( // - EfiConventionalMemory and EfiBootServicesData should use the // same attribute // - // MU_CHANGE START: We allow code types to have NX + // MU_CHANGE START: We allow code types to have NX and EfiBootServicesData to differ in attributes from + // EfiConventionalMemory // 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) + // ); // MU_CHANGE END - ASSERT ( - GetPermissionAttributeForMemoryType (EfiBootServicesData) == - GetPermissionAttributeForMemoryType (EfiConventionalMemory) - ); - - Status = CoreCreateEvent ( - EVT_NOTIFY_SIGNAL, - // MU_CHANGE START: Use Project Mu Arch Protocol Notify - TPL_CALLBACK - 1, - // MemoryProtectionCpuArchProtocolNotify, - MemoryProtectionCpuArchProtocolNotifyMu, - // MU_CHANGE END - NULL, - &Event - ); - ASSERT_EFI_ERROR (Status); - // - // Register for protocol notifactions on this event - // - Status = CoreRegisterProtocolNotify ( - &gEfiCpuArchProtocolGuid, - Event, - &Registration - ); - ASSERT_EFI_ERROR (Status); + // MU_CHANGE START: Change ordering of GCD sync and memory protection initialization + // Status = CoreCreateEvent ( + // EVT_NOTIFY_SIGNAL, + // TPL_CALLBACK, + // MemoryProtectionCpuArchProtocolNotify, + // NULL, + // &Event + // ); + // ASSERT_EFI_ERROR (Status); + + // // + // // Register for protocol notifactions on this event + // // + // Status = CoreRegisterProtocolNotify ( + // &gEfiCpuArchProtocolGuid, + // Event, + // &Registration + // ); + // ASSERT_EFI_ERROR (Status); + // MU_CHANGE END // MU_CHANGE START: Register an event to populate the memory attribute protocol Status = CoreCreateEvent ( @@ -1172,6 +1186,13 @@ CoreInitializeMemoryProtection ( ); ASSERT_EFI_ERROR (Status); // MU_CHANGE END + + // MU_CHANGE START: Add event to apply page attribues according to the memory protection policy + // after the GCD has synced. + Status = RegisterPageAccessAttributesUpdateOnGcdSyncComplete (); + ASSERT_EFI_ERROR (Status); + // MU_CHANGE END + // // Register a callback to disable NULL pointer detection at EndOfDxe // @@ -1313,7 +1334,10 @@ ApplyMemoryProtectionPolicy ( // permission attributes, and it is the job of the driver that installs this // protocol to set the permissions on existing allocations. // - if (gCpu == NULL) { + // 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) { return EFI_SUCCESS; } diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c index bd127de4f5..bc443aae14 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c @@ -30,6 +30,8 @@ EFI_MEMORY_ATTRIBUTE_PROTOCOL *mMemoryAttributeProtocol = NULL; UINT8 *mBitmapGlobal = NULL; LIST_ENTRY **mArrayOfListEntryPointers = NULL; +extern BOOLEAN mPageAttributesInitialized; + #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))) @@ -1735,8 +1737,9 @@ SetAccessAttributesInMemoryMap ( return EFI_INVALID_PARAMETER; } - MemoryMapEntry = MemoryMap; - MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + *MemoryMapSize); + mPageAttributesInitialized = TRUE; + MemoryMapEntry = MemoryMap; + MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + *MemoryMapSize); while (MemoryMapEntry < MemoryMapEnd) { if (!IS_BITMAP_INDEX_SET (Bitmap, Index)) { @@ -1748,6 +1751,7 @@ SetAccessAttributesInMemoryMap ( MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, *DescriptorSize); } + mPageAttributesInitialized = FALSE; return EFI_SUCCESS; } @@ -2681,8 +2685,6 @@ MemoryProtectionCpuArchProtocolNotifyMu ( goto Done; } - InitializePageAttributesForMemoryProtectionPolicy (); - // // Call notify function meant for Heap Guard. // @@ -2895,3 +2897,61 @@ IsEnhancedMemoryProtectionActive ( { return mEnhancedMemoryProtectionActive; } + +/** + Event function called when gEdkiiGcdSyncCompleteProtocolGuid is + installed to initialize access attributes on tested and untested memory. +**/ +VOID +EFIAPI +InitializePageAttributesCallback ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + InitializePageAttributesForMemoryProtectionPolicy (); + + HeapGuardCpuArchProtocolNotify (); + + mPageAttributesInitialized = TRUE; + + CoreCloseEvent (Event); +} + +/** + Registers a callback on gEdkiiGcdSyncCompleteProtocolGuid to initialize page attributes + in accordance with to the memory protection policy. + + @retval EFI_SUCCESS Event successfully registered + @retval other Event was not registered + */ +EFI_STATUS +EFIAPI +RegisterPageAccessAttributesUpdateOnGcdSyncComplete ( + VOID + ) +{ + EFI_STATUS Status; + EFI_EVENT Event; + VOID *Registration; + + Status = CoreCreateEvent ( + EVT_NOTIFY_SIGNAL, + TPL_CALLBACK, + InitializePageAttributesCallback, + NULL, + &Event + ); + + if (EFI_ERROR (Status)) { + return Status; + } + + Status = CoreRegisterProtocolNotify ( + &gEdkiiGcdSyncCompleteProtocolGuid, + Event, + &Registration + ); + + return Status; +} diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.h b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.h index d6915db394..092384d5e5 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.h +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.h @@ -183,4 +183,17 @@ GetImageList ( IN IMAGE_RANGE_PROTECTION_STATUS ProtectedOrNonProtected ); +/** + Registers a callback on gEdkiiGcdSyncCompleteProtocolGuid to initialize + page attributes in accordance with to the memory protection policy. + + @retval EFI_SUCCESS Event successfully registered + @retval other Event was not registered +**/ +EFI_STATUS +EFIAPI +RegisterPageAccessAttributesUpdateOnGcdSyncComplete ( + VOID + ); + #endif diff --git a/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h b/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h index 4cedd80b37..f455d577e8 100644 --- a/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h +++ b/MdeModulePkg/Include/Guid/DxeMemoryProtectionSettings.h @@ -1,7 +1,9 @@ /** @file - Defines memory protection settings guid and struct +For information on the available settings, see: +https://github.com/microsoft/mu_basecore/tree/HEAD/Docs/feature_memory_protection.md + Copyright (C) Microsoft Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent @@ -22,10 +24,9 @@ typedef union { typedef union { UINT8 Data; struct { - UINT8 UefiPageGuard : 1; - UINT8 UefiPoolGuard : 1; - UINT8 UefiFreedMemoryGuard : 1; - UINT8 Direction : 1; + UINT8 UefiPageGuard : 1; + UINT8 UefiPoolGuard : 1; + UINT8 Direction : 1; } Fields; } DXE_HEAP_GUARD_POLICY; @@ -65,7 +66,7 @@ typedef union { typedef UINT8 DXE_MEMORY_PROTECTION_SETTINGS_VERSION; -#define DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION 6 // Current iteration of DXE_MEMORY_PROTECTION_SETTINGS +#define DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION 7 // Current iteration of DXE_MEMORY_PROTECTION_SETTINGS // // Memory Protection Settings struct @@ -83,6 +84,10 @@ typedef struct { // FALSE - UEFI Stack Guard will be disabled. BOOLEAN CpuStackGuard; + // If enabled, all EfiConventionalMemory will be marked with EFI_MEMORY_RP. This can + // be used in conjunction with the NX setting for EfiConventionalMemory. + BOOLEAN FreeMemoryReadProtected; + // Bitfield to control the NULL address detection in code for different phases. // If enabled, accessing NULL address in UEFI or SMM code can be caught by marking // the NULL page as not present. @@ -107,7 +112,6 @@ typedef struct { // // .UefiPageGuard : Enable UEFI page guard. // .UefiPoolGuard : Enable UEFI pool guard. - // .UefiFreedMemoryGuard : Enable UEFI freed-memory guard (Use-After-Free memory detection). // .Direction : The direction of Guard Page for Pool Guard. // 0 - The returned pool is near the tail guard page. // 1 - The returned pool is near the head guard page. @@ -153,8 +157,6 @@ typedef struct { // // If a bit is set, memory regions of the associated type will be mapped // non-executable. If a bit is cleared, nothing will be done to associated type of memory. - // - // NOTE: - User MUST set the same NX protection for EfiBootServicesData and EfiConventionalMemory. DXE_HEAP_GUARD_MEMORY_TYPES NxProtectionPolicy; // Indicates if stack cookie protection will be enabled @@ -190,6 +192,7 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION, \ TRUE, /* Stack Guard On */ \ + TRUE, /* Free Memory Guard On*/ \ { \ .Fields.UefiNullDetection = 1, \ .Fields.DisableEndOfDxe = 0, \ @@ -198,7 +201,6 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ .Fields.UefiPageGuard = 1, \ .Fields.UefiPoolGuard = 1, \ - .Fields.UefiFreedMemoryGuard = 0, \ .Fields.Direction = 0 \ }, \ { \ @@ -280,6 +282,7 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION, \ TRUE, /* Stack Guard On */ \ + TRUE, /* Free Memory Guard On*/ \ { \ .Fields.UefiNullDetection = 1, \ .Fields.DisableEndOfDxe = 0, \ @@ -288,7 +291,6 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ .Fields.UefiPageGuard = 1, \ .Fields.UefiPoolGuard = 0, \ - .Fields.UefiFreedMemoryGuard = 0, \ .Fields.Direction = 0 \ }, \ { \ @@ -369,6 +371,7 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION, \ TRUE, /* Stack Guard On */ \ + TRUE, /* Free Memory Guard On*/ \ { \ .Fields.UefiNullDetection = 1, \ .Fields.DisableEndOfDxe = 0, \ @@ -377,7 +380,6 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ .Fields.UefiPageGuard = 0, \ .Fields.UefiPoolGuard = 0, \ - .Fields.UefiFreedMemoryGuard = 0, \ .Fields.Direction = 0 \ }, \ { \ @@ -456,7 +458,8 @@ extern GUID gDxeMemoryProtectionSettingsGuid; #define DXE_MEMORY_PROTECTION_SETTINGS_OFF \ { \ DXE_MEMORY_PROTECTION_SETTINGS_CURRENT_VERSION, \ - FALSE, /* Stack Guard On */ \ + FALSE, /* Stack Guard Off */ \ + FALSE, /* Free Memory Guard Off */ \ { \ .Fields.UefiNullDetection = 0, \ .Fields.DisableEndOfDxe = 0, \ @@ -465,7 +468,6 @@ extern GUID gDxeMemoryProtectionSettingsGuid; { \ .Fields.UefiPageGuard = 0, \ .Fields.UefiPoolGuard = 0, \ - .Fields.UefiFreedMemoryGuard = 0, \ .Fields.Direction = 0 \ }, \ { \ diff --git a/MdeModulePkg/Include/Guid/MmMemoryProtectionSettings.h b/MdeModulePkg/Include/Guid/MmMemoryProtectionSettings.h index aaa840f4b2..6baa7275bf 100644 --- a/MdeModulePkg/Include/Guid/MmMemoryProtectionSettings.h +++ b/MdeModulePkg/Include/Guid/MmMemoryProtectionSettings.h @@ -1,7 +1,9 @@ /** @file - Defines memory protection settings guid and struct +For information on the available settings, see: +https://github.com/microsoft/mu_basecore/tree/HEAD/Docs/feature_memory_protection.md + Copyright (C) Microsoft Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent diff --git a/MdeModulePkg/Library/MemoryProtectionHobLib/DxeMemoryProtectionHobLib.c b/MdeModulePkg/Library/MemoryProtectionHobLib/DxeMemoryProtectionHobLib.c index a49ab8c952..fca5366263 100644 --- a/MdeModulePkg/Library/MemoryProtectionHobLib/DxeMemoryProtectionHobLib.c +++ b/MdeModulePkg/Library/MemoryProtectionHobLib/DxeMemoryProtectionHobLib.c @@ -80,22 +80,6 @@ DxeMemoryProtectionSettingsConsistencyCheck ( VOID ) { - if ((gDxeMps.HeapGuardPolicy.Fields.UefiPoolGuard || gDxeMps.HeapGuardPolicy.Fields.UefiPageGuard) && - gDxeMps.HeapGuardPolicy.Fields.UefiFreedMemoryGuard) - { - DEBUG (( - DEBUG_WARN, - "%a: - HeapGuardPolicy.UefiFreedMemoryGuard and \ -UEFI HeapGuardPolicy.UefiPoolGuard/HeapGuardPolicy.UefiPageGuard \ -cannot be active at the same time. Setting all three to ZERO in \ -the memory protection settings global.\n", - __FUNCTION__ - )); - gDxeMps.HeapGuardPolicy.Fields.UefiPoolGuard = 0; - gDxeMps.HeapGuardPolicy.Fields.UefiPageGuard = 0; - gDxeMps.HeapGuardPolicy.Fields.UefiFreedMemoryGuard = 0; - } - if (!gDxeMps.ImageProtectionPolicy.Data && (gDxeMps.NxProtectionPolicy.Fields.EfiLoaderData || gDxeMps.NxProtectionPolicy.Fields.EfiBootServicesData || diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index e5d1bc6917..517128b46a 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -833,6 +833,10 @@ ## Include/Protocol/UsbEthernetProtocol.h gEdkIIUsbEthProtocolGuid = { 0x8d8969cc, 0xfeb0, 0x4303, { 0xb2, 0x1a, 0x1f, 0x11, 0x6f, 0x38, 0x56, 0x43 } } + ## MU_CHANGE START: Add protocol to indicate the GCD sync has completed + ## Include/Protocol/Cpu.h + gEdkiiGcdSyncCompleteProtocolGuid = { 0x650B7F40, 0x6564, 0x4FA9, { 0x93, 0x75, 0xCD, 0x6B, 0x52, 0x1B, 0x6E, 0x50 }} + ## MU_CHANGE END [PcdsFeatureFlag] ## Indicates if the platform can support update capsule across a system reset.

# TRUE - Supports update capsule across a system reset.
diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h index 17566b62ab..9ec18cd8d1 100644 --- a/MdePkg/Include/Library/DebugLib.h +++ b/MdePkg/Include/Library/DebugLib.h @@ -574,12 +574,9 @@ UnitTestDebugAssert ( @param Length The number of bytes in the buffer to set. **/ -#define DEBUG_CLEAR_MEMORY(Address, Length) \ - do { \ - if (DebugClearMemoryEnabled ()) { \ - DebugClearMemory (Address, Length); \ - } \ - } while (FALSE) +// MU_CHANGE START: Disable debug clear memory to support RP on free memory +#define DEBUG_CLEAR_MEMORY(Address, Length) +// MU_CHANGE END /** Macro that calls DebugAssert() if the containing record does not have a diff --git a/MdePkg/Include/Protocol/Cpu.h b/MdePkg/Include/Protocol/Cpu.h index 3d25ad08f8..4d5e58cdd3 100644 --- a/MdePkg/Include/Protocol/Cpu.h +++ b/MdePkg/Include/Protocol/Cpu.h @@ -16,6 +16,13 @@ #define EFI_CPU_ARCH_PROTOCOL_GUID \ { 0x26baccb1, 0x6f42, 0x11d4, {0xbc, 0xe7, 0x0, 0x80, 0xc7, 0x3c, 0x88, 0x81 } } +// MU_CHANGE START: Guid used to signal the completion of GCD sync +#define GCD_SYNC_COMPLETE_PROTOCOL_GUID \ + { 0x650B7F40, 0x6564, 0x4FA9, {0x93, 0x75, 0xCD, 0x6B, 0x52, 0x1B, 0x6E, 0x50 } } + +extern EFI_GUID gEdkiiGcdSyncCompleteProtocolGuid; +// MU_CHANGE_END + typedef struct _EFI_CPU_ARCH_PROTOCOL EFI_CPU_ARCH_PROTOCOL; /// diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c index f973b2e7f8..952e3cefc3 100644 --- a/UefiCpuPkg/CpuDxe/CpuDxe.c +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c @@ -1046,6 +1046,15 @@ InitializeCpu ( // RefreshGcdMemoryAttributes (); + // MU_CHANGE START: Install blank protocol to signal the end of the GCD sync + gBS->InstallMultipleProtocolInterfaces ( + &ImageHandle, + &gEdkiiGcdSyncCompleteProtocolGuid, + NULL, + NULL + ); + // MU_CHANGE END + // // Add and allocate local APIC memory mapped space // diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf index dfc41ae353..bc050e0d69 100644 --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf @@ -68,6 +68,7 @@ gEfiMemoryAttributeProtocolGuid ## TCBZ3519 MU_CHANGE PRODUCES gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES gMemoryProtectionNonstopModeProtocolGuid ## MU_CHANGE: PRODUCES + gEdkiiGcdSyncCompleteProtocolGuid ## MU_CHANGE: PRODUCES [Guids] gIdleLoopEventGuid ## CONSUMES ## Event From 9345b79d6fc01798450d5aa5946bf7c1ffe3858c Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Fri, 26 Apr 2024 15:37:28 -0700 Subject: [PATCH 2/5] Return if Allocation Fails When Manipulating Guarded Memory Map Description This PR updates a set of ASSERTs checking if memory was successfully allocated to branch if the allocation failed in addition to ASSERTing. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] 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 booting Q35 to shell Integration Instructions N/A --- MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c index df4a627529..7651731087 100644 --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c @@ -252,8 +252,14 @@ FindGuardedMemoryMap ( &MapMemory, FALSE ); - ASSERT_EFI_ERROR (Status); - ASSERT (MapMemory != 0); + // MU_CHANGE START: Check if memory was successfully allocated + if (EFI_ERROR (Status) || (MapMemory == 0)) { + ASSERT_EFI_ERROR (Status); + ASSERT (MapMemory != 0); + return 0; + } + + // MU_CHANGE END // MU_CHANGE START: Apply Protection policy to the allocated memory ApplyMemoryProtectionPolicy ( EfiConventionalMemory, @@ -291,8 +297,14 @@ FindGuardedMemoryMap ( &MapMemory, FALSE ); - ASSERT_EFI_ERROR (Status); - ASSERT (MapMemory != 0); + // MU_CHANGE START: Check if memory was successfully allocated + if (EFI_ERROR (Status) || (MapMemory == 0)) { + ASSERT_EFI_ERROR (Status); + ASSERT (MapMemory != 0); + return 0; + } + + // MU_CHANGE END // MU_CHANGE START: Apply Protection policy to the allocated memory ApplyMemoryProtectionPolicy ( EfiConventionalMemory, From 27f20fcc76ef920243b41fafba19fd1293199b57 Mon Sep 17 00:00:00 2001 From: Taylor Beebe <31827475+TaylorBeebe@users.noreply.github.com> Date: Fri, 26 Apr 2024 17:00:22 -0700 Subject: [PATCH 3/5] Add EFI Event When Compatibility Mode is Triggered (#832) ## Description For a description of Memory Protection Compatibility Mode, see: https://microsoft.github.io/mu/WhatAndWhy/enhancedmemoryprotection/ This PR adds an event which will be signaled when Compatibility Mode is activated. Platforms can hook into this event to perform custom actions when this mode is activated. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] 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 booting Q35 to shell. ## Integration Instructions N/A --- MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + .../Core/Dxe/Misc/MemoryProtectionSupport.c | 1 + .../Guid/CompatibilityModeActivatedEvent.h | 18 ++++++++++++++++++ MdeModulePkg/MdeModulePkg.dec | 5 +++++ 4 files changed, 25 insertions(+) create mode 100644 MdeModulePkg/Include/Guid/CompatibilityModeActivatedEvent.h diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf index 1608117dbe..903e64099b 100644 --- a/MdeModulePkg/Core/Dxe/DxeMain.inf +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf @@ -131,6 +131,7 @@ gMuEventPreExitBootServicesGuid ## PRODUCES ## Event // MU_CHANGE gDxeMemoryProtectionSettingsGuid ## CONSUMES ## HOB // MU_CHANGE gMemoryProtectionSpecialRegionHobGuid ## CONSUMES ## HOB // MU_CHANGE + gCompatibilityModeActivatedEventGuid ## SOMETIMES_PRODUCES ## Event // MU_CHANGE [Ppis] gEfiVectorHandoffInfoPpiGuid ## UNDEFINED # HOB diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c index bc443aae14..c2dca192e6 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c @@ -2884,6 +2884,7 @@ ActivateCompatibilityMode ( DisableNullDetection (); UninstallMemoryAttributeProtocol (); MapLegacyBiosMemoryRWX (); + CoreNotifySignalList (&gCompatibilityModeActivatedEventGuid); } /** diff --git a/MdeModulePkg/Include/Guid/CompatibilityModeActivatedEvent.h b/MdeModulePkg/Include/Guid/CompatibilityModeActivatedEvent.h new file mode 100644 index 0000000000..ea6888eb45 --- /dev/null +++ b/MdeModulePkg/Include/Guid/CompatibilityModeActivatedEvent.h @@ -0,0 +1,18 @@ +/** @file + Event group triggered when Memory Protection Compatibility Mode is activated. + Platforms can hook into this event to perform follow up actions. + More information on Compatibility Mode: https://microsoft.github.io/mu/WhatAndWhy/enhancedmemoryprotection/ + + Copyright (c) Microsoft Corporation. + SPDX-License-Identifier: BSD-2-Clause-Patent +**/ + +#ifndef COMPATIBILITY_MODE_ACTIVATED_EVENT_H_ +#define COMPATIBILITY_MODE_ACTIVATED_EVENT_H_ + +#define COMPATIBILITY_MODE_ACTIVATED_EVENT_GUID \ + { 0x209BA820, 0xCE32, 0x49E1, { 0x95, 0xAE, 0x39, 0x43, 0x3D, 0xEF, 0xD5, 0xEE } } + +extern EFI_GUID gCompatibilityModeActivatedEventGuid; + +#endif diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 517128b46a..3c1feae13a 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -510,6 +510,11 @@ # Include/Guid/MemoryProtectionSpecialRegionGuid.h gMemoryProtectionSpecialRegionHobGuid = { 0xBFCC1325, 0x3BFE, 0x469A, { 0x9A, 0xAC, 0x0C, 0x99, 0x0E, 0x4E, 0xC9, 0xF1 } } + # MU_CHANGE + # This GUID is used to trigger the event group when the compatibility mode is activated. + # Include/Guid/CompatibilityModeActivatedEvent.h + gCompatibilityModeActivatedEventGuid = { 0x209BA820, 0xCE32, 0x49E1, { 0x95, 0xAE, 0x39, 0x43, 0x3D, 0xEF, 0xD5, 0xEE } } + ## MSCHANGE END From d175925e30b53112529acbd9822a1d4b350d60bd Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Thu, 25 Apr 2024 11:53:32 -0700 Subject: [PATCH 4/5] Print Failure Address When Stack Cookie Check Fails Description Use the RETURN_ADDRESS macro to print the failure address of the stack cookie failure. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] 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 triggering a stack check failure Integration Instructions N/A --- MdePkg/Library/StackCheckLib/StackCheckLibCommon.c | 3 +++ MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf | 1 + MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf | 1 + 3 files changed, 5 insertions(+) diff --git a/MdePkg/Library/StackCheckLib/StackCheckLibCommon.c b/MdePkg/Library/StackCheckLib/StackCheckLibCommon.c index 0bfa4581a0..4baaf0f156 100644 --- a/MdePkg/Library/StackCheckLib/StackCheckLibCommon.c +++ b/MdePkg/Library/StackCheckLib/StackCheckLibCommon.c @@ -8,6 +8,7 @@ #include +#include #include #include @@ -28,6 +29,7 @@ __stack_chk_fail ( VOID ) { + DEBUG ((DEBUG_ERROR, "Stack cookie check failed at address 0x%llx!\n", RETURN_ADDRESS (0))); StackCheckFailureHook (RETURN_ADDRESS (0)); TriggerStackCookieInterrupt (); } @@ -40,6 +42,7 @@ StackCheckFailure ( VOID *ActualCookieValue ) { + DEBUG ((DEBUG_ERROR, "Stack cookie check failed at address 0x%llx!\n", RETURN_ADDRESS (0))); StackCheckFailureHook (RETURN_ADDRESS (0)); TriggerStackCookieInterrupt (); } diff --git a/MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf b/MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf index e358f78694..9bd27ec64c 100644 --- a/MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf +++ b/MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf @@ -43,6 +43,7 @@ RngLib StackCheckFailureHookLib BaseLib + DebugLib [FixedPcd] gEfiMdePkgTokenSpaceGuid.PcdStackCookieExceptionVector diff --git a/MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf b/MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf index 0bc899dfa6..fdccd73f08 100644 --- a/MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf +++ b/MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf @@ -39,6 +39,7 @@ [LibraryClasses] StackCheckFailureHookLib BaseLib + DebugLib [FixedPcd] gEfiMdePkgTokenSpaceGuid.PcdStackCookieExceptionVector From 9dd0624e63c9a3a7513a30320da2dfca23e7f902 Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Thu, 25 Apr 2024 11:53:56 -0700 Subject: [PATCH 5/5] Add StackCheckLib Readme Description Add a readme file for the StackCheckLib library. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] 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, ... - [x] 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 N/A Integration Instructions N/A --- MdePkg/Library/StackCheckLib/Readme.md | 124 +++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 MdePkg/Library/StackCheckLib/Readme.md diff --git a/MdePkg/Library/StackCheckLib/Readme.md b/MdePkg/Library/StackCheckLib/Readme.md new file mode 100644 index 0000000000..07b16b4197 --- /dev/null +++ b/MdePkg/Library/StackCheckLib/Readme.md @@ -0,0 +1,124 @@ +# StackCheckLib + +## Table of Contents + +- [StackCheckLib](#stackchecklib) + - [Table of Contents](#table-of-contents) + - [Introduction and Library Instances](#introduction-and-library-instances) + - [StackCheckLibStaticInit](#stackchecklibstaticinit) + - [StackCheckLibDynamicInit](#stackchecklibdynamicinit) + - [StackCheckLibNull](#stackchecklibnull) + - [How Failures are Handled](#how-failures-are-handled) + - [Debugging Stack Cookie Check Failures](#debugging-stack-cookie-check-failures) + - [Usage](#usage) + +## Introduction and Library Instances + +`StackCheckLib` contains the required functionality for initializing the stack cookie +value, checking the value, and triggering an interrupt when a mismatch occurs. +The stack cookie is a random value placed on the stack between the stack variables +and the return address so that continuously writing past the stack variables will +cause the stack cookie to be overwritten. Before the function returns, the stack +cookie value will be checked and if there is a mismatch then `StackCheckLib` handles +the failure. + +Because UEFI doesn't use the C runtime libraries provided by MSVC, the stack +check code is written in assembly within this library. GCC and Clang compilers +have built-in support for stack cookie checking, so this library only handles failures. + +### StackCheckLibStaticInit + +`StackCheckLibStaticInit` is an instance of `StackCheckLib` which does not update the +stack cookie value for the module at runtime. It's always preferable to use +`StackCheckLibDynamicInit` for improved security but there are cases where the stack +cookie global cannot be written to such as in execute-in-place (XIP) modules and during +the Cache-as-RAM (CAR) phase of the boot process. The stack cookie value is initialized +at compile time via Project Mu updates to the AutoGen process. Each module will define +`STACK_COOKIE_VALUE` which is used for the module stack cookie value. + +### StackCheckLibDynamicInit + +`StackCheckLibDynamicInit` is an instance of `StackCheckLib` which updates the stack +cookie value for the module at runtime. This is the preferred method for stack cookie +initialization as it provides improved security. The stack cookie value is initialized +at runtime by calling `GetRandomNumber32()` or `GetRandomNumber64()` to generate a random +value via the platform's random number generator protocol. If the random number generator +returns an error, then the value will still have the build-time randomized value to fall +back on. + +### StackCheckLibNull + +`StackCheckLibNull` is an instance of `StackCheckLib` which does not perform any stack +cookie checks. This is useful for modules which will fail if stack cookie checks are +inserted. Of course, this is not recommended for production code. + +## How Failures are Handled + +When a stack cookie check fails, the `StackCheckLib` library will first call into a hook +function `StackCheckFailureHook()` which only has a NULL implementation in Project Mu. +The NULL implementation will simply print the failure address and return, but a platform +can implement their own instance of this library which can perform additional actions +before the system triggers an interrupt. + +After `StackCheckFailureHook()` returns, the library will trigger an interrupt with +PcdStackCookieExceptionVector. + +- On IA32 and X64 platforms, PcdStackCookieExceptionVector is used as an index into the +Interrupt Descriptor Table. +- On ARM platforms, a software interrupt (`SWI`) is called with the value of +PcdStackCookieExceptionVector. The value can be retrieved by the handler by reading +bits [7:0] of the instruction opcode which will allow the handler to determine if the +interrupt was triggered by the stack cookie check. Reference: +[Arm A64 Instruction Set Architecture Version 2024-3](https://developer.arm.com/documentation/ddi0597/2024-03/Base-Instructions/SVC--Supervisor-Call-?lang=en) +- On AARCH64 platforms, a supervisor call (`SVC`) is called with the value +of PcdStackCookieExceptionVector. This value can similarly be retrieved by the +handler to determine if the interrupt was triggered by the stack cookie check. Reference: +[Arm A64 Instruction Set Architecture Version 2024-3](https://developer.arm.com/documentation/ddi0602/2024-03/Base-Instructions/SVC--Supervisor-Call-?lang=en) + +## Debugging Stack Cookie Check Failures + +Tracking down the origin of stack cookie failures can be difficult. Programmers may attempt +printf debugging to determine which function has an overflow only to find that the failure +disappears on the next boot. This curiosity is usually due to the black-box heuristic used +by compilers to determine where to put stack cookie checks or compiler optimization features +removing the failing check. The address where the failed stack cookie check occurred will +be printed using DebugLib. If .map files are available, the address combined with the image +offset can be used to determine the function which failed. + +GNU-based compilers have the `-fstack-protector-all` flag to force stack cookie checks on +all functions which could create a more consistent environment for debugging assuming an +earlier failure doesn't mask the targeted one and the flash space can accommodate the +increased size. + +The Visual Studio (MSVC) toolchain has the ability to generate `.cod` files during compilation +which interleave C and the generated assembly code. These files will contain the stack cookie +checks and are useful for determining where the checks are placed. To generate these files, +append `/FAcs` to the build options for each target module. The easiest way to do this is to +update the tools_def file so the `___CC_FLAGS` includes `/FAcs`. + +## Usage + +Project Mu updated the tools_def to add `/GS` to VS2022 and VS2019 IA32/X64 builds and +`-fstack-protector` to GCC builds. This will cause stack cookie references to be inserted +throughout the code. Every module should have a `StackCheckLib` instances linked to satisfy +these references. So every module doesn't need to add `StackCheckLib` to the LibraryClasses +section of the INF file, `StackCheckLib` instances should be linked as NULL in the platform +DSC fies. The only exception to this is host-based unit tests as they will be compiled with +the runtime libraries which already contain the stack cookie definitions and will collide +with `StackCheckLib`. + +SEC and PEI_CORE modules should always use `StackCheckLibNull` and pre-memory modules +should use `StackCheckLibStaticInit`. All other modules should use `StackCheckLibDynamicInit`. +Below is an **example** of how to link the `StackCheckLib` instances in the platform DSC file +but it may need customization based on the platform's requirements: + +```text +[LibraryClasses.common.SEC, LibraryClasses.common.PEI_CORE] + NULL|MdePkg/Library/StackCheckLibNull/StackCheckLibNull.inf + +[LibraryClasses.common.PEIM] + NULL|MdePkg/Library/StackCheckLib/StackCheckLibStaticInit.inf + +[LibraryClasses.common.MM_CORE_STANDALONE, LibraryClasses.common.MM_STANDALONE, LibraryClasses.common.DXE_CORE, LibraryClasses.common.SMM_CORE, LibraryClasses.common.DXE_SMM_DRIVER, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_SAL_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION] + NULL|MdePkg/Library/StackCheckLib/StackCheckLibDynamicInit.inf +```