[edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes

[edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes

[Oliver Smith-Denny]

ImagePropertiesRecordLib is currently creating Image Records that
are not accurate. It is setting the CodeSegmentSize to be the size
of the raw data in the image file, however, when the image is
loaded into memory, the raw data size is aligned to the
section alignment. This caused the memory attributes table to
have incorrect entries for systems, like ARM64, where the section
alignment is not 4k for all modules.

In fixing this, I noticed that MemoryProtection.c is using its own
version of image record creation where this logic was actually
correct. ImagePropertiesRecordLib was created to consolidate the
logic around creating and managing image records, so this patchset
also updates MemoryProtection.c to use ImagePropertiesRecordsLib
after making a few small adjustments to ensure the same functionality
is present.

This patchset was tested on ArmVirtQemu to ensure that all image
records were the same before and after this, other than fixing
the CodeSegmentSize.

v2:
- Align VirtualSize instead of SizeOfRawData

Github PR: https://github.com/tianocore/edk2/pull/5402

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <taylor.d.beebe@gmail.com>

Oliver Smith-Denny (3):
  MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for
    CodeSize
  MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage
  MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib

 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c                            | 241 +++-----------------
 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c |  86 +++++--
 2 files changed, 94 insertions(+), 233 deletions(-)

-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116657): https://edk2.groups.io/g/devel/message/116657
Mute This Topic: https://groups.io/mt/104873191/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize

[Oliver Smith-Denny]

When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the
CodeSegmentSize as the SizeOfRawData from the image. However, the image
as loaded into memory is aligned to the SectionAlignment, so
SizeOfRawData is under the actual size in memory. This is important,
because the memory attributes table uses these image records to create
its entries and it will report that the alignment of an image is
incorrect, even though the actual image is correct.

This was discovered on ARM64, which has a 64k runtime page granularity
alignment, which is backed by a 64k section alignment for
DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being
loaded into memory, however the memory attribute table was incorrectly
reporting misaligned ranges to the OS, causing attributes to be
ignored for these sections for OSes using greater than 4k pages.

This patch correctly aligns the CodeSegmentSize to the SectionAlignment
and the corresponding memory attribute table entries are now correctly
aligned and pointing to the right places in memory.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <taylor.d.beebe@gmail.com>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
index e53ce086c54c..763a8d65d565 100644
--- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
+++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
@@ -1090,7 +1090,9 @@ CreateImagePropertiesRecord (
       ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
 
       ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
-      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
+      // We still need to align the VirtualSize to the SectionAlignment because MSVC does not do
+      // this when creating a PE image. It expects the loader to do this.
+      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].Misc.VirtualSize, SectionAlignment);
 
       InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
       ImageRecord->CodeSegmentCount++;
-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116659): https://edk2.groups.io/g/devel/message/116659
Mute This Topic: https://groups.io/mt/104873193/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/3] MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage

[Oliver Smith-Denny]

Currently, there are multiple instances of code create image
records. ImagePropertiesRecordLib was created to only have
this code in one place. Update the lib to use additional
logic from the copy in MemoryProtection.c before converging
that code to use the lib.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Taylor Beebe <taylor.d.beebe@gmail.com>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 82 +++++++++++++++-----
 1 file changed, 63 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
index 763a8d65d565..3ac043f98098 100644
--- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
+++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
@@ -60,6 +60,39 @@ EfiSizeToPages (
   return RShiftU64 (Size, EFI_PAGE_SHIFT) + ((((UINTN)Size) & EFI_PAGE_MASK) ? 1 : 0);
 }
 
+/**
+  Frees the memory for each ImageRecordCodeSection within an ImageRecord
+  and removes the entries from the list. It does not free the ImageRecord
+  itself.
+
+  @param[in]  ImageRecord The ImageRecord in which to free code sections
+**/
+STATIC
+VOID
+FreeImageRecordCodeSections (
+  IMAGE_PROPERTIES_RECORD  *ImageRecord
+  )
+{
+  LIST_ENTRY                            *CodeSegmentListHead;
+  IMAGE_PROPERTIES_RECORD_CODE_SECTION  *ImageRecordCodeSection;
+
+  if (ImageRecord == NULL) {
+    return;
+  }
+
+  CodeSegmentListHead = &ImageRecord->CodeSegmentList;
+  while (!IsListEmpty (CodeSegmentListHead)) {
+    ImageRecordCodeSection = CR (
+                               CodeSegmentListHead->ForwardLink,
+                               IMAGE_PROPERTIES_RECORD_CODE_SECTION,
+                               Link,
+                               IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE
+                               );
+    RemoveEntryList (&ImageRecordCodeSection->Link);
+    FreePool (ImageRecordCodeSection);
+  }
+}
+
 /**
   Sort memory map entries based upon PhysicalStart from low to high.
 
@@ -993,6 +1026,7 @@ CreateImagePropertiesRecord (
   UINT8                                 *Name;
   UINT32                                SectionAlignment;
   UINT32                                PeCoffHeaderOffset;
+  CHAR8                                 *PdbPointer;
 
   if ((ImageRecord == NULL) || (ImageBase == NULL)) {
     return EFI_INVALID_PARAMETER;
@@ -1016,6 +1050,11 @@ CreateImagePropertiesRecord (
   InitializeListHead (&ImageRecord->Link);
   InitializeListHead (&ImageRecord->CodeSegmentList);
 
+  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageBase);
+  if (PdbPointer != NULL) {
+    DEBUG ((DEBUG_ERROR, " Image - %a\n", PdbPointer));
+  }
+
   // Check PE/COFF image
   DosHdr             = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageBase;
   PeCoffHeaderOffset = 0;
@@ -1084,7 +1123,8 @@ CreateImagePropertiesRecord (
       // Record code section(s)
       ImageRecordCodeSection = AllocatePool (sizeof (*ImageRecordCodeSection));
       if (ImageRecordCodeSection == NULL) {
-        return EFI_OUT_OF_RESOURCES;
+        Status = EFI_OUT_OF_RESOURCES;
+        goto CreateImagePropertiesRecordEnd;
       }
 
       ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
@@ -1103,6 +1143,27 @@ CreateImagePropertiesRecord (
     SortImageRecordCodeSection (ImageRecord);
   }
 
+  //
+  // Check overlap all section in ImageBase/Size
+  //
+  if (!IsImageRecordCodeSectionValid (ImageRecord)) {
+    DEBUG ((DEBUG_ERROR, "IsImageRecordCodeSectionValid - FAIL\n"));
+    Status = EFI_INVALID_PARAMETER;
+    goto CreateImagePropertiesRecordEnd;
+  }
+
+  //
+  // Round up the ImageSize, some CPU arch may return EFI_UNSUPPORTED if ImageSize is not aligned.
+  // Given that the loader always allocates full pages, we know the space after the image is not used.
+  //
+  ImageRecord->ImageSize = ALIGN_VALUE (ImageRecord->ImageSize, EFI_PAGE_SIZE);
+
+CreateImagePropertiesRecordEnd:
+  if (EFI_ERROR (Status)) {
+    // we failed to create a valid record, free the section memory that was allocated
+    FreeImageRecordCodeSections (ImageRecord);
+  }
+
   return Status;
 }
 
@@ -1119,24 +1180,7 @@ DeleteImagePropertiesRecord (
   IN  IMAGE_PROPERTIES_RECORD  *ImageRecord
   )
 {
-  LIST_ENTRY                            *CodeSegmentListHead;
-  IMAGE_PROPERTIES_RECORD_CODE_SECTION  *ImageRecordCodeSection;
-
-  if (ImageRecord == NULL) {
-    return;
-  }
-
-  CodeSegmentListHead = &ImageRecord->CodeSegmentList;
-  while (!IsListEmpty (CodeSegmentListHead)) {
-    ImageRecordCodeSection = CR (
-                               CodeSegmentListHead->ForwardLink,
-                               IMAGE_PROPERTIES_RECORD_CODE_SECTION,
-                               Link,
-                               IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE
-                               );
-    RemoveEntryList (&ImageRecordCodeSection->Link);
-    FreePool (ImageRecordCodeSection);
-  }
+  FreeImageRecordCodeSections (ImageRecord);
 
   if (!IsListEmpty (&ImageRecord->Link)) {
     RemoveEntryList (&ImageRecord->Link);
-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116660): https://edk2.groups.io/g/devel/message/116660
Mute This Topic: https://groups.io/mt/104873194/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib

[Oliver Smith-Denny]

The functionality to create and delete Image Records has been
consolidated in a library and ensured that MemoryProtection.c's
usage is encapsulated there.

This patch moves MemoryProtection.c to reuse the code in the lib
and to prevent issues in the future where code is updated in one
place but not the other.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Taylor Beebe <taylor.d.beebe@gmail.com>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++-----------------
 1 file changed, 28 insertions(+), 213 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 977239d08afc..ae9f8ae60bc7 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -281,80 +281,43 @@ SetUefiImageProtectionAttributes (
 }
 
 /**
-  Return if the PE image section is aligned.
+  Return the section alignment requirement for the PE image section type.
 
-  @param[in]  SectionAlignment    PE/COFF section alignment
-  @param[in]  MemoryType          PE/COFF image memory type
+  @param[in]  MemoryType  PE/COFF image memory type
+
+  @retval     The required section alignment for this memory type
 
-  @retval TRUE  The PE image section is aligned.
-  @retval FALSE The PE image section is not aligned.
 **/
-BOOLEAN
-IsMemoryProtectionSectionAligned (
-  IN UINT32           SectionAlignment,
+STATIC
+UINT32
+GetMemoryProtectionSectionAlignment (
   IN EFI_MEMORY_TYPE  MemoryType
   )
 {
-  UINT32  PageAlignment;
+  UINT32  SectionAlignment;
 
   switch (MemoryType) {
     case EfiRuntimeServicesCode:
     case EfiACPIMemoryNVS:
-      PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
+      SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
       break;
     case EfiRuntimeServicesData:
     case EfiACPIReclaimMemory:
       ASSERT (FALSE);
-      PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
+      SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
       break;
     case EfiBootServicesCode:
     case EfiLoaderCode:
     case EfiReservedMemoryType:
-      PageAlignment = EFI_PAGE_SIZE;
+      SectionAlignment = EFI_PAGE_SIZE;
       break;
     default:
       ASSERT (FALSE);
-      PageAlignment = EFI_PAGE_SIZE;
+      SectionAlignment = EFI_PAGE_SIZE;
       break;
   }
 
-  if ((SectionAlignment & (PageAlignment - 1)) != 0) {
-    return FALSE;
-  } else {
-    return TRUE;
-  }
-}
-
-/**
-  Free Image record.
-
-  @param[in]  ImageRecord    A UEFI image record
-**/
-VOID
-FreeImageRecord (
-  IN IMAGE_PROPERTIES_RECORD  *ImageRecord
-  )
-{
-  LIST_ENTRY                            *CodeSegmentListHead;
-  IMAGE_PROPERTIES_RECORD_CODE_SECTION  *ImageRecordCodeSection;
-
-  CodeSegmentListHead = &ImageRecord->CodeSegmentList;
-  while (!IsListEmpty (CodeSegmentListHead)) {
-    ImageRecordCodeSection = CR (
-                               CodeSegmentListHead->ForwardLink,
-                               IMAGE_PROPERTIES_RECORD_CODE_SECTION,
-                               Link,
-                               IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE
-                               );
-    RemoveEntryList (&ImageRecordCodeSection->Link);
-    FreePool (ImageRecordCodeSection);
-  }
-
-  if (ImageRecord->Link.ForwardLink != NULL) {
-    RemoveEntryList (&ImageRecord->Link);
-  }
-
-  FreePool (ImageRecord);
+  return SectionAlignment;
 }
 
 /**
@@ -369,19 +332,10 @@ ProtectUefiImage (
   IN EFI_DEVICE_PATH_PROTOCOL   *LoadedImageDevicePath
   )
 {
-  VOID                                  *ImageAddress;
-  EFI_IMAGE_DOS_HEADER                  *DosHdr;
-  UINT32                                PeCoffHeaderOffset;
-  UINT32                                SectionAlignment;
-  EFI_IMAGE_SECTION_HEADER              *Section;
-  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
-  UINT8                                 *Name;
-  UINTN                                 Index;
-  IMAGE_PROPERTIES_RECORD               *ImageRecord;
-  CHAR8                                 *PdbPointer;
-  IMAGE_PROPERTIES_RECORD_CODE_SECTION  *ImageRecordCodeSection;
-  BOOLEAN                               IsAligned;
-  UINT32                                ProtectionPolicy;
+  IMAGE_PROPERTIES_RECORD  *ImageRecord;
+  UINT32                   ProtectionPolicy;
+  EFI_STATUS               Status;
+  UINT32                   RequiredAlignment;
 
   DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));
   DEBUG ((DEBUG_INFO, "  - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));
@@ -406,160 +360,21 @@ ProtectUefiImage (
     return;
   }
 
-  ImageRecord->Signature = IMAGE_PROPERTIES_RECORD_SIGNATURE;
+  RequiredAlignment = GetMemoryProtectionSectionAlignment (LoadedImage->ImageCodeType);
 
-  //
-  // Step 1: record whole region
-  //
-  ImageRecord->ImageBase = (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase;
-  ImageRecord->ImageSize = LoadedImage->ImageSize;
-
-  ImageAddress = LoadedImage->ImageBase;
-
-  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);
-  if (PdbPointer != NULL) {
-    DEBUG ((DEBUG_VERBOSE, "  Image - %a\n", PdbPointer));
-  }
-
-  //
-  // Check PE/COFF image
-  //
-  DosHdr             = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress;
-  PeCoffHeaderOffset = 0;
-  if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
-    PeCoffHeaderOffset = DosHdr->e_lfanew;
-  }
-
-  Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress + PeCoffHeaderOffset);
-  if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
-    DEBUG ((DEBUG_VERBOSE, "Hdr.Pe32->Signature invalid - 0x%x\n", Hdr.Pe32->Signature));
-    // It might be image in SMM.
-    goto Finish;
-  }
-
-  //
-  // Get SectionAlignment
-  //
-  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment;
-  } else {
-    SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
-  }
-
-  IsAligned = IsMemoryProtectionSectionAligned (SectionAlignment, LoadedImage->ImageCodeType);
-  if (!IsAligned) {
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "!!!!!!!!  ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect  !!!!!!!!\n",
-      SectionAlignment
-      ));
-    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);
-    if (PdbPointer != NULL) {
-      DEBUG ((DEBUG_VERBOSE, "!!!!!!!!  Image - %a  !!!!!!!!\n", PdbPointer));
-    }
+  Status = CreateImagePropertiesRecord (
+             LoadedImage->ImageBase,
+             LoadedImage->ImageSize,
+             &RequiredAlignment,
+             ImageRecord
+             );
 
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a failed to create image properties record\n", __func__));
+    FreePool (ImageRecord);
     goto Finish;
   }
 
-  Section = (EFI_IMAGE_SECTION_HEADER *)(
-                                         (UINT8 *)(UINTN)ImageAddress +
-                                         PeCoffHeaderOffset +
-                                         sizeof (UINT32) +
-                                         sizeof (EFI_IMAGE_FILE_HEADER) +
-                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
-                                         );
-  ImageRecord->CodeSegmentCount = 0;
-  InitializeListHead (&ImageRecord->CodeSegmentList);
-  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
-    Name = Section[Index].Name;
-    DEBUG ((
-      DEBUG_VERBOSE,
-      "  Section - '%c%c%c%c%c%c%c%c'\n",
-      Name[0],
-      Name[1],
-      Name[2],
-      Name[3],
-      Name[4],
-      Name[5],
-      Name[6],
-      Name[7]
-      ));
-
-    //
-    // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE
-    // can always be mapped read-only, classify a section as a code section only
-    // if it has the executable attribute set and the writable attribute cleared.
-    //
-    // This adheres more closely to the PE/COFF spec, and avoids issues with
-    // Linux OS loaders that may consist of a single read/write/execute section.
-    //
-    if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE | EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) {
-      DEBUG ((DEBUG_VERBOSE, "  VirtualSize          - 0x%08x\n", Section[Index].Misc.VirtualSize));
-      DEBUG ((DEBUG_VERBOSE, "  VirtualAddress       - 0x%08x\n", Section[Index].VirtualAddress));
-      DEBUG ((DEBUG_VERBOSE, "  SizeOfRawData        - 0x%08x\n", Section[Index].SizeOfRawData));
-      DEBUG ((DEBUG_VERBOSE, "  PointerToRawData     - 0x%08x\n", Section[Index].PointerToRawData));
-      DEBUG ((DEBUG_VERBOSE, "  PointerToRelocations - 0x%08x\n", Section[Index].PointerToRelocations));
-      DEBUG ((DEBUG_VERBOSE, "  PointerToLinenumbers - 0x%08x\n", Section[Index].PointerToLinenumbers));
-      DEBUG ((DEBUG_VERBOSE, "  NumberOfRelocations  - 0x%08x\n", Section[Index].NumberOfRelocations));
-      DEBUG ((DEBUG_VERBOSE, "  NumberOfLinenumbers  - 0x%08x\n", Section[Index].NumberOfLinenumbers));
-      DEBUG ((DEBUG_VERBOSE, "  Characteristics      - 0x%08x\n", Section[Index].Characteristics));
-
-      //
-      // Step 2: record code section
-      //
-      ImageRecordCodeSection = AllocatePool (sizeof (*ImageRecordCodeSection));
-      if (ImageRecordCodeSection == NULL) {
-        return;
-      }
-
-      ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
-
-      ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageAddress + Section[Index].VirtualAddress;
-      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment);
-
-      DEBUG ((DEBUG_VERBOSE, "ImageCode: 0x%016lx - 0x%016lx\n", ImageRecordCodeSection->CodeSegmentBase, ImageRecordCodeSection->CodeSegmentSize));
-
-      InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
-      ImageRecord->CodeSegmentCount++;
-    }
-  }
-
-  if (ImageRecord->CodeSegmentCount == 0) {
-    //
-    // If a UEFI executable consists of a single read+write+exec PE/COFF
-    // section, that isn't actually an error. The image can be launched
-    // alright, only image protection cannot be applied to it fully.
-    //
-    // One example that elicits this is (some) Linux kernels (with the EFI stub
-    // of course).
-    //
-    DEBUG ((DEBUG_WARN, "!!!!!!!!  ProtectUefiImageCommon - CodeSegmentCount is 0  !!!!!!!!\n"));
-    PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);
-    if (PdbPointer != NULL) {
-      DEBUG ((DEBUG_WARN, "!!!!!!!!  Image - %a  !!!!!!!!\n", PdbPointer));
-    }
-
-    goto Finish;
-  }
-
-  //
-  // Final
-  //
-  SortImageRecordCodeSection (ImageRecord);
-  //
-  // Check overlap all section in ImageBase/Size
-  //
-  if (!IsImageRecordCodeSectionValid (ImageRecord)) {
-    DEBUG ((DEBUG_ERROR, "IsImageRecordCodeSectionValid - FAIL\n"));
-    goto Finish;
-  }
-
-  //
-  // Round up the ImageSize, some CPU arch may return EFI_UNSUPPORTED if ImageSize is not aligned.
-  // Given that the loader always allocates full pages, we know the space after the image is not used.
-  //
-  ImageRecord->ImageSize = ALIGN_VALUE (LoadedImage->ImageSize, EFI_PAGE_SIZE);
-
   //
   // CPU ARCH present. Update memory attribute directly.
   //
@@ -607,7 +422,7 @@ UnprotectUefiImage (
           ImageRecord->ImageSize,
           0
           );
-        FreeImageRecord (ImageRecord);
+        DeleteImagePropertiesRecord (ImageRecord);
         return;
       }
     }
-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116658): https://edk2.groups.io/g/devel/message/116658
Mute This Topic: https://groups.io/mt/104873192/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize

[Ard Biesheuvel]

On Mon, 11 Mar 2024 at 22:29, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the
> CodeSegmentSize as the SizeOfRawData from the image. However, the image
> as loaded into memory is aligned to the SectionAlignment, so
> SizeOfRawData is under the actual size in memory. This is important,
> because the memory attributes table uses these image records to create
> its entries and it will report that the alignment of an image is
> incorrect, even though the actual image is correct.
>
> This was discovered on ARM64, which has a 64k runtime page granularity
> alignment, which is backed by a 64k section alignment for
> DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being
> loaded into memory, however the memory attribute table was incorrectly
> reporting misaligned ranges to the OS, causing attributes to be
> ignored for these sections for OSes using greater than 4k pages.
>
> This patch correctly aligns the CodeSegmentSize to the SectionAlignment
> and the corresponding memory attribute table entries are now correctly
> aligned and pointing to the right places in memory.
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Taylor Beebe <taylor.d.beebe@gmail.com>
>
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
> ---
>  MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> index e53ce086c54c..763a8d65d565 100644
> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> @@ -1090,7 +1090,9 @@ CreateImagePropertiesRecord (
>        ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
>
>        ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
> -      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
> +      // We still need to align the VirtualSize to the SectionAlignment because MSVC does not do
> +      // this when creating a PE image. It expects the loader to do this.
> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].Misc.VirtualSize, SectionAlignment);
>
>        InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
>        ImageRecord->CodeSegmentCount++;
> --
> 2.40.1
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116672): https://edk2.groups.io/g/devel/message/116672
Mute This Topic: https://groups.io/mt/104873193/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

Reviewed-by: Marvin Häuser <mhaeuser@posteo.de>

On Mon, Mar 11, 2024 at 02:29 PM, Oliver Smith-Denny wrote:

> 
> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the
> CodeSegmentSize as the SizeOfRawData from the image. However, the image
> as loaded into memory is aligned to the SectionAlignment, so
> SizeOfRawData is under the actual size in memory. This is important,
> because the memory attributes table uses these image records to create
> its entries and it will report that the alignment of an image is
> incorrect, even though the actual image is correct.
> 
> This was discovered on ARM64, which has a 64k runtime page granularity
> alignment, which is backed by a 64k section alignment for
> DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being
> loaded into memory, however the memory attribute table was incorrectly
> reporting misaligned ranges to the OS, causing attributes to be
> ignored for these sections for OSes using greater than 4k pages.
> 
> This patch correctly aligns the CodeSegmentSize to the SectionAlignment
> and the corresponding memory attribute table entries are now correctly
> aligned and pointing to the right places in memory.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Taylor Beebe <taylor.d.beebe@gmail.com>
> 
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
> ---
> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c=
> | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertie=
> 
> sRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropert=
> 
> iesRecordLib.c
> index e53ce086c54c..763a8d65d565 100644
> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecord=
> 
> Lib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecord=
> 
> Lib.c
> @@ -1090,7 +1090,9 @@ CreateImagePropertiesRecord (
> ImageRecordCodeSection->Signature =3D IMAGE_PROPERTIES_RECORD_CODE=
> _SECTION_SIGNATURE;
> =20
> ImageRecordCodeSection->CodeSegmentBase =3D (UINTN)ImageBase + Sec=
> tion[Index].VirtualAddress;
> - ImageRecordCodeSection->CodeSegmentSize =3D Section[Index].SizeOfR=
> awData;
> + // We still need to align the VirtualSize to the SectionAlignment =
> because MSVC does not do
> + // this when creating a PE image. It expects the loader to do this=
> .
> + ImageRecordCodeSection->CodeSegmentSize =3D ALIGN_VALUE (Section[I=
> ndex].Misc.VirtualSize, SectionAlignment);
> =20
> InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSec=
> tion->Link);
> ImageRecord->CodeSegmentCount++;
> --=20
> 2.40.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116822): https://edk2.groups.io/g/devel/message/116822
Mute This Topic: https://groups.io/mt/104873193/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 3866 bytes --]