[edk2-devel] [PATCH] MdeModulePkg/DxeCore: Consistent DebugImageInfoTable updates

[Marvin Häuser]

In theory, modifications to the DebugImageInfoTable may cause
exceptions. If the exception handler parses the table, this can lead
to subsequent exceptions if the table state is inconsistent.

Ensure the DebugImageInfoTable remains consistent during
modifications. This includes:
1) Free the old table only only after the new table has been
published. Mitigates use-after-free of the old table.
2) Do not insert an image entry till it is fully initialised. Entries
may be inserted in the live range if an entry was deleted previously.
Mitigaes the usage of inconsistent entries.
3) Free the old image entry only after the table has been updated
with the NULL value. Mitigates use-after-free of the old entry.
4) Set the MODIFIED state before performing any modifications.

Cc: Jian J Wang <jian.j.wang at intel.com>
Cc: Hao A Wu <hao.a.wu at intel.com>
Cc: Dandan Bi <dandan.bi at intel.com>
Cc: Liming Gao <gaoliming at byosoft.com.cn>
Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser at posteo.de>
---
 MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c | 60 +++++++++++---------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
index a75d4158280b..7bd970115111 100644
--- a/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
+++ b/MdeModulePkg/Core/Dxe/Misc/DebugImageInfo.c
@@ -165,10 +165,11 @@ CoreNewDebugImageInfoEntry (
   IN  EFI_HANDLE                  ImageHandle

   )

 {

-  EFI_DEBUG_IMAGE_INFO      *Table;

-  EFI_DEBUG_IMAGE_INFO      *NewTable;

-  UINTN                     Index;

-  UINTN                     TableSize;

+  EFI_DEBUG_IMAGE_INFO        *Table;

+  EFI_DEBUG_IMAGE_INFO        *NewTable;

+  UINTN                       Index;

+  UINTN                       TableSize;

+  EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;

 

   //

   // Set the flag indicating that we're in the process of updating the table.

@@ -203,14 +204,6 @@ CoreNewDebugImageInfoEntry (
     // Copy the old table into the new one

     //

     CopyMem (NewTable, Table, TableSize);

-    //

-    // Free the old table

-    //

-    CoreFreePool (Table);

-    //

-    // Update the table header

-    //

-    Table = NewTable;

     mDebugInfoTableHeader.EfiDebugImageInfoTable = NewTable;

     //

     // Enlarge the max table entries and set the first empty entry index to

@@ -218,24 +211,34 @@ CoreNewDebugImageInfoEntry (
     //

     Index             = mMaxTableEntries;

     mMaxTableEntries += EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;

+    //

+    // Free the old table

+    //

+    CoreFreePool (Table);

+    //

+    // Update the table header

+    //

+    Table = NewTable;

   }

 

   //

   // Allocate data for new entry

   //

-  Table[Index].NormalImage = AllocateZeroPool (sizeof (EFI_DEBUG_IMAGE_INFO_NORMAL));

-  if (Table[Index].NormalImage != NULL) {

+  NormalImage = AllocateZeroPool (sizeof (EFI_DEBUG_IMAGE_INFO_NORMAL));

+  if (NormalImage != NULL) {

     //

     // Update the entry

     //

-    Table[Index].NormalImage->ImageInfoType               = (UINT32) ImageInfoType;

-    Table[Index].NormalImage->LoadedImageProtocolInstance = LoadedImage;

-    Table[Index].NormalImage->ImageHandle                 = ImageHandle;

+    NormalImage->ImageInfoType               = (UINT32) ImageInfoType;

+    NormalImage->LoadedImageProtocolInstance = LoadedImage;

+    NormalImage->ImageHandle                 = ImageHandle;

     //

-    // Increase the number of EFI_DEBUG_IMAGE_INFO elements and set the mDebugInfoTable in modified status.

+    // Set the mDebugInfoTable in modified status, insert the entry, and

+    // increase the number of EFI_DEBUG_IMAGE_INFO elements.

     //

-    mDebugInfoTableHeader.TableSize++;

     mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;

+    Table[Index].NormalImage = NormalImage;

+    mDebugInfoTableHeader.TableSize++;

   }

   mDebugInfoTableHeader.UpdateStatus &= ~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;

 }

@@ -253,8 +256,9 @@ CoreRemoveDebugImageInfoEntry (
   EFI_HANDLE ImageHandle

   )

 {

-  EFI_DEBUG_IMAGE_INFO  *Table;

-  UINTN                 Index;

+  EFI_DEBUG_IMAGE_INFO        *Table;

+  UINTN                       Index;

+  EFI_DEBUG_IMAGE_INFO_NORMAL *NormalImage;

 

   mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;

 

@@ -263,16 +267,20 @@ CoreRemoveDebugImageInfoEntry (
   for (Index = 0; Index < mMaxTableEntries; Index++) {

     if (Table[Index].NormalImage != NULL && Table[Index].NormalImage->ImageHandle == ImageHandle) {

       //

-      // Found a match. Free up the record, then NULL the pointer to indicate the slot

-      // is free.

+      // Found a match. Set the mDebugInfoTable in modified status and NULL the

+      // pointer to indicate the slot is free and.

       //

-      CoreFreePool (Table[Index].NormalImage);

+      NormalImage = Table[Index].NormalImage;

+      mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;

       Table[Index].NormalImage = NULL;

       //

-      // Decrease the number of EFI_DEBUG_IMAGE_INFO elements and set the mDebugInfoTable in modified status.

+      // Decrease the number of EFI_DEBUG_IMAGE_INFO elements.

       //

       mDebugInfoTableHeader.TableSize--;

-      mDebugInfoTableHeader.UpdateStatus |= EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;

+      //

+      // Free up the record.

+      //

+      CoreFreePool (NormalImage);

       break;

     }

   }

-- 
2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78882): https://edk2.groups.io/g/devel/message/78882
Mute This Topic: https://groups.io/mt/84754054/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-