From ef5218ab095e2d86b1903d62185ba9892607663f Mon Sep 17 00:00:00 2001 From: Taylor Beebe <31827475+TaylorBeebe@users.noreply.github.com> Date: Fri, 8 Dec 2023 12:56:20 -0800 Subject: [PATCH] DxePagingAudit: Update MemoryOutsideEfiMemoryMapIsInaccessible Test (#381) ## Description 1. MemoryOutsideEfiMemoryMapIsInaccessible checks if memory not present in the EFI memory map has the EFI_MEMORY_RP attribute. The previous version of this test assumed that the memory range spanned by the EFI memory map was contiguous which is sometimes not the case on platforms. This update changes the flow of the test to look at interstitial gaps in the memory map and not just those on the flanks. 2. The EFI memory map returned through the boot services table is sometimes out of order. This update sorts the memory map and memory space map whenever they're populated for a test. 3. The X64 MemoryOutsideEfiMemoryMapIsInaccessible HTML test had a typo which is fixed in this update. - [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, ... - [x] 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 on Q35 and ARM ## Integration Instructions N/A --- .../UEFI/Dxe/App/DxePagingAuditTestApp.c | 103 +++++++++++++----- .../PagingAudit/UEFI/PagingAuditCommon.c | 4 +- .../PagingAudit/UEFI/PagingAuditCommon.h | 32 ++++++ .../Windows/DxePaging_template_X64.html | 2 +- 4 files changed, 108 insertions(+), 33 deletions(-) diff --git a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c index 5be101ff0d..16165bf622 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c +++ b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c @@ -353,6 +353,8 @@ PopulateMemorySpaceMap ( mMemorySpaceMap = NULL; } + SortMemorySpaceMap (mMemorySpaceMap, mMemorySpaceMapCount, sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR)); + return Status; } @@ -424,6 +426,8 @@ PopulateEfiMemoryMap ( } } while (Status == EFI_BUFFER_TOO_SMALL); + SortMemoryMap (mEfiMemoryMap, mEfiMemoryMapSize, mEfiMemoryMapDescriptorSize); + return Status; } @@ -1473,13 +1477,12 @@ MemoryOutsideEfiMemoryMapIsInaccessible ( { UINT64 StartOfAddressSpace; UINT64 EndOfAddressSpace; - UINT64 StartOfEfiMemoryMap; - UINT64 EndOfEfiMemoryMap; - EFI_MEMORY_DESCRIPTOR *FinalEfiMemoryMapDescriptor; - UINTN Index; + EFI_MEMORY_DESCRIPTOR *EndOfEfiMemoryMap; + EFI_MEMORY_DESCRIPTOR *CurrentEfiMemoryMapEntry; BOOLEAN TestFailure; - UINT64 StartOfMapEntry; - UINT64 EndOfMapEntry; + EFI_PHYSICAL_ADDRESS LastMemoryMapEntryEnd; + UINT64 Attributes; + EFI_STATUS Status; DEBUG ((DEBUG_INFO, "%a Enter...\n", __FUNCTION__)); @@ -1491,35 +1494,75 @@ MemoryOutsideEfiMemoryMapIsInaccessible ( UT_ASSERT_NOT_NULL (mMap.Entries); StartOfAddressSpace = mMemorySpaceMap[0].BaseAddress; - EndOfAddressSpace = mMemorySpaceMap[mMemorySpaceMapCount - 1].BaseAddress + mMemorySpaceMap[mMemorySpaceMapCount - 1].Length; - TestFailure = FALSE; - - StartOfEfiMemoryMap = mEfiMemoryMap->PhysicalStart; - FinalEfiMemoryMapDescriptor = (EFI_MEMORY_DESCRIPTOR *)(((UINT8 *)mEfiMemoryMap + mEfiMemoryMapSize) - mEfiMemoryMapDescriptorSize); - EndOfEfiMemoryMap = FinalEfiMemoryMapDescriptor->PhysicalStart + (FinalEfiMemoryMapDescriptor->NumberOfPages * EFI_PAGE_SIZE); - - for (Index = 0; Index < mMap.EntryCount; Index++) { - StartOfMapEntry = mMap.Entries[Index].LinearAddress; - EndOfMapEntry = mMap.Entries[Index].LinearAddress + mMap.Entries[Index].Length; - if (CHECK_OVERLAP (StartOfMapEntry, EndOfMapEntry, StartOfAddressSpace, StartOfEfiMemoryMap)) { - if (IsPageReadable (mMap.Entries[Index].PageEntry)) { - UT_LOG_ERROR ( - "Memory Range 0x%llx-0x%llx is accessible\n", - StartOfMapEntry, - EndOfMapEntry - ); - TestFailure = TRUE; - } - } else if (CHECK_OVERLAP (StartOfMapEntry, EndOfMapEntry, EndOfEfiMemoryMap, EndOfAddressSpace)) { - if (IsPageReadable (mMap.Entries[Index].PageEntry)) { + EndOfAddressSpace = mMemorySpaceMap[mMemorySpaceMapCount - 1].BaseAddress + + mMemorySpaceMap[mMemorySpaceMapCount - 1].Length; + TestFailure = FALSE; + EndOfEfiMemoryMap = (EFI_MEMORY_DESCRIPTOR *)(((UINT8 *)mEfiMemoryMap + mEfiMemoryMapSize)); + CurrentEfiMemoryMapEntry = mEfiMemoryMap; + + if (CurrentEfiMemoryMapEntry->PhysicalStart > StartOfAddressSpace) { + Attributes = 0; + Status = GetRegionCommonAccessAttributes ( + &mMap, + StartOfAddressSpace, + CurrentEfiMemoryMapEntry->PhysicalStart - StartOfAddressSpace, + &Attributes + ); + + if ((Status != EFI_NOT_FOUND) && ((Attributes & EFI_MEMORY_RP) == 0)) { + UT_LOG_ERROR ( + "Memory Range 0x%llx-0x%llx is not EFI_MEMORY_RP\n", + StartOfAddressSpace, + CurrentEfiMemoryMapEntry->PhysicalStart + ); + TestFailure = TRUE; + } + } + + LastMemoryMapEntryEnd = CurrentEfiMemoryMapEntry->PhysicalStart + + (CurrentEfiMemoryMapEntry->NumberOfPages * EFI_PAGE_SIZE); + CurrentEfiMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (CurrentEfiMemoryMapEntry, mEfiMemoryMapDescriptorSize); + + while ((UINTN)CurrentEfiMemoryMapEntry < (UINTN)EndOfEfiMemoryMap) { + if (CurrentEfiMemoryMapEntry->PhysicalStart > LastMemoryMapEntryEnd) { + Attributes = 0; + Status = GetRegionCommonAccessAttributes ( + &mMap, + LastMemoryMapEntryEnd, + CurrentEfiMemoryMapEntry->PhysicalStart - LastMemoryMapEntryEnd, + &Attributes + ); + if ((Status != EFI_NOT_FOUND) && ((Attributes & EFI_MEMORY_RP) == 0)) { UT_LOG_ERROR ( - "Memory Range 0x%llx-0x%llx is accessible\n", - StartOfMapEntry, - EndOfMapEntry + "Memory Range 0x%llx-0x%llx is not EFI_MEMORY_RP\n", + LastMemoryMapEntryEnd, + CurrentEfiMemoryMapEntry->PhysicalStart ); TestFailure = TRUE; } } + + LastMemoryMapEntryEnd = CurrentEfiMemoryMapEntry->PhysicalStart + + (CurrentEfiMemoryMapEntry->NumberOfPages * EFI_PAGE_SIZE); + CurrentEfiMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (CurrentEfiMemoryMapEntry, mEfiMemoryMapDescriptorSize); + } + + if (LastMemoryMapEntryEnd < EndOfAddressSpace) { + Attributes = 0; + Status = GetRegionCommonAccessAttributes ( + &mMap, + LastMemoryMapEntryEnd, + EndOfAddressSpace - LastMemoryMapEntryEnd, + &Attributes + ); + if ((Status != EFI_NOT_FOUND) && ((Attributes & EFI_MEMORY_RP) == 0)) { + UT_LOG_ERROR ( + "Memory Range 0x%llx-0x%llx is not EFI_MEMORY_RP\n", + LastMemoryMapEntryEnd, + EndOfAddressSpace + ); + TestFailure = TRUE; + } } UT_ASSERT_FALSE (TestFailure); diff --git a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.c b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.c index 4c6c4e261e..5fdcaa3a0e 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.c +++ b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.c @@ -387,8 +387,8 @@ MemoryAttributesTableDump ( @param[in] DescriptorSize Size, in bytes, of each descriptor region in the array NOTE: This is not sizeof (EFI_MEMORY_DESCRIPTOR) **/ -STATIC VOID +EFIAPI SortMemoryMap ( IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap, IN UINTN MemoryMapSize, @@ -428,8 +428,8 @@ SortMemoryMap ( @param[in] MemoryMapSize Size, in bytes, of the MemoryMap buffer @param[in] DescriptorSize Size, in bytes, of an individual EFI_GCD_MEMORY_SPACE_DESCRIPTOR **/ -STATIC VOID +EFIAPI SortMemorySpaceMap ( IN OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemoryMap, IN UINTN MemoryMapSize, diff --git a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.h b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.h index 1400de68e5..8d74cd1656 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.h +++ b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/PagingAuditCommon.h @@ -200,4 +200,36 @@ DumpPlatforminfo ( VOID ); +/** + Sort memory map entries based upon PhysicalStart, from low to high. + + @param[in, out] MemoryMap A pointer to the buffer in which firmware places + the current memory map + @param[in] MemoryMapSize Size, in bytes, of the MemoryMap buffer + @param[in] DescriptorSize Size, in bytes, of each descriptor region in the array + NOTE: This is not sizeof (EFI_MEMORY_DESCRIPTOR) +**/ +VOID +EFIAPI +SortMemoryMap ( + IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap, + IN UINTN MemoryMapSize, + IN UINTN DescriptorSize + ); + +/** + Sort memory map entries based upon PhysicalStart, from low to high. + + @param[in, out] MemoryMap A pointer to the buffer containing the current memory map + @param[in] MemoryMapSize Size, in bytes, of the MemoryMap buffer + @param[in] DescriptorSize Size, in bytes, of an individual EFI_GCD_MEMORY_SPACE_DESCRIPTOR +**/ +VOID +EFIAPI +SortMemorySpaceMap ( + IN OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemoryMap, + IN UINTN MemoryMapSize, + IN UINTN DescriptorSize + ); + #endif // _PAGING_AUDIT_COMMON_H_ diff --git a/UefiTestingPkg/AuditTests/PagingAudit/Windows/DxePaging_template_X64.html b/UefiTestingPkg/AuditTests/PagingAudit/Windows/DxePaging_template_X64.html index 8403a9c241..866e776c05 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/Windows/DxePaging_template_X64.html +++ b/UefiTestingPkg/AuditTests/PagingAudit/Windows/DxePaging_template_X64.html @@ -659,7 +659,7 @@

External Licenses

"Description": "Memory not in the EFI memory map should cause a fault if accessed", "Filter": function (mrObject) { var isTargetType = mrObject["Memory Type"] === "None"; - var hasInvalidAttributes = mrObject["Present"] !== "Yes"; + var hasInvalidAttributes = mrObject["Present"] !== "No"; return isTargetType && hasInvalidAttributes; }, //end of Filter function "ConfigureFilter": function () {