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

[edk2-devel][PATCH v1 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.

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 |  84 +++++--
 2 files changed, 92 insertions(+), 233 deletions(-)

-- 
2.40.1



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

[edk2-devel][PATCH v1 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
index e53ce086c54c..07ced0e54e38 100644
--- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
+++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
@@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
       ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
 
       ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
-      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
+      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, 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 (#116060): https://edk2.groups.io/g/devel/message/116060
Mute This Topic: https://groups.io/mt/104610770/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel][PATCH v1 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 07ced0e54e38..9b99cb0f77d2 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;
@@ -1101,6 +1141,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;
 }
 
@@ -1117,24 +1178,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 (#116061): https://edk2.groups.io/g/devel/message/116061
Mute This Topic: https://groups.io/mt/104610771/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel][PATCH v1 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 (#116062): https://edk2.groups.io/g/devel/message/116062
Mute This Topic: https://groups.io/mt/104610772/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

Hi Oliver,

On Tue, 27 Feb 2024 at 21:27, 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.
>

Can you explain how these can differ in the first place? Our flaky
ELF-to-PE/COFF converter should never generate such images to begin
with (which is probably how we ended up with this problem in the first
place), so I  suppose this is native PE/COFF tooling emitting sections
either using a non-1:1 file:memory mapping, or with unallocated holes
in the file representation?

> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> index e53ce086c54c..07ced0e54e38 100644
> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
>        ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
>
>        ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
> -      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment);
>

This should be the virtual size, not the file size, right?

>        InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
>        ImageRecord->CodeSegmentCount++;
> --
> 2.40.1
>


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

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

[Oliver Smith-Denny]

Hi Ard,

On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
> Hi Oliver,
> 
> On Tue, 27 Feb 2024 at 21:27, 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.
>>
> 
> Can you explain how these can differ in the first place? Our flaky
> ELF-to-PE/COFF converter should never generate such images to begin
> with (which is probably how we ended up with this problem in the first
> place), so I  suppose this is native PE/COFF tooling emitting sections
> either using a non-1:1 file:memory mapping, or with unallocated holes
> in the file representation?
> 

This is an artifact of PE/COFF itself and is useful for having smaller
FW images. In PE/COFF we have the section alignment (which is how we get
loaded into memory) and the file alignment (how the actual file is
aligned). It is valid for these two to be different. For example, these
runtime DXE drivers, which are not XIP (which the case we do need the
section and file alignment to be the same, as we are executing from
the file image) can be a smaller size in the file, but when loaded into
memory we will relocate them and do the proper rebasing to set these on
64k boundaries. Different toolchains have different ways of specifying
the two things, on gcc I have seen common-page-size affect the section
alignment and max-page-size affect both section and file alignment. For
msvc there are /ALIGN and /FILEALIGN commands which directly manipulate
these values.

The issue here was not that we have different section and file
alignment, in fact, the issue still exists if section alignement ==
file alignment. This is because SizeOfRawData in the PE/COFF header is
the raw size of bytes used, not even page aligned. So no matter what,
the image records we were creating here had bad sizes being set which
do not match what the image loader was actually doing.

I do think there is a fair argument that would say the image loader
should create the image records when it loads images, since in the
end we want the record to match exactly what the image in memory is,
creating after the fact is a poor pattern.

>> 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 | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>> index e53ce086c54c..07ced0e54e38 100644
>> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
>>         ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
>>
>>         ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
>> -      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment);
>>
> 
> This should be the virtual size, not the file size, right?

Correct, SectionAlignment is the alignment of the image as loaded in
memory, so in the case of a DXE runtime driver on ARM64, it will be
64k.

> 
>>         InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link);
>>         ImageRecord->CodeSegmentCount++;
>> --
>> 2.40.1
>>


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

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

[Ard Biesheuvel]

On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Hi Ard,
>
> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
> > Hi Oliver,
> >
> > On Tue, 27 Feb 2024 at 21:27, 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.
> >>
> >
> > Can you explain how these can differ in the first place? Our flaky
> > ELF-to-PE/COFF converter should never generate such images to begin
> > with (which is probably how we ended up with this problem in the first
> > place), so I  suppose this is native PE/COFF tooling emitting sections
> > either using a non-1:1 file:memory mapping, or with unallocated holes
> > in the file representation?
> >
>
> This is an artifact of PE/COFF itself and is useful for having smaller
> FW images. In PE/COFF we have the section alignment (which is how we get
> loaded into memory) and the file alignment (how the actual file is
> aligned). It is valid for these two to be different. For example, these
> runtime DXE drivers, which are not XIP (which the case we do need the
> section and file alignment to be the same, as we are executing from
> the file image) can be a smaller size in the file, but when loaded into
> memory we will relocate them and do the proper rebasing to set these on
> 64k boundaries. Different toolchains have different ways of specifying
> the two things, on gcc I have seen common-page-size affect the section
> alignment and max-page-size affect both section and file alignment. For
> msvc there are /ALIGN and /FILEALIGN commands which directly manipulate
> these values.
>
> The issue here was not that we have different section and file
> alignment, in fact, the issue still exists if section alignement ==
> file alignment. This is because SizeOfRawData in the PE/COFF header is
> the raw size of bytes used, not even page aligned. So no matter what,
> the image records we were creating here had bad sizes being set which
> do not match what the image loader was actually doing.
>

IIUC the SizeOfRawData is the file view not the memory view, and must
always be aligned to the FileAlignment.

> I do think there is a fair argument that would say the image loader
> should create the image records when it loads images, since in the
> end we want the record to match exactly what the image in memory is,
> creating after the fact is a poor pattern.
>

Yeah, no disagreement there.

> >> 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 | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> >> index e53ce086c54c..07ced0e54e38 100644
> >> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> >> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
> >> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
> >>         ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
> >>
> >>         ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
> >> -      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
> >> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment);
> >>
> >
> > This should be the virtual size, not the file size, right?
>
> Correct, SectionAlignment is the alignment of the image as loaded in
> memory, so in the case of a DXE runtime driver on ARM64, it will be
> 64k.
>

No, I mean we should not be using SizeOfRawData here but VirtualSize.

I understand this is unlikely to make a difference in practice, but
VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
whole region to be covered.


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

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

[Oliver Smith-Denny]

On 3/4/2024 10:54 AM, Ard Biesheuvel wrote:
> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> Hi Ard,
>>
>> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
>>> Hi Oliver,
>>>
>>> On Tue, 27 Feb 2024 at 21:27, 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.
>>>>
>>>
>>> Can you explain how these can differ in the first place? Our flaky
>>> ELF-to-PE/COFF converter should never generate such images to begin
>>> with (which is probably how we ended up with this problem in the first
>>> place), so I  suppose this is native PE/COFF tooling emitting sections
>>> either using a non-1:1 file:memory mapping, or with unallocated holes
>>> in the file representation?
>>>
>>
>> This is an artifact of PE/COFF itself and is useful for having smaller
>> FW images. In PE/COFF we have the section alignment (which is how we get
>> loaded into memory) and the file alignment (how the actual file is
>> aligned). It is valid for these two to be different. For example, these
>> runtime DXE drivers, which are not XIP (which the case we do need the
>> section and file alignment to be the same, as we are executing from
>> the file image) can be a smaller size in the file, but when loaded into
>> memory we will relocate them and do the proper rebasing to set these on
>> 64k boundaries. Different toolchains have different ways of specifying
>> the two things, on gcc I have seen common-page-size affect the section
>> alignment and max-page-size affect both section and file alignment. For
>> msvc there are /ALIGN and /FILEALIGN commands which directly manipulate
>> these values.
>>
>> The issue here was not that we have different section and file
>> alignment, in fact, the issue still exists if section alignement ==
>> file alignment. This is because SizeOfRawData in the PE/COFF header is
>> the raw size of bytes used, not even page aligned. So no matter what,
>> the image records we were creating here had bad sizes being set which
>> do not match what the image loader was actually doing.
>>
> 
> IIUC the SizeOfRawData is the file view not the memory view, and must
> always be aligned to the FileAlignment.

I relooked at the spec, you are correct, SizeOfRawData is aligned to
the FileAlignment. So this is only a bug for a file with
SectionAlignment != FileAlignment.

> 
>> I do think there is a fair argument that would say the image loader
>> should create the image records when it loads images, since in the
>> end we want the record to match exactly what the image in memory is,
>> creating after the fact is a poor pattern.
>>
> 
> Yeah, no disagreement there.
> 
>>>> 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 | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>>>> index e53ce086c54c..07ced0e54e38 100644
>>>> --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>>>> +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c
>>>> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord (
>>>>          ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE;
>>>>
>>>>          ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress;
>>>> -      ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData;
>>>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment);
>>>>
>>>
>>> This should be the virtual size, not the file size, right?
>>
>> Correct, SectionAlignment is the alignment of the image as loaded in
>> memory, so in the case of a DXE runtime driver on ARM64, it will be
>> 64k.
>>
> 
> No, I mean we should not be using SizeOfRawData here but VirtualSize.
> 
> I understand this is unlikely to make a difference in practice, but
> VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
> whole region to be covered.
> 

I see, yes I do believe VirtualSize could be used here instead. Two
things give me pause. One is that the PE spec states that SizeOfRawData
is rounded and VirtualSize is not, so that SizeOfRawData may be greater
than the VirtualSize in some cases (which seems incorrect).

The other is that the image loader partially uses VirtualSize when
loading:

https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400

However, when determining the size of a loaded image (and therefore the
number of pages to allocate) it will allocate an extra page:

https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652

as ImageSize here is:

https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312

Which according to the spec, SizeOfImage is the size of the image as
loaded into memory and must be a multiple of section alignment.

So, reflecting on this, let me test with VirtualSize here, I think
that is the right value to use, the only time we would have a
SizeOfRawData that is greater than the VirtualSize would be if our
FileAlignment is greater than our SectionAlignment, which would be
a misconfiguration.

I do think the ImageLoader should also be fixed to only allocate
ImageSize number of pages (which should be the sum of the section
VirtualSizes + any headers that aren't included). Might as well not
waste an extra page for each image and then our image record code is
simpler as well (ensuring we are protecting the right pages).

I think this patch series should go in, as it is fixing an active bug,
but I will also take a look at the image loader creating the image
records and having a protocol it produces to retrieve the list, to
attempt to avoid issues like this in the future.


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

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

[Oliver Smith-Denny]

On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote:
> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote:
>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
>> <osde@linux.microsoft.com> wrote:
>>>
>>> Hi Ard,
>>>
>>> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
>>>> Hi Oliver,
>>>>
>>>> On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny
>>>> <osde@linux.microsoft.com> wrote:
>>>>>
>>>>> -      ImageRecordCodeSection->CodeSegmentSize = 
>>>>> Section[Index].SizeOfRawData;
>>>>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE 
>>>>> (Section[Index].SizeOfRawData, SectionAlignment);
>>>>>
>>>>
>>>> This should be the virtual size, not the file size, right?
>>>
>>> Correct, SectionAlignment is the alignment of the image as loaded in
>>> memory, so in the case of a DXE runtime driver on ARM64, it will be
>>> 64k.
>>>
>>
>> No, I mean we should not be using SizeOfRawData here but VirtualSize.
>>
>> I understand this is unlikely to make a difference in practice, but
>> VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
>> whole region to be covered.
>>
> 
> I see, yes I do believe VirtualSize could be used here instead. Two
> things give me pause. One is that the PE spec states that SizeOfRawData
> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
> than the VirtualSize in some cases (which seems incorrect).
> 
> The other is that the image loader partially uses VirtualSize when
> loading:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
> 
> However, when determining the size of a loaded image (and therefore the
> number of pages to allocate) it will allocate an extra page:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
> 
> as ImageSize here is:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
> 
> Which according to the spec, SizeOfImage is the size of the image as
> loaded into memory and must be a multiple of section alignment.
> 
> So, reflecting on this, let me test with VirtualSize here, I think
> that is the right value to use, the only time we would have a
> SizeOfRawData that is greater than the VirtualSize would be if our
> FileAlignment is greater than our SectionAlignment, which would be
> a misconfiguration.
> 
> I do think the ImageLoader should also be fixed to only allocate
> ImageSize number of pages (which should be the sum of the section
> VirtualSizes + any headers that aren't included). Might as well not
> waste an extra page for each image and then our image record code is
> simpler as well (ensuring we are protecting the right pages).
> 
> I think this patch series should go in, as it is fixing an active bug,
> but I will also take a look at the image loader creating the image
> records and having a protocol it produces to retrieve the list, to
> attempt to avoid issues like this in the future.
> 

Surprisingly, I am seeing that the VirtualSize is not 64k aligned there.
I am looking deeper into GenFw to make sure it is correctly getting set
to align to SectionAlignment in the section headers. When I use dumpbin
to dump the headers, it shows each section having VirtualSize as 64k
aligned for a runtime image, but the same image doesn't show that in FW.

I'll do some digging here.

Thanks,
Oliver


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

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

[Oliver Smith-Denny]

On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote:
> On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote:
>> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote:
>>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
>>> <osde@linux.microsoft.com> wrote:
>>>>
>>>> Hi Ard,
>>>>
>>>> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
>>>>> Hi Oliver,
>>>>>
>>>>> On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny
>>>>> <osde@linux.microsoft.com> wrote:
>>>>>>
>>>>>> -      ImageRecordCodeSection->CodeSegmentSize = 
>>>>>> Section[Index].SizeOfRawData;
>>>>>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE 
>>>>>> (Section[Index].SizeOfRawData, SectionAlignment);
>>>>>>
>>>>>
>>>>> This should be the virtual size, not the file size, right?
>>>>
>>>> Correct, SectionAlignment is the alignment of the image as loaded in
>>>> memory, so in the case of a DXE runtime driver on ARM64, it will be
>>>> 64k.
>>>>
>>>
>>> No, I mean we should not be using SizeOfRawData here but VirtualSize.
>>>
>>> I understand this is unlikely to make a difference in practice, but
>>> VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
>>> whole region to be covered.
>>>
>>
>> I see, yes I do believe VirtualSize could be used here instead. Two
>> things give me pause. One is that the PE spec states that SizeOfRawData
>> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
>> than the VirtualSize in some cases (which seems incorrect).
>>
>> The other is that the image loader partially uses VirtualSize when
>> loading:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
>>
>> However, when determining the size of a loaded image (and therefore the
>> number of pages to allocate) it will allocate an extra page:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
>>
>> as ImageSize here is:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
>>
>> Which according to the spec, SizeOfImage is the size of the image as
>> loaded into memory and must be a multiple of section alignment.
>>
>> So, reflecting on this, let me test with VirtualSize here, I think
>> that is the right value to use, the only time we would have a
>> SizeOfRawData that is greater than the VirtualSize would be if our
>> FileAlignment is greater than our SectionAlignment, which would be
>> a misconfiguration.
>>
>> I do think the ImageLoader should also be fixed to only allocate
>> ImageSize number of pages (which should be the sum of the section
>> VirtualSizes + any headers that aren't included). Might as well not
>> waste an extra page for each image and then our image record code is
>> simpler as well (ensuring we are protecting the right pages).
>>
>> I think this patch series should go in, as it is fixing an active bug,
>> but I will also take a look at the image loader creating the image
>> records and having a protocol it produces to retrieve the list, to
>> attempt to avoid issues like this in the future.
>>
> 
> Surprisingly, I am seeing that the VirtualSize is not 64k aligned there.
> I am looking deeper into GenFw to make sure it is correctly getting set
> to align to SectionAlignment in the section headers. When I use dumpbin
> to dump the headers, it shows each section having VirtualSize as 64k
> aligned for a runtime image, but the same image doesn't show that in FW.
> 
> I'll do some digging here.
> 

Following up on this:

Not surprisingly, different toolchains do different things here.

gcc obviously creates ELF files and ElfConvert*.c converts these to PE
images. However, when it does this, it always sets the section and file
alignment to the same value (I'm not as familiar with ELF images, I'm
not sure if there is the same concept as file vs section alignment).
GenFw could probably update this information to shrink the file
alignment, but that's a space optimization for gcc built binaries.

In ElfConvert.c, the VirtualSize does get set to the section aligned
value, which is what I would expect from the spec.

In MSVC PE images, VirtualSize is not section aligned and the
expectation appears to be that loaders will align the VirtualSize
to the section alignment. I am planning on reaching out to the MSVC
folks to learn why this is the case, is it intended, etc., as my
understanding is that the VirtualSize in the section headers should
be section aligned.

We are stuck with this for existing MSVC toolchains, however, so we
will need to align either the VirtualSize or SizeOfRawData to the
section alignment when we create the image records. I don't have a
preference between the two, they end up being the same when we align
them, so I can send a v2 with aligning the VirtualSize. This will be
a no-op on gcc built binaries and will set it to the correct value for
MSVC.

Thanks,
Oliver


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

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

[Ard Biesheuvel]

On Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote:
> > On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote:
> >> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote:
> >>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
> >>> <osde@linux.microsoft.com> wrote:
> >>>>
> >>>> Hi Ard,
> >>>>
> >>>> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
> >>>>> Hi Oliver,
> >>>>>
> >>>>> On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny
> >>>>> <osde@linux.microsoft.com> wrote:
> >>>>>>
> >>>>>> -      ImageRecordCodeSection->CodeSegmentSize =
> >>>>>> Section[Index].SizeOfRawData;
> >>>>>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE
> >>>>>> (Section[Index].SizeOfRawData, SectionAlignment);
> >>>>>>
> >>>>>
> >>>>> This should be the virtual size, not the file size, right?
> >>>>
> >>>> Correct, SectionAlignment is the alignment of the image as loaded in
> >>>> memory, so in the case of a DXE runtime driver on ARM64, it will be
> >>>> 64k.
> >>>>
> >>>
> >>> No, I mean we should not be using SizeOfRawData here but VirtualSize.
> >>>
> >>> I understand this is unlikely to make a difference in practice, but
> >>> VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
> >>> whole region to be covered.
> >>>
> >>
> >> I see, yes I do believe VirtualSize could be used here instead. Two
> >> things give me pause. One is that the PE spec states that SizeOfRawData
> >> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
> >> than the VirtualSize in some cases (which seems incorrect).
> >>
> >> The other is that the image loader partially uses VirtualSize when
> >> loading:
> >>
> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
> >>
> >> However, when determining the size of a loaded image (and therefore the
> >> number of pages to allocate) it will allocate an extra page:
> >>
> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
> >>
> >> as ImageSize here is:
> >>
> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
> >>
> >> Which according to the spec, SizeOfImage is the size of the image as
> >> loaded into memory and must be a multiple of section alignment.
> >>
> >> So, reflecting on this, let me test with VirtualSize here, I think
> >> that is the right value to use, the only time we would have a
> >> SizeOfRawData that is greater than the VirtualSize would be if our
> >> FileAlignment is greater than our SectionAlignment, which would be
> >> a misconfiguration.
> >>
> >> I do think the ImageLoader should also be fixed to only allocate
> >> ImageSize number of pages (which should be the sum of the section
> >> VirtualSizes + any headers that aren't included). Might as well not
> >> waste an extra page for each image and then our image record code is
> >> simpler as well (ensuring we are protecting the right pages).
> >>
> >> I think this patch series should go in, as it is fixing an active bug,
> >> but I will also take a look at the image loader creating the image
> >> records and having a protocol it produces to retrieve the list, to
> >> attempt to avoid issues like this in the future.
> >>
> >
> > Surprisingly, I am seeing that the VirtualSize is not 64k aligned there.
> > I am looking deeper into GenFw to make sure it is correctly getting set
> > to align to SectionAlignment in the section headers. When I use dumpbin
> > to dump the headers, it shows each section having VirtualSize as 64k
> > aligned for a runtime image, but the same image doesn't show that in FW.
> >
> > I'll do some digging here.
> >
>
> Following up on this:
>
> Not surprisingly, different toolchains do different things here.
>
> gcc obviously creates ELF files and ElfConvert*.c converts these to PE
> images. However, when it does this, it always sets the section and file
> alignment to the same value (I'm not as familiar with ELF images, I'm
> not sure if there is the same concept as file vs section alignment).
> GenFw could probably update this information to shrink the file
> alignment, but that's a space optimization for gcc built binaries.
>
> In ElfConvert.c, the VirtualSize does get set to the section aligned
> value, which is what I would expect from the spec.
>
> In MSVC PE images, VirtualSize is not section aligned and the
> expectation appears to be that loaders will align the VirtualSize
> to the section alignment. I am planning on reaching out to the MSVC
> folks to learn why this is the case, is it intended, etc., as my
> understanding is that the VirtualSize in the section headers should
> be section aligned.
>
> We are stuck with this for existing MSVC toolchains, however, so we
> will need to align either the VirtualSize or SizeOfRawData to the
> section alignment when we create the image records. I don't have a
> preference between the two, they end up being the same when we align
> them, so I can send a v2 with aligning the VirtualSize. This will be
> a no-op on gcc built binaries and will set it to the correct value for
> MSVC.
>

I don't think this is correct. The VirtualSize could be much larger
than the SizeOfRawData (to the extent where the delta exceeds the
alignment padding), in cases where some of the memory is
data-initialized and some of it is zero-initialized. We certainly make
use of this in Linux when constructing the PE/COFF view of the kernel
image.

So for the memory view of the image, only VirtualSize is appropriate -
SizeOfRawData just gives you the size of the data to copy to the start
of this section.


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

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

[Oliver Smith-Denny]

On 3/12/2024 1:32 AM, Ard Biesheuvel wrote:
> On Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote:
>>> On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote:
>>>> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote:
>>>>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny
>>>>> <osde@linux.microsoft.com> wrote:
>>>>>>
>>>>>> Hi Ard,
>>>>>>
>>>>>> On 3/1/2024 3:58 AM, Ard Biesheuvel wrote:
>>>>>>> Hi Oliver,
>>>>>>>
>>>>>>> On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny
>>>>>>> <osde@linux.microsoft.com> wrote:
>>>>>>>>
>>>>>>>> -      ImageRecordCodeSection->CodeSegmentSize =
>>>>>>>> Section[Index].SizeOfRawData;
>>>>>>>> +      ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE
>>>>>>>> (Section[Index].SizeOfRawData, SectionAlignment);
>>>>>>>>
>>>>>>>
>>>>>>> This should be the virtual size, not the file size, right?
>>>>>>
>>>>>> Correct, SectionAlignment is the alignment of the image as loaded in
>>>>>> memory, so in the case of a DXE runtime driver on ARM64, it will be
>>>>>> 64k.
>>>>>>
>>>>>
>>>>> No, I mean we should not be using SizeOfRawData here but VirtualSize.
>>>>>
>>>>> I understand this is unlikely to make a difference in practice, but
>>>>> VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
>>>>> whole region to be covered.
>>>>>
>>>>
>>>> I see, yes I do believe VirtualSize could be used here instead. Two
>>>> things give me pause. One is that the PE spec states that SizeOfRawData
>>>> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
>>>> than the VirtualSize in some cases (which seems incorrect).
>>>>
>>>> The other is that the image loader partially uses VirtualSize when
>>>> loading:
>>>>
>>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
>>>>
>>>> However, when determining the size of a loaded image (and therefore the
>>>> number of pages to allocate) it will allocate an extra page:
>>>>
>>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
>>>>
>>>> as ImageSize here is:
>>>>
>>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
>>>>
>>>> Which according to the spec, SizeOfImage is the size of the image as
>>>> loaded into memory and must be a multiple of section alignment.
>>>>
>>>> So, reflecting on this, let me test with VirtualSize here, I think
>>>> that is the right value to use, the only time we would have a
>>>> SizeOfRawData that is greater than the VirtualSize would be if our
>>>> FileAlignment is greater than our SectionAlignment, which would be
>>>> a misconfiguration.
>>>>
>>>> I do think the ImageLoader should also be fixed to only allocate
>>>> ImageSize number of pages (which should be the sum of the section
>>>> VirtualSizes + any headers that aren't included). Might as well not
>>>> waste an extra page for each image and then our image record code is
>>>> simpler as well (ensuring we are protecting the right pages).
>>>>
>>>> I think this patch series should go in, as it is fixing an active bug,
>>>> but I will also take a look at the image loader creating the image
>>>> records and having a protocol it produces to retrieve the list, to
>>>> attempt to avoid issues like this in the future.
>>>>
>>>
>>> Surprisingly, I am seeing that the VirtualSize is not 64k aligned there.
>>> I am looking deeper into GenFw to make sure it is correctly getting set
>>> to align to SectionAlignment in the section headers. When I use dumpbin
>>> to dump the headers, it shows each section having VirtualSize as 64k
>>> aligned for a runtime image, but the same image doesn't show that in FW.
>>>
>>> I'll do some digging here.
>>>
>>
>> Following up on this:
>>
>> Not surprisingly, different toolchains do different things here.
>>
>> gcc obviously creates ELF files and ElfConvert*.c converts these to PE
>> images. However, when it does this, it always sets the section and file
>> alignment to the same value (I'm not as familiar with ELF images, I'm
>> not sure if there is the same concept as file vs section alignment).
>> GenFw could probably update this information to shrink the file
>> alignment, but that's a space optimization for gcc built binaries.
>>
>> In ElfConvert.c, the VirtualSize does get set to the section aligned
>> value, which is what I would expect from the spec.
>>
>> In MSVC PE images, VirtualSize is not section aligned and the
>> expectation appears to be that loaders will align the VirtualSize
>> to the section alignment. I am planning on reaching out to the MSVC
>> folks to learn why this is the case, is it intended, etc., as my
>> understanding is that the VirtualSize in the section headers should
>> be section aligned.
>>
>> We are stuck with this for existing MSVC toolchains, however, so we
>> will need to align either the VirtualSize or SizeOfRawData to the
>> section alignment when we create the image records. I don't have a
>> preference between the two, they end up being the same when we align
>> them, so I can send a v2 with aligning the VirtualSize. This will be
>> a no-op on gcc built binaries and will set it to the correct value for
>> MSVC.
>>
> 
> I don't think this is correct. The VirtualSize could be much larger
> than the SizeOfRawData (to the extent where the delta exceeds the
> alignment padding), in cases where some of the memory is
> data-initialized and some of it is zero-initialized. We certainly make
> use of this in Linux when constructing the PE/COFF view of the kernel
> image.
> 
> So for the memory view of the image, only VirtualSize is appropriate -
> SizeOfRawData just gives you the size of the data to copy to the start
> of this section.
> 

Sure, I agree that per spec, the VirtualSize should be the correct value
to use. For MSVC built binaries, I have only seen VirtualSize <
SizeOfRawData, but this is just anecdotal. I will follow up with the
MSVC folks and give more information on why MSVC built binaries do not
have the VirtualSize aligned with the SectionAlignment, which would be
my expectation, as VirtualSize should be the size of the image as loaded
into memory.

Thanks,
Oliver


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

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

[Marvin Häuser]

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

Good day everyone,

Sorry for interjecting, but I’d like to avoid premature changes to PE code that would only make the current mess even worse.

> On 4. Mar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote:
> 
> I relooked at the spec, you are correct, SizeOfRawData is aligned to
> the FileAlignment. So this is only a bug for a file with
> SectionAlignment != FileAlignment.

Still no, for some reasons you already mentioned, but also reasons you didn’t:
- VirtualSize often is not aligned, so they can still trivially mismatch.
- VirtualSize includes zero-initialized data not part of the file.
- In the old days, VirtualSize = 0 was used to disable loading of debug data, but SizeOfRawData remained intact, so it could be loaded on demand.
- SizeOfRawData may be unaligned due to bugs. In fact, there is no reason to trust FileAlignment, it has no real meaning in an UEFI context (I am not even sure it does on Windows).

> I see, yes I do believe VirtualSize could be used here instead.

*Must*. As Ard said, SizeOfRawData is only interesting to know how many bytes to copy from the file and for *nothing* else.

> Two
> things give me pause. One is that the PE spec states that SizeOfRawData
> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
> than the VirtualSize in some cases (which seems incorrect).
> 
> The other is that the image loader partially uses VirtualSize when
> loading:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
> 
> However, when determining the size of a loaded image (and therefore the
> number of pages to allocate) it will allocate an extra page:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
> 

Sorry, I don’t understand the “however”, this is sound.

> as ImageSize here is:
> 
> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
> 
> Which according to the spec, SizeOfImage is the size of the image as
> loaded into memory and must be a multiple of section alignment.

Side note, SizeOfImage may be borked, not only due to bugs, but also due to malice. There is literally no reason whatsoever to use it - to validate it, you need to calculate a sane value, and once you have that, you obviously do not need SizeOfImage anymore. You could reject images with malformed SizeOfImage, but there likely are various legitimate-bugged ones out there. This is just an ancient artifact from before memory safety was a thing. :)

> So, reflecting on this, let me test with VirtualSize here, I think
> that is the right value to use, the only time we would have a
> SizeOfRawData that is greater than the VirtualSize would be if our
> FileAlignment is greater than our SectionAlignment, which would be
> a misconfiguration.

No, it can simply be that FileAlignment = SectionAlignment and SizeOfRawData is aligned and VirtualSize is not. Even when both are aligned, the former may carry extra optional data (like previously mentioned debug data), or it might be some weird I/O performance related padding or something.

> I do think the ImageLoader should also be fixed to only allocate
> ImageSize number of pages (which should be the sum of the section
> VirtualSizes + any headers that aren't included). Might as well not
> waste an extra page for each image and then our image record code is
> simpler as well (ensuring we are protecting the right pages).

NO! :) There are *many* issues surrounding this:
- Sections may overlap (observed with early macOS EFI images, could be ignored, but needs to be validated across the landscape).
- Sections may have gaps (observed with “old" Linux EFI Shims iirc, there’ll be plenty of broken images in the wild yet).
- The extra page is not pointless - as you can see, it is allocated when exceeding EFI page size, which is the only granularity you can page-allocate at. So, when you require > 4K alignment, you need the extra page to be able to “shift” the image into proper alignment in the allocated region as needed. In AUDK, I resolved this by abstracting existing, buried aligned-allocation code into a MemoryAllocationLibEx class: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70
Without aligned-allocation, you cannot get rid of this extra page.

For reference, my image record code is here: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335

> I think this patch series should go in, as it is fixing an active bug,
> but I will also take a look at the image loader creating the image
> records and having a protocol it produces to retrieve the list, to
> attempt to avoid issues like this in the future.

Why does there need to be a protocol? Why should non-Core modules get any information about image topology?

A few general words of caution when dealing with PE:
- Do not trust the spec.
- Do not trust the loader.
- Do not trust the converter.
- Do *not* trust your intuition.

*Everything* is broken, from the producers to the consumers, from the signing to the validation tools, technically, security-wise, and design-wise. Do *not* change things without as little as a collection of Windows and Linux images from various versions and various OpROMs to regression-test with. Otherwise, we will all be in a world of more pain. :)

Thanks!

Best regards,
Marvin

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116736): https://edk2.groups.io/g/devel/message/116736
Mute This Topic: https://groups.io/mt/104610770/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: 7900 bytes --]

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

[Oliver Smith-Denny]

Hi Marvin,

Thanks for looking at this. Since sending out the v1, I have done a lot
more digging into our PE loader, others, the spec, and differences
between MSVC and our ElfConvert tool. I agree that my understanding was
flawed in my original comments.

On 3/14/2024 2:57 AM, Marvin Häuser wrote:
> Good day everyone,
> 
> Sorry for interjecting, but I’d like to avoid premature changes to PE 
> code that would only make the current mess even worse.
> 
>> On 4. Mar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote:
>>
>> I relooked at the spec, you are correct, SizeOfRawData is aligned to
>> the FileAlignment. So this is only a bug for a file with
>> SectionAlignment != FileAlignment.
> 
> Still no, for some reasons you already mentioned, but also reasons you 
> didn’t:
> - VirtualSize often is not aligned, so they can still trivially mismatch.
> - VirtualSize includes zero-initialized data not part of the file.

This does not appear to be the case with MSVC built binaries. I am
seeing that VirtualSize is the size of the data-initialized part of the
section. Whereas on our ElfConverted binaries, what you say is true. The
difference concerns me.

> - In the old days, VirtualSize = 0 was used to disable loading of debug 
> data, but SizeOfRawData remained intact, so it could be loaded on demand.
> - SizeOfRawData may be unaligned due to bugs. In fact, there is no 
> reason to trust FileAlignment, it has no real meaning in an UEFI context 
> (I am not even sure it does on Windows).
> 
>> I see, yes I do believe VirtualSize could be used here instead.
> 
> *Must*. As Ard said, SizeOfRawData is only interesting to know how many 
> bytes to copy from the file and for *nothing* else.

Agreed. I have done further research and v2 uses VirtualSize. I was
initially led astray here because the ElfConvert tool sets VirtualSize
and SizeOfRawData to the SectionAlignment universally:

https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136

> 
>> Two
>> things give me pause. One is that the PE spec states that SizeOfRawData
>> is rounded and VirtualSize is not, so that SizeOfRawData may be greater
>> than the VirtualSize in some cases (which seems incorrect).
>>
>> The other is that the image loader partially uses VirtualSize when
>> loading:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400
>>
>> However, when determining the size of a loaded image (and therefore the
>> number of pages to allocate) it will allocate an extra page:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652
>>
> 
> Sorry, I don’t understand the “however”, this is sound.
> 
>> as ImageSize here is:
>>
>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312
>>
>> Which according to the spec, SizeOfImage is the size of the image as
>> loaded into memory and must be a multiple of section alignment.
> 
> Side note, SizeOfImage may be borked, not only due to bugs, but also due 
> to malice. There is literally no reason whatsoever to use it - to 
> validate it, you need to calculate a sane value, and once you have that, 
> you obviously do not need SizeOfImage anymore. You could reject images 
> with malformed SizeOfImage, but there likely are various 
> legitimate-bugged ones out there. This is just an ancient artifact from 
> before memory safety was a thing. :)

Yep, the PE headers are definitely filled with information that can be
deduced from other sources and the spec is not very verbose, which is
why I definitely want to tread carefully here. For example, it says
SizeOfImage is the size of the image loaded into memory, which
current implementations seem to take to mean the sum of the headers
plus each section's VirtualSize rounded to the SectionAlignment (which
the spec does call out that SizeOfImage must be aligned to the
SectionAlignment). VirtualSize it says is also the size of the section
as loaded into memory, but current implementations (other than our
ElfConvert script which I'm currently investigating if we should
change that) seem to take this to mean the size of the initialized
data in memory, i.e. the "real" data from the image, not the zeroed
data to get to the SectionAlignment. But the language in the spec
is almost identical between the two. And again, we have a difference
in our MSVC and gcc built binaries here.

> 
>> So, reflecting on this, let me test with VirtualSize here, I think
>> that is the right value to use, the only time we would have a
>> SizeOfRawData that is greater than the VirtualSize would be if our
>> FileAlignment is greater than our SectionAlignment, which would be
>> a misconfiguration.
> 
> No, it can simply be that FileAlignment = SectionAlignment and 
> SizeOfRawData is aligned and VirtualSize is not. Even when both are 
> aligned, the former may carry extra optional data (like previously 
> mentioned debug data), or it might be some weird I/O performance related 
> padding or something.

Agreed, this was misinformed by looking at gcc built binaries.

> 
>> I do think the ImageLoader should also be fixed to only allocate
>> ImageSize number of pages (which should be the sum of the section
>> VirtualSizes + any headers that aren't included). Might as well not
>> waste an extra page for each image and then our image record code is
>> simpler as well (ensuring we are protecting the right pages).
> 
> NO! :) There are *many* issues surrounding this:
> - Sections may overlap (observed with early macOS EFI images, could be 
> ignored, but needs to be validated across the landscape).
> - Sections may have gaps (observed with “old" Linux EFI Shims iirc, 
> there’ll be plenty of broken images in the wild yet).
> - The extra page is not pointless - as you can see, it is allocated when 
> exceeding EFI page size, which is the only granularity you can 
> page-allocate at. So, when you require > 4K alignment, you need the 
> extra page to be able to “shift” the image into proper alignment in the 
> allocated region as needed. In AUDK, I resolved this by abstracting 
> existing, buried aligned-allocation code into a MemoryAllocationLibEx 
> class: 
> https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70>
> Without aligned-allocation, you cannot get rid of this extra page.
> 
> For reference, my image record code is here: 
> https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335>

Yep, this was me being dumb and not looking a few lines further ahead :)
After sending this I looked deeper at this offhand comment and realized
the purpose.

> 
>> I think this patch series should go in, as it is fixing an active bug,
>> but I will also take a look at the image loader creating the image
>> records and having a protocol it produces to retrieve the list, to
>> attempt to avoid issues like this in the future.
> 
> Why does there need to be a protocol? Why should non-Core modules get 
> any information about image topology?

Agreed, only core code needs this. I think I said protocol originally
so that the existing ImagePropertiesRecordLib could use it without
depending on a core only function, but in a new implementation we
would drop the lib.

> 
> A few general words of caution when dealing with PE:
> - Do not trust the spec.
> - Do not trust the loader.
> - Do not trust the converter.
> - Do *not* trust your intuition.
> 
> *Everything* is broken, from the producers to the consumers, from the 
> signing to the validation tools, technically, security-wise, and 
> design-wise. Do *not* change things without as little as a collection of 
> Windows and Linux images from various versions and various OpROMs to 
> regression-test with. Otherwise, we will all be in a world of more pain. :)
> 

I agree with you there. That being said, the current Image record code
is, unsurprisingly, broken, and does need to be fixed. Can you review
the v2 of this? The only difference is that I changed to VirtualSize.
Do you think the code should be updated for the case of VirtualSize
== 0? In the current implementation, it will round 0 up to the
SectionAlignment, which is what I believe will happen in the PE
loader, also, but want to get your thoughts on it.

This would still be a short term solution to fix the active bug
(ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or
we will boot without the benefit of the MAT, if we happen to have
an aligned total region but not each entry, testing shows). I do
think it is a broken pattern to try to determine the image records
after we've loaded them into memory, the PE loader should create
them with the factual data of what was loaded.

Thanks,
Oliver


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

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

[Marvin Häuser]

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


> On 14. Mar 2024, at 15:45, Oliver Smith-Denny <osde@linux.microsoft.com> wrote:
> 
> This does not appear to be the case with MSVC built binaries. I am
> seeing that VirtualSize is the size of the data-initialized part of the
> section.

What? :( So you are saying modern MSVC generates PEs that either have non-contiguous sections, or they have sections that contain zero-initialized portions explicitly? Both variants sound badly borked and should be investigated, even if only for resource constraints.

Just thinking about it, is there any chance you set ALIGN = FILEALIGN? Because then this will be invoked and your file will be converted to XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612

BaseTools has no concept of what should be a XIP and what should not, so if a file somewhat qualifies, it will just always emit a XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150

In AUDK I addressed this by tying XIP to reloc stripping, but that is more of a convenient heuristic: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293

> Whereas on our ElfConverted binaries, what you say is true. The
> difference concerns me.

Yes, very concerning. Would you mind sharing dumps (section table, etc.) and/or binaries (unless you indeed hit what I outlined above)? I recall MSVC to rather be more aggressive with space-savings, whereas ElfConvert would include section-alignment padding within the file (but not zero-init data).

> Agreed. I have done further research and v2 uses VirtualSize. I was
> initially led astray here because the ElfConvert tool sets VirtualSize
> and SizeOfRawData to the SectionAlignment universally:
> 
> https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136

Yes… plenty of more things in the tool to lead people astray. :)

> Yep, the PE headers are definitely filled with information that can be
> deduced from other sources and the spec is not very verbose, which is
> why I definitely want to tread carefully here. For example, it says
> SizeOfImage is the size of the image loaded into memory, which
> current implementations seem to take to mean the sum of the headers
> plus each section's VirtualSize rounded to the SectionAlignment (which
> the spec does call out that SizeOfImage must be aligned to the
> SectionAlignment). VirtualSize it says is also the size of the section
> as loaded into memory, but current implementations (other than our
> ElfConvert script which I'm currently investigating if we should
> change that) seem to take this to mean the size of the initialized
> data in memory, i.e. the "real" data from the image, not the zeroed
> data to get to the SectionAlignment.

Do you have an example for that? I’m quite certain SizeOfImage should always cover the whole thing.

> But the language in the spec
> is almost identical between the two. And again, we have a difference
> in our MSVC and gcc built binaries here.

Do we? :(

> I agree with you there. That being said, the current Image record code
> is, unsurprisingly, broken, and does need to be fixed. Can you review
> the v2 of this? The only difference is that I changed to VirtualSize.

LGTM.

> Do you think the code should be updated for the case of VirtualSize
> == 0? In the current implementation, it will round 0 up to the
> SectionAlignment

It should not. Well, it will technically round, but it will remain 0. It does not round up to the strictly next boundary, but if the current value is a multiple of the alignment (0 is a multiple of any alignment), it remains unchanged.

> , which is what I believe will happen in the PE
> loader, also, but want to get your thoughts on it.
> 
> This would still be a short term solution to fix the active bug
> (ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or
> we will boot without the benefit of the MAT, if we happen to have
> an aligned total region but not each entry, testing shows). I do
> think it is a broken pattern to try to determine the image records
> after we've loaded them into memory, the PE loader should create
> them with the factual data of what was loaded.

There are many more broken patterns everywhere. Did you notice yet that the API itself is broken from the ground up? Not to keep “advertising” it, but maybe our paper will give you some fun too. :) https://arxiv.org/pdf/2012.05471.pdf

Best regards,
Marvin

> 
> Thanks,
> Oliver



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116780): https://edk2.groups.io/g/devel/message/116780
Mute This Topic: https://groups.io/mt/104610770/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: 36075 bytes --]

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

[Oliver Smith-Denny]

On 3/14/2024 2:57 PM, Marvin Häuser wrote:
> 
>> On 14. Mar 2024, at 15:45, Oliver Smith-Denny 
>> <osde@linux.microsoft.com> wrote:
>>
>> This does not appear to be the case with MSVC built binaries. I am
>> seeing that VirtualSize is the size of the data-initialized part of the
>> section.
> 
> What? :( So you are saying modern MSVC generates PEs that either have 
> non-contiguous sections, or they have sections that contain 
> zero-initialized portions explicitly? Both variants sound badly borked 
> and should be investigated, even if only for resource constraints.
> 

I don't think this is what I'm saying. What I am trying to say is that
on MSVC, I see PE images getting created that have VirtualSize set to
the actual number of initialized bytes in that section (not padded to
the section alignment). On ElfConverted binaries, I see the VirtualSize
is padded to the section alignment. I've dropped an example below

> Just thinking about it, is there any chance you set ALIGN = FILEALIGN? 

No, the specific case where I was researching this was explicitly
setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs
on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData
is aligned to the file alignment, as expected, but VirtualSize would
be the actual size of the data. Again, the troubling thing here for
me is that the same binary built with gcc has the VirtualSize aligned
to the section alignment. And I have seen other code that loads PE
images that relies on VirtualSize not including the padding. The spec
is vague here, it says VirtualSize is the size of the section as
loaded in memory (which would lead me to believe this should include
padding) but it does not explicitly say it should be a multiple of
the section alignment (as other fields do). But at a minimum I think
we should have different toolchains doing the same behavior here.

> Because then this will be invoked and your file will be converted to 
> XIP: 
> https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612>
> 
> BaseTools has no concept of what should be a XIP and what should not, so 
> if a file somewhat qualifies, it will just always emit a XIP: 
> https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150>
> 
> In AUDK I addressed this by tying XIP to reloc stripping, but that is 
> more of a convenient heuristic: 
> https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293>
> 
>> Whereas on our ElfConverted binaries, what you say is true. The
>> difference concerns me.
> 
> Yes, very concerning. Would you mind sharing dumps (section table, etc.) 
> and/or binaries (unless you indeed hit what I outlined above)? I recall 
> MSVC to rather be more aggressive with space-savings, whereas ElfConvert 
> would include section-alignment padding within the file (but not 
> zero-init data).

Correct, ElfConvert sets the file alignment and the section alignment
to the same value, based on common-page-size/max-page-size. I will show
some examples at the bottom.

> 
>> Agreed. I have done further research and v2 uses VirtualSize. I was
>> initially led astray here because the ElfConvert tool sets VirtualSize
>> and SizeOfRawData to the SectionAlignment universally:
>>
>> https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136>
> 
> Yes… plenty of more things in the tool to lead people astray. :)
> 
>> Yep, the PE headers are definitely filled with information that can be
>> deduced from other sources and the spec is not very verbose, which is
>> why I definitely want to tread carefully here. For example, it says
>> SizeOfImage is the size of the image loaded into memory, which
>> current implementations seem to take to mean the sum of the headers
>> plus each section's VirtualSize rounded to the SectionAlignment (which
>> the spec does call out that SizeOfImage must be aligned to the
>> SectionAlignment). VirtualSize it says is also the size of the section
>> as loaded into memory, but current implementations (other than our
>> ElfConvert script which I'm currently investigating if we should
>> change that) seem to take this to mean the size of the initialized
>> data in memory, i.e. the "real" data from the image, not the zeroed
>> data to get to the SectionAlignment.
> 
> Do you have an example for that? I’m quite certain SizeOfImage should 
> always cover the whole thing.

See below for the VirtualSize examples, I'm confused on your comment on
SizeOfImage. I agree that SizeOfImage covers everything as loaded into
memory and I have not seen any issues there.

> 
>> But the language in the spec
>> is almost identical between the two. And again, we have a difference
>> in our MSVC and gcc built binaries here.
> 
> Do we? :(
> 
>> I agree with you there. That being said, the current Image record code
>> is, unsurprisingly, broken, and does need to be fixed. Can you review
>> the v2 of this? The only difference is that I changed to VirtualSize.
> 
> LGTM.

Do you mind adding your RB to v2? And certainly if you have any other
comments that is greatly appreciated.

> 
>> Do you think the code should be updated for the case of VirtualSize
>> == 0? In the current implementation, it will round 0 up to the
>> SectionAlignment
> 
> It should not. Well, it will technically round, but it will remain 0. It 
> does not round up to the strictly next boundary, but if the current 
> value is a multiple of the alignment (0 is a multiple of any alignment), 
> it remains unchanged.
> 
>> , which is what I believe will happen in the PE
>> loader, also, but want to get your thoughts on it.
>>
>> This would still be a short term solution to fix the active bug
>> (ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or
>> we will boot without the benefit of the MAT, if we happen to have
>> an aligned total region but not each entry, testing shows). I do
>> think it is a broken pattern to try to determine the image records
>> after we've loaded them into memory, the PE loader should create
>> them with the factual data of what was loaded.
> 
> There are many more broken patterns everywhere. Did you notice yet that 
> the API itself is broken from the ground up? Not to keep “advertising” 
> it, but maybe our paper will give you some fun too. :) 
> https://arxiv.org/pdf/2012.05471.pdf <https://arxiv.org/pdf/2012.05471.pdf>

I'll take a look. It's been on my list to look at for awhile but as
always other things come up :)

Examples of the differences I see between MSVC and gcc binaries:

I originally noticed this on ARM64 on edk2, but wanted to make sure I
saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg
(edk2 doesn't have VS2022 support and I didn't feel like adding it
or reverting back to VS2019). For reference, this is building the
current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a.

I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin
(from VS2022) to examine the PE headers.

MSVC selected header values:

Main header:
0x3200 size of code
0x2400 size of initialized data
0x0    size of uninitialized data
0x1000 section alignment
0x200  file alignment
0xB000 size of image

6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata

.text section:
0x30DF virtual size
0x3200 size of raw data

.data section:
0x130 virtual size
0x200 size of raw data


GCC ElfConverted selected header values:

Main header:
0x4000 size of code
0x1000 size of initialized data
0x0    size of uninitialized data
0x1000 section alignment
0x1000 file alignment
0x7000 size of image

3 sections: .data, .text, .reloc

.text section:
0x4000 virtual size
0x4000 size of raw data

.data section:
0x1000 virtual size
0x1000 size of raw data

So my concern here is that ElfConvert takes a
different view of VirtualSize, that is should be
section aligned, whereas MSVC binaries take
VirtualSize to be the actual size without padding.
I think the correct thing to do would be change
ElfConvert to do what MSVC does (although the spec
is vague and so I can understand the confusion).
In practice this will tend to work as is, since we
are using SizeOfImage to allocate with, but as you
have pointed out there are so many edge cases that
having a difference here makes me worried we would
see something weird with only binaries built by one toolchain.

I am curious to hear your thoughts though. This is
very easy to reproduce, build any UEFI binary with
MSVC and GCC and compare the headers.

Thanks,
Oliver


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

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

[Marvin Häuser]

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


> On 15. Mar 2024, at 23:57, Oliver Smith-Denny <osde@linux.microsoft.com> wrote:
> 
> I don't think this is what I'm saying. What I am trying to say is that
> on MSVC, I see PE images getting created that have VirtualSize set to
> the actual number of initialized bytes in that section (not padded to
> the section alignment). On ElfConverted binaries, I see the VirtualSize
> is padded to the section alignment. I've dropped an example below

Ah, mismatched terminology. Zero-initialized as Ard and I used it refers to implicitly or explicitly 0-initialized global variables and such, which is not stored in the file, not the padding. So when you mentioned “real data”, I assumed you meant strictly the non-0 data from the file. Same misunderstanding with SizeOfImage, so that’s all fine. Whew. :)

> No, the specific case where I was researching this was explicitly
> setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs
> on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData
> is aligned to the file alignment, as expected, but VirtualSize would
> be the actual size of the data. Again, the troubling thing here for
> me is that the same binary built with gcc has the VirtualSize aligned
> to the section alignment. And I have seen other code that loads PE
> images that relies on VirtualSize not including the padding. The spec
> is vague here, it says VirtualSize is the size of the section as
> loaded in memory (which would lead me to believe this should include
> padding) but it does not explicitly say it should be a multiple of
> the section alignment (as other fields do). But at a minimum I think
> we should have different toolchains doing the same behavior here.

Well, not rounding to pad is somewhat superior in some scenarios. If you round, you lose the information on what is section data and what is padding, so you might end up treating padding as data for some reason (because it is indistinguishable from mentioned 0-initialized data). This shouldn’t matter too much for executables and libraries, but MSVC/PE have a lot less of a distinction between object file and executable/library concepts (e.g. no distinction between sections and segments). That might be why they do it this way.

> See below for the VirtualSize examples, I'm confused on your comment on
> SizeOfImage. I agree that SizeOfImage covers everything as loaded into
> memory and I have not seen any issues there.

See first comment.

> Do you mind adding your RB to v2? And certainly if you have any other
> comments that is greatly appreciated.

Will try to remember tomorrow. :)

> Examples of the differences I see between MSVC and gcc binaries:
> 
> I originally noticed this on ARM64 on edk2, but wanted to make sure I
> saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg
> (edk2 doesn't have VS2022 support and I didn't feel like adding it
> or reverting back to VS2019). For reference, this is building the
> current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a.
> 
> I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin
> (from VS2022) to examine the PE headers.
> 
> MSVC selected header values:
> 
> Main header:
> 0x3200 size of code
> 0x2400 size of initialized data
> 0x0    size of uninitialized data
> 0x1000 section alignment
> 0x200  file alignment
> 0xB000 size of image
> 
> 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata
> 
> .text section:
> 0x30DF virtual size
> 0x3200 size of raw data
> 
> .data section:
> 0x130 virtual size
> 0x200 size of raw data
> 
> 
> GCC ElfConverted selected header values:
> 
> Main header:
> 0x4000 size of code
> 0x1000 size of initialized data
> 0x0    size of uninitialized data
> 0x1000 section alignment
> 0x1000 file alignment
> 0x7000 size of image
> 
> 3 sections: .data, .text, .reloc
> 
> .text section:
> 0x4000 virtual size
> 0x4000 size of raw data
> 
> .data section:
> 0x1000 virtual size
> 0x1000 size of raw data
> 
> So my concern here is that ElfConvert takes a
> different view of VirtualSize, that is should be
> section aligned, whereas MSVC binaries take
> VirtualSize to be the actual size without padding.
> I think the correct thing to do would be change
> ElfConvert to do what MSVC does (although the spec
> is vague and so I can understand the confusion).

I don’t think it really matters, but it wouldn’t hurt either. Both kinds of binaries are in the wild, so you cannot really leverage any of the choices’ advantages either way. Adjusting to MSVC’s behaviour would be right though, as you can at least properly distinguish between padding and 0-data with new binaries.

> In practice this will tend to work as is, since we
> are using SizeOfImage to allocate with, but as you
> have pointed out there are so many edge cases that
> having a difference here makes me worried we would
> see something weird with only binaries built by one toolchain.

There is plenty of room for that as-is, including that MSVC emits .rdata (and others), but GCC does not, and there is a super awkward heuristic in DxeCore to determine section permissions rather than using the PE values at face value - R-- is literally not respected. Also there had been issues with NASM sections, because PE needs .rdata, but ELF needs .rodata naming there.

> I am curious to hear your thoughts though. This is
> very easy to reproduce, build any UEFI binary with
> MSVC and GCC and compare the headers.

Yes, sorry for the confusion, this all looks as expected.

Best regards,
Marvin

> 
> Thanks,
> Oliver


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



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2422 bytes --]

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

[Oliver Smith-Denny]

On 3/15/2024 4:45 PM, Marvin Häuser wrote:
> 
>> On 15. Mar 2024, at 23:57, Oliver Smith-Denny <osde@linux.microsoft.com> wrote:
>>
>> I don't think this is what I'm saying. What I am trying to say is that
>> on MSVC, I see PE images getting created that have VirtualSize set to
>> the actual number of initialized bytes in that section (not padded to
>> the section alignment). On ElfConverted binaries, I see the VirtualSize
>> is padded to the section alignment. I've dropped an example below
> 
> Ah, mismatched terminology. Zero-initialized as Ard and I used it refers to implicitly or explicitly 0-initialized global variables and such, which is not stored in the file, not the padding. So when you mentioned “real data”, I assumed you meant strictly the non-0 data from the file. Same misunderstanding with SizeOfImage, so that’s all fine. Whew. :)

Ah gotcha, thanks I was figuring I was using the wrong terminology :).
What you said makes sense and I see your concern based on what I said.

> 
>> No, the specific case where I was researching this was explicitly
>> setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs
>> on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData
>> is aligned to the file alignment, as expected, but VirtualSize would
>> be the actual size of the data. Again, the troubling thing here for
>> me is that the same binary built with gcc has the VirtualSize aligned
>> to the section alignment. And I have seen other code that loads PE
>> images that relies on VirtualSize not including the padding. The spec
>> is vague here, it says VirtualSize is the size of the section as
>> loaded in memory (which would lead me to believe this should include
>> padding) but it does not explicitly say it should be a multiple of
>> the section alignment (as other fields do). But at a minimum I think
>> we should have different toolchains doing the same behavior here.
> 
> Well, not rounding to pad is somewhat superior in some scenarios. If you round, you lose the information on what is section data and what is padding, so you might end up treating padding as data for some reason (because it is indistinguishable from mentioned 0-initialized data). This shouldn’t matter too much for executables and libraries, but MSVC/PE have a lot less of a distinction between object file and executable/library concepts (e.g. no distinction between sections and segments). That might be why they do it this way.
> 

I agree, I've seen other environments that will use VirtualSize to set
attributes for that section and then set stricter attributes, like RP
on the padded section (if greater than a page). Point being that there
are use cases and at least some folks relying on that definition of
VirtualSize.

>> See below for the VirtualSize examples, I'm confused on your comment on
>> SizeOfImage. I agree that SizeOfImage covers everything as loaded into
>> memory and I have not seen any issues there.
> 
> See first comment.
> 
>> Do you mind adding your RB to v2? And certainly if you have any other
>> comments that is greatly appreciated.
> 
> Will try to remember tomorrow. :)
> 

Thanks!

>> Examples of the differences I see between MSVC and gcc binaries:
>>
>> I originally noticed this on ARM64 on edk2, but wanted to make sure I
>> saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg
>> (edk2 doesn't have VS2022 support and I didn't feel like adding it
>> or reverting back to VS2019). For reference, this is building the
>> current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a.
>>
>> I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin
>> (from VS2022) to examine the PE headers.
>>
>> MSVC selected header values:
>>
>> Main header:
>> 0x3200 size of code
>> 0x2400 size of initialized data
>> 0x0    size of uninitialized data
>> 0x1000 section alignment
>> 0x200  file alignment
>> 0xB000 size of image
>>
>> 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata
>>
>> .text section:
>> 0x30DF virtual size
>> 0x3200 size of raw data
>>
>> .data section:
>> 0x130 virtual size
>> 0x200 size of raw data
>>
>>
>> GCC ElfConverted selected header values:
>>
>> Main header:
>> 0x4000 size of code
>> 0x1000 size of initialized data
>> 0x0    size of uninitialized data
>> 0x1000 section alignment
>> 0x1000 file alignment
>> 0x7000 size of image
>>
>> 3 sections: .data, .text, .reloc
>>
>> .text section:
>> 0x4000 virtual size
>> 0x4000 size of raw data
>>
>> .data section:
>> 0x1000 virtual size
>> 0x1000 size of raw data
>>
>> So my concern here is that ElfConvert takes a
>> different view of VirtualSize, that is should be
>> section aligned, whereas MSVC binaries take
>> VirtualSize to be the actual size without padding.
>> I think the correct thing to do would be change
>> ElfConvert to do what MSVC does (although the spec
>> is vague and so I can understand the confusion).
> 
> I don’t think it really matters, but it wouldn’t hurt either. Both kinds of binaries are in the wild, so you cannot really leverage any of the choices’ advantages either way. Adjusting to MSVC’s behaviour would be right though, as you can at least properly distinguish between padding and 0-data with new binaries.

Yeah, I agree. I may take a look at this, but to your
point from an earlier email, it may be safer to leave
it as is, considering we have all these binaries in the
wild already and changes in a broken environment are
risky.

Thanks,
Oliver


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