diff options
author | Taylor Beebe <tabeebe@microsoft.com> | 2023-06-29 09:17:57 -0700 |
---|---|---|
committer | mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> | 2023-07-03 14:29:32 +0000 |
commit | 3b74b0394d5ba50b1a465f8102bfa7a5033f4994 (patch) | |
tree | 0115bd894ceafaac5f5efc0013052a70f7021496 | |
parent | 1b25a7049c4a2621da48d19817b2053217e65478 (diff) | |
download | edk2-3b74b0394d5ba50b1a465f8102bfa7a5033f4994.zip edk2-3b74b0394d5ba50b1a465f8102bfa7a5033f4994.tar.gz edk2-3b74b0394d5ba50b1a465f8102bfa7a5033f4994.tar.bz2 |
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 <t@taylorbeebe.com>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
-rw-r--r-- | ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 21 | ||||
-rw-r--r-- | ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 36 |
2 files changed, 45 insertions, 12 deletions
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c index 0d3bc28..d9d386d 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 268c0bf..5a2f36d 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);
|