From 3b74b0394d5ba50b1a465f8102bfa7a5033f4994 Mon Sep 17 00:00:00 2001 From: Taylor Beebe Date: Thu, 29 Jun 2023 09:17:57 -0700 Subject: ArmPkg: Fix Unsafe ASSERTs in MMU Logic There are ASSERTs present in the MMU logic to ensure various functions return successfully, but these ASSERTs may be ignored on release builds causing unsafe behavior. This patch updates the logic to handle unexpected return values and branch safely. Signed-off-by: Taylor Beebe Reviewed-by: Ard Biesheuvel --- ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 21 +++++++++++++++------ ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 36 ++++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 12 deletions(-) (limited to 'ArmPkg/Drivers') diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c index 0d3bc28096..d9d386dbed 100644 --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c @@ -148,10 +148,12 @@ GetNextEntryAttribute ( // Get the memory space map from GCD MemorySpaceMap = NULL; Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); - ASSERT_EFI_ERROR (Status); - // We cannot get more than 3-level page table - ASSERT (TableLevel <= 3); + if (EFI_ERROR (Status) || (TableLevel > 3)) { + ASSERT_EFI_ERROR (Status); + ASSERT (TableLevel <= 3); + return 0; + } // While the top level table might not contain TT_ENTRY_COUNT entries; // the subsequent ones should be filled up @@ -243,7 +245,11 @@ SyncCacheConfig ( // MemorySpaceMap = NULL; Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); - ASSERT_EFI_ERROR (Status); + + if (EFI_ERROR (Status)) { + ASSERT_EFI_ERROR (Status); + return Status; + } // The GCD implementation maintains its own copy of the state of memory space attributes. GCD needs // to know what the initial memory space attributes are. The CPU Arch. Protocol does not provide a @@ -277,7 +283,7 @@ SyncCacheConfig ( ); // Update GCD with the last region if valid - if (PageAttribute != INVALID_ENTRY) { + if ((PageAttribute != INVALID_ENTRY) && (EndAddressGcdRegion > BaseAddressGcdRegion)) { SetGcdMemorySpaceAttributes ( MemorySpaceMap, NumberOfDescriptors, @@ -430,7 +436,10 @@ GetMemoryRegion ( UINTN EntryCount; UINTN T0SZ; - ASSERT ((BaseAddress != NULL) && (RegionLength != NULL) && (RegionAttributes != NULL)); + if ((BaseAddress == NULL) || (RegionLength == NULL) || (RegionAttributes == NULL)) { + ASSERT ((BaseAddress != NULL) && (RegionLength != NULL) && (RegionAttributes != NULL)); + return EFI_INVALID_PARAMETER; + } TranslationTable = ArmGetTTBR0BaseAddress (); diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c index 268c0bf3f9..5a2f36d060 100644 --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c @@ -217,7 +217,10 @@ SyncCacheConfigPage ( } else if (PageAttributes != NextPageAttributes) { // Convert Section Attributes into GCD Attributes Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes); - ASSERT_EFI_ERROR (Status); + if (EFI_ERROR (Status)) { + ASSERT_EFI_ERROR (Status); + GcdAttributes = 0; + } // update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK) SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes); @@ -230,7 +233,10 @@ SyncCacheConfigPage ( } else if (NextPageAttributes != 0) { // Convert Page Attributes into GCD Attributes Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes); - ASSERT_EFI_ERROR (Status); + if (EFI_ERROR (Status)) { + ASSERT_EFI_ERROR (Status); + GcdAttributes = 0; + } // update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK) SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes); @@ -278,7 +284,12 @@ SyncCacheConfig ( // MemorySpaceMap = NULL; Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap); - ASSERT_EFI_ERROR (Status); + + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - GetMemorySpaceMap() failed! Status: %r\n", Status)); + ASSERT_EFI_ERROR (Status); + return Status; + } // The GCD implementation maintains its own copy of the state of memory space attributes. GCD needs // to know what the initial memory space attributes are. The CPU Arch. Protocol does not provide a @@ -307,7 +318,12 @@ SyncCacheConfig ( } else if (SectionAttributes != NextSectionAttributes) { // Convert Section Attributes into GCD Attributes Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes); - ASSERT_EFI_ERROR (Status); + + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status)); + ASSERT_EFI_ERROR (Status); + GcdAttributes = 0; + } // update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK) SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes); @@ -343,7 +359,11 @@ SyncCacheConfig ( if (NextSectionAttributes != 0) { // Convert Section Attributes into GCD Attributes Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes); - ASSERT_EFI_ERROR (Status); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status)); + ASSERT_EFI_ERROR (Status); + GcdAttributes = 0; + } // update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK) SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes); @@ -360,7 +380,11 @@ SyncCacheConfig ( if (NextSectionAttributes != 0) { // Convert Section Attributes into GCD Attributes Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes); - ASSERT_EFI_ERROR (Status); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status)); + ASSERT_EFI_ERROR (Status); + GcdAttributes = 0; + } // update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK) SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes); -- cgit v1.2.3