[PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack
@ 2018-09-06 13:45 Ard Biesheuvel
  2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 13:45 UTC (permalink / raw)
  To: edk2-devel
  Cc: Ard Biesheuvel, Star Zeng, Jian J Wang, Michael D Kinney,
	Liming Gao, Chao Zhang, Jiewen Yao, Laszlo Ersek, Leif Lindholm

Now that Itanium support has been dropped from EDK2, we can remove all 11 (!)
occurrences of the ELILO PE/COFF loader hack from the code base.

Note that SecurityPkg appears to have four mostly identical implementations
of the PE/COFF measuring routine, so this may be another area for cleanup
later.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816

Cc: Star Zeng <star.zeng@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>

Ard Biesheuvel (4):
  MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF
  MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on
    IPF
  SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
  EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF

 .../Library/BasePeCoffLib/BasePeCoff.c        | 60 +++---------------
 .../Core/Dxe/Mem/MemoryProfileRecord.c        | 31 +---------
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 17 +-----
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  | 17 +-----
 .../Core/PiSmmCore/MemoryAttributesTable.c    | 17 +-----
 .../Core/PiSmmCore/SmramProfileRecord.c       | 31 +---------
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c     | 61 +++----------------
 .../DxeImageVerificationLib.c                 | 47 +++-----------
 .../DxeTpmMeasureBootLib.c                    | 27 ++------
 SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c   | 27 ++------
 .../SecureBootConfigImpl.c                    | 25 ++------
 11 files changed, 47 insertions(+), 313 deletions(-)

-- 
2.18.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
@ 2018-09-06 13:45 ` Ard Biesheuvel
  2018-09-06 16:29   ` Laszlo Ersek
  2018-09-07  5:15   ` Zeng, Star
  2018-09-06 13:45 ` [PATCH 2/4] MdePkg/BasePeCoffLib: " Ard Biesheuvel
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 13:45 UTC (permalink / raw)
  To: edk2-devel
  Cc: Ard Biesheuvel, Star Zeng, Jian J Wang, Michael D Kinney,
	Liming Gao, Chao Zhang, Jiewen Yao, Laszlo Ersek, Leif Lindholm

Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c     | 31 +-------------------
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c       | 17 +----------
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c        | 17 +----------
 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 17 +----------
 MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c    | 31 +-------------------
 5 files changed, 5 insertions(+), 108 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
index 83ed43a16e95..ff1940431c2f 100644
--- a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
+++ b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
@@ -254,35 +254,6 @@ GetMemoryProfileContext (
   return mMemoryProfileContextPtr;
 }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-InternalPeCoffGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
 /**
   Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
   If Pe32Data is NULL, then ASSERT().
@@ -319,7 +290,7 @@ InternalPeCoffGetSubsystem (
   if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
     return Hdr.Te->Subsystem;
   } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
-    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
+    Magic = Hdr.Pe32->OptionalHeader.Magic;
     if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       return Hdr.Pe32->OptionalHeader.Subsystem;
     } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 4cd219c88efc..fa8f8fe91ac7 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -406,7 +406,6 @@ ProtectUefiImage (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
   BOOLEAN                              IsAligned;
   UINT32                               ProtectionPolicy;
 
@@ -466,21 +465,7 @@ ProtectUefiImage (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index a96d442fbc64..05eb4f422b2f 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -1076,7 +1076,6 @@ InsertImageRecord (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
 
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%x\n", RuntimeImage));
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize));
@@ -1126,21 +1125,7 @@ InsertImageRecord (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
index 1682d0f9e404..157beb1e67ff 100644
--- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
@@ -1032,7 +1032,6 @@ SmmInsertImageRecord (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
 
   DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%x\n", DriverEntry));
   DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%016lx - 0x%08x\n", DriverEntry->ImageBuffer, DriverEntry->NumberOfPage));
@@ -1076,21 +1075,7 @@ SmmInsertImageRecord (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index b586afa7948e..e8d60c2d2357 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -255,35 +255,6 @@ GetSmramProfileContext (
   return mSmramProfileContextPtr;
 }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-InternalPeCoffGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
 /**
   Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
   If Pe32Data is NULL, then ASSERT().
@@ -320,7 +291,7 @@ InternalPeCoffGetSubsystem (
   if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
     return Hdr.Te->Subsystem;
   } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
-    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
+    Magic = Hdr.Pe32->OptionalHeader.Magic;
     if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       return Hdr.Pe32->OptionalHeader.Subsystem;
     } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
-- 
2.18.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
@ 2018-09-06 16:29   ` Laszlo Ersek
  2018-09-07  5:15   ` Zeng, Star
  1 sibling, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 16:29 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel
  Cc: Liming Gao, Jiewen Yao, Star Zeng, Michael D Kinney, Chao Zhang

On 09/06/18 15:45, Ard Biesheuvel wrote:
> Now that Itanium support has been dropped, we can remove the various
> occurrences of the ELILO on Itanium PE/COFF header workaround.
> 
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c     | 31 +-------------------
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c       | 17 +----------
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c        | 17 +----------
>  MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 17 +----------
>  MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c    | 31 +-------------------
>  5 files changed, 5 insertions(+), 108 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
> index 83ed43a16e95..ff1940431c2f 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
> @@ -254,35 +254,6 @@ GetMemoryProfileContext (
>    return mMemoryProfileContextPtr;
>  }
>  
> -/**
> -  Retrieves the magic value from the PE/COFF header.
> -
> -  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
> -
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
> -
> -**/
> -UINT16
> -InternalPeCoffGetPeHeaderMagicValue (
> -  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
> -  )
> -{
> -  //
> -  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
> -  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -  //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  }
> -  //
> -  // Return the magic value from the PC/COFF Optional Header
> -  //
> -  return Hdr.Pe32->OptionalHeader.Magic;
> -}
> -
>  /**
>    Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
>    If Pe32Data is NULL, then ASSERT().
> @@ -319,7 +290,7 @@ InternalPeCoffGetSubsystem (
>    if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
>      return Hdr.Te->Subsystem;
>    } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
> -    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
> +    Magic = Hdr.Pe32->OptionalHeader.Magic;
>      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        return Hdr.Pe32->OptionalHeader.Subsystem;
>      } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 4cd219c88efc..fa8f8fe91ac7 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -406,7 +406,6 @@ ProtectUefiImage (
>    IMAGE_PROPERTIES_RECORD              *ImageRecord;
>    CHAR8                                *PdbPointer;
>    IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
> -  UINT16                               Magic;
>    BOOLEAN                              IsAligned;
>    UINT32                               ProtectionPolicy;
>  
> @@ -466,21 +465,7 @@ ProtectUefiImage (
>    //
>    // Get SectionAlignment
>    //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
> -  }
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
>    } else {
>      SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index a96d442fbc64..05eb4f422b2f 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -1076,7 +1076,6 @@ InsertImageRecord (
>    IMAGE_PROPERTIES_RECORD              *ImageRecord;
>    CHAR8                                *PdbPointer;
>    IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
> -  UINT16                               Magic;
>  
>    DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%x\n", RuntimeImage));
>    DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize));
> @@ -1126,21 +1125,7 @@ InsertImageRecord (
>    //
>    // Get SectionAlignment
>    //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
> -  }
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
>    } else {
>      SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
> diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> index 1682d0f9e404..157beb1e67ff 100644
> --- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> +++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
> @@ -1032,7 +1032,6 @@ SmmInsertImageRecord (
>    IMAGE_PROPERTIES_RECORD              *ImageRecord;
>    CHAR8                                *PdbPointer;
>    IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
> -  UINT16                               Magic;
>  
>    DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%x\n", DriverEntry));
>    DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%016lx - 0x%08x\n", DriverEntry->ImageBuffer, DriverEntry->NumberOfPage));
> @@ -1076,21 +1075,7 @@ SmmInsertImageRecord (
>    //
>    // Get SectionAlignment
>    //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
> -  }
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
>    } else {
>      SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
> diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
> index b586afa7948e..e8d60c2d2357 100644
> --- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
> +++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
> @@ -255,35 +255,6 @@ GetSmramProfileContext (
>    return mSmramProfileContextPtr;
>  }
>  
> -/**
> -  Retrieves the magic value from the PE/COFF header.
> -
> -  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
> -
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
> -
> -**/
> -UINT16
> -InternalPeCoffGetPeHeaderMagicValue (
> -  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
> -  )
> -{
> -  //
> -  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
> -  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -  //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  }
> -  //
> -  // Return the magic value from the PC/COFF Optional Header
> -  //
> -  return Hdr.Pe32->OptionalHeader.Magic;
> -}
> -
>  /**
>    Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
>    If Pe32Data is NULL, then ASSERT().
> @@ -320,7 +291,7 @@ InternalPeCoffGetSubsystem (
>    if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
>      return Hdr.Te->Subsystem;
>    } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
> -    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
> +    Magic = Hdr.Pe32->OptionalHeader.Magic;
>      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        return Hdr.Pe32->OptionalHeader.Subsystem;
>      } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
  2018-09-06 16:29   ` Laszlo Ersek
@ 2018-09-07  5:15   ` Zeng, Star
  1 sibling, 0 replies; 18+ messages in thread
From: Zeng, Star @ 2018-09-07  5:15 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel@lists.01.org
  Cc: Gao, Liming, Yao, Jiewen, Kinney, Michael D, Laszlo Ersek,
	Zhang, Chao B, Zeng, Star

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Thursday, September 6, 2018 9:45 PM
To: edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: [edk2] [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF

Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c     | 31 +-------------------
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c       | 17 +----------
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c        | 17 +----------
 MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c | 17 +----------
 MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c    | 31 +-------------------
 5 files changed, 5 insertions(+), 108 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
index 83ed43a16e95..ff1940431c2f 100644
--- a/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
+++ b/MdeModulePkg/Core/Dxe/Mem/MemoryProfileRecord.c
@@ -254,35 +254,6 @@ GetMemoryProfileContext (
   return mMemoryProfileContextPtr;
 }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-InternalPeCoffGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
 /**
   Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
   If Pe32Data is NULL, then ASSERT().
@@ -319,7 +290,7 @@ InternalPeCoffGetSubsystem (
   if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
     return Hdr.Te->Subsystem;
   } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
-    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
+    Magic = Hdr.Pe32->OptionalHeader.Magic;
     if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       return Hdr.Pe32->OptionalHeader.Subsystem;
     } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 4cd219c88efc..fa8f8fe91ac7 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -406,7 +406,6 @@ ProtectUefiImage (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
   BOOLEAN                              IsAligned;
   UINT32                               ProtectionPolicy;
 
@@ -466,21 +465,7 @@ ProtectUefiImage (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index a96d442fbc64..05eb4f422b2f 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -1076,7 +1076,6 @@ InsertImageRecord (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
 
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%x\n", RuntimeImage));
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize));
@@ -1126,21 +1125,7 @@ InsertImageRecord (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
index 1682d0f9e404..157beb1e67ff 100644
--- a/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/PiSmmCore/MemoryAttributesTable.c
@@ -1032,7 +1032,6 @@ SmmInsertImageRecord (
   IMAGE_PROPERTIES_RECORD              *ImageRecord;
   CHAR8                                *PdbPointer;
   IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-  UINT16                               Magic;
 
   DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%x\n", DriverEntry));
   DEBUG ((DEBUG_VERBOSE, "SMM InsertImageRecord - 0x%016lx - 0x%08x\n", DriverEntry->ImageBuffer, DriverEntry->NumberOfPage));
@@ -1076,21 +1075,7 @@ SmmInsertImageRecord (
   //
   // Get SectionAlignment
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
   } else {
     SectionAlignment  = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
diff --git a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
index b586afa7948e..e8d60c2d2357 100644
--- a/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
+++ b/MdeModulePkg/Core/PiSmmCore/SmramProfileRecord.c
@@ -255,35 +255,6 @@ GetSmramProfileContext (
   return mSmramProfileContextPtr;
 }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param Hdr    The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-InternalPeCoffGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
 /**
   Retrieves and returns the Subsystem of a PE/COFF image that has been loaded into system memory.
   If Pe32Data is NULL, then ASSERT().
@@ -320,7 +291,7 @@ InternalPeCoffGetSubsystem (
   if (Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) {
     return Hdr.Te->Subsystem;
   } else if (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)  {
-    Magic = InternalPeCoffGetPeHeaderMagicValue (Hdr);
+    Magic = Hdr.Pe32->OptionalHeader.Magic;
     if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       return Hdr.Pe32->OptionalHeader.Subsystem;
     } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
-- 
2.18.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
  2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
@ 2018-09-06 13:45 ` Ard Biesheuvel
  2018-09-06 16:31   ` Laszlo Ersek
  2018-09-07  5:15   ` Zeng, Star
  2018-09-06 13:45 ` [PATCH 3/4] SecurityPkg: " Ard Biesheuvel
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 13:45 UTC (permalink / raw)
  To: edk2-devel
  Cc: Ard Biesheuvel, Star Zeng, Jian J Wang, Michael D Kinney,
	Liming Gao, Chao Zhang, Jiewen Yao, Laszlo Ersek, Leif Lindholm

Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 61 +++-----------------
 1 file changed, 9 insertions(+), 52 deletions(-)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 32eca0ad2ef4..c57816a80887 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -46,36 +46,6 @@ PeCoffLoaderAdjustOffsetForTeImage (
   SectionHeader->PointerToRawData -= TeStrippedOffset;
 }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-PeCoffLoaderGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
-
 /**
   Retrieves the PE or TE Header from a PE/COFF or TE image.
 
@@ -101,7 +71,6 @@ PeCoffLoaderGetPeHeader (
   EFI_IMAGE_DOS_HEADER  DosHdr;
   UINTN                 Size;
   UINTN                 ReadSize;
-  UINT16                Magic;
   UINT32                SectionHeaderOffset;
   UINT32                Index;
   UINT32                HeaderWithoutDataDir;
@@ -222,9 +191,7 @@ PeCoffLoaderGetPeHeader (
     ImageContext->IsTeImage = FALSE;
     ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // 1. Check OptionalHeader.NumberOfRvaAndSizes filed.
       //
@@ -339,7 +306,7 @@ PeCoffLoaderGetPeHeader (
       ImageContext->SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
       ImageContext->SizeOfHeaders     = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
 
-    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+    } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
       //
       // 1. Check FileHeader.NumberOfRvaAndSizes filed.
       //
@@ -605,7 +572,6 @@ PeCoffLoaderGetImageInfo (
   EFI_IMAGE_SECTION_HEADER              SectionHeader;
   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   UINT32                                TeStrippedOffset;
 
   if (ImageContext == NULL) {
@@ -622,14 +588,12 @@ PeCoffLoaderGetImageInfo (
     return Status;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
   //
   // Retrieve the base address of the image
   //
   if (!(ImageContext->IsTeImage)) {
     TeStrippedOffset = 0;
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -678,7 +642,7 @@ PeCoffLoaderGetImageInfo (
   }
 
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -952,7 +916,6 @@ PeCoffLoaderRelocateImage (
   CHAR8                                 *FixupData;
   PHYSICAL_ADDRESS                      BaseAddress;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   UINT32                                TeStrippedOffset;
 
   ASSERT (ImageContext != NULL);
@@ -985,9 +948,8 @@ PeCoffLoaderRelocateImage (
   if (!(ImageContext->IsTeImage)) {
     Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext->ImageAddress + ImageContext->PeCoffHeaderOffset);
     TeStrippedOffset = 0;
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
 
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1230,7 +1192,6 @@ PeCoffLoaderLoadImage (
   UINTN                                 Size;
   UINT32                                TempDebugEntryRva;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   EFI_IMAGE_RESOURCE_DIRECTORY          *ResourceDirectory;
   EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY    *ResourceDirectoryEntry;
   EFI_IMAGE_RESOURCE_DIRECTORY_STRING   *ResourceDirectoryString;
@@ -1404,12 +1365,11 @@ PeCoffLoaderLoadImage (
   //
   // Get image's entry point
   //
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
   if (!(ImageContext->IsTeImage)) {
     //
     // Sizes of AddressOfEntryPoint are different so we need to do this safely
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1444,7 +1404,7 @@ PeCoffLoaderLoadImage (
   // the optional header to verify a desired directory entry is there.
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1565,7 +1525,7 @@ PeCoffLoaderLoadImage (
   //
   ImageContext->HiiResourceData = 0;
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1721,7 +1681,6 @@ PeCoffLoaderRelocateImageForRuntime (
   CHAR8                               *FixupData;
   UINTN                               Adjust;
   RETURN_STATUS                       Status;
-  UINT16                              Magic;
 
   OldBase = (CHAR8 *)((UINTN)ImageBase);
   NewBase = (CHAR8 *)((UINTN)VirtImageBase);
@@ -1750,9 +1709,7 @@ PeCoffLoaderRelocateImageForRuntime (
     return ;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
-- 
2.18.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 2/4] MdePkg/BasePeCoffLib: " Ard Biesheuvel
@ 2018-09-06 16:31   ` Laszlo Ersek
  2018-09-07  5:15   ` Zeng, Star
  1 sibling, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 16:31 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel
  Cc: Liming Gao, Jiewen Yao, Star Zeng, Michael D Kinney, Chao Zhang

On 09/06/18 15:45, Ard Biesheuvel wrote:
> Now that Itanium support has been dropped, we can remove the various
> occurrences of the ELILO on Itanium PE/COFF header workaround.
> 
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 61 +++-----------------
>  1 file changed, 9 insertions(+), 52 deletions(-)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 32eca0ad2ef4..c57816a80887 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -46,36 +46,6 @@ PeCoffLoaderAdjustOffsetForTeImage (
>    SectionHeader->PointerToRawData -= TeStrippedOffset;
>  }
>  
> -/**
> -  Retrieves the magic value from the PE/COFF header.
> -
> -  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE header.
> -
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
> -  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
> -
> -**/
> -UINT16
> -PeCoffLoaderGetPeHeaderMagicValue (
> -  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
> -  )
> -{
> -  //
> -  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
> -  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -  //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  }
> -  //
> -  // Return the magic value from the PC/COFF Optional Header
> -  //
> -  return Hdr.Pe32->OptionalHeader.Magic;
> -}
> -
> -
>  /**
>    Retrieves the PE or TE Header from a PE/COFF or TE image.
>  
> @@ -101,7 +71,6 @@ PeCoffLoaderGetPeHeader (
>    EFI_IMAGE_DOS_HEADER  DosHdr;
>    UINTN                 Size;
>    UINTN                 ReadSize;
> -  UINT16                Magic;
>    UINT32                SectionHeaderOffset;
>    UINT32                Index;
>    UINT32                HeaderWithoutDataDir;
> @@ -222,9 +191,7 @@ PeCoffLoaderGetPeHeader (
>      ImageContext->IsTeImage = FALSE;
>      ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
>  
> -    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
> -
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // 1. Check OptionalHeader.NumberOfRvaAndSizes filed.
>        //
> @@ -339,7 +306,7 @@ PeCoffLoaderGetPeHeader (
>        ImageContext->SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
>        ImageContext->SizeOfHeaders     = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
>  
> -    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> +    } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>        //
>        // 1. Check FileHeader.NumberOfRvaAndSizes filed.
>        //
> @@ -605,7 +572,6 @@ PeCoffLoaderGetImageInfo (
>    EFI_IMAGE_SECTION_HEADER              SectionHeader;
>    EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
>    UINT32                                NumberOfRvaAndSizes;
> -  UINT16                                Magic;
>    UINT32                                TeStrippedOffset;
>  
>    if (ImageContext == NULL) {
> @@ -622,14 +588,12 @@ PeCoffLoaderGetImageInfo (
>      return Status;
>    }
>  
> -  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
> -
>    //
>    // Retrieve the base address of the image
>    //
>    if (!(ImageContext->IsTeImage)) {
>      TeStrippedOffset = 0;
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -678,7 +642,7 @@ PeCoffLoaderGetImageInfo (
>    }
>  
>    if (!(ImageContext->IsTeImage)) {
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -952,7 +916,6 @@ PeCoffLoaderRelocateImage (
>    CHAR8                                 *FixupData;
>    PHYSICAL_ADDRESS                      BaseAddress;
>    UINT32                                NumberOfRvaAndSizes;
> -  UINT16                                Magic;
>    UINT32                                TeStrippedOffset;
>  
>    ASSERT (ImageContext != NULL);
> @@ -985,9 +948,8 @@ PeCoffLoaderRelocateImage (
>    if (!(ImageContext->IsTeImage)) {
>      Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext->ImageAddress + ImageContext->PeCoffHeaderOffset);
>      TeStrippedOffset = 0;
> -    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>  
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -1230,7 +1192,6 @@ PeCoffLoaderLoadImage (
>    UINTN                                 Size;
>    UINT32                                TempDebugEntryRva;
>    UINT32                                NumberOfRvaAndSizes;
> -  UINT16                                Magic;
>    EFI_IMAGE_RESOURCE_DIRECTORY          *ResourceDirectory;
>    EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY    *ResourceDirectoryEntry;
>    EFI_IMAGE_RESOURCE_DIRECTORY_STRING   *ResourceDirectoryString;
> @@ -1404,12 +1365,11 @@ PeCoffLoaderLoadImage (
>    //
>    // Get image's entry point
>    //
> -  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>    if (!(ImageContext->IsTeImage)) {
>      //
>      // Sizes of AddressOfEntryPoint are different so we need to do this safely
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -1444,7 +1404,7 @@ PeCoffLoaderLoadImage (
>    // the optional header to verify a desired directory entry is there.
>    //
>    if (!(ImageContext->IsTeImage)) {
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -1565,7 +1525,7 @@ PeCoffLoaderLoadImage (
>    //
>    ImageContext->HiiResourceData = 0;
>    if (!(ImageContext->IsTeImage)) {
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -1721,7 +1681,6 @@ PeCoffLoaderRelocateImageForRuntime (
>    CHAR8                               *FixupData;
>    UINTN                               Adjust;
>    RETURN_STATUS                       Status;
> -  UINT16                              Magic;
>  
>    OldBase = (CHAR8 *)((UINTN)ImageBase);
>    NewBase = (CHAR8 *)((UINTN)VirtImageBase);
> @@ -1750,9 +1709,7 @@ PeCoffLoaderRelocateImageForRuntime (
>      return ;
>    }
>  
> -  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
> -
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset
>      //
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 2/4] MdePkg/BasePeCoffLib: " Ard Biesheuvel
  2018-09-06 16:31   ` Laszlo Ersek
@ 2018-09-07  5:15   ` Zeng, Star
  2018-09-07  5:36     ` Gao, Liming
  1 sibling, 1 reply; 18+ messages in thread
From: Zeng, Star @ 2018-09-07  5:15 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel@lists.01.org
  Cc: Wang, Jian J, Kinney, Michael D, Gao, Liming, Zhang, Chao B,
	Yao, Jiewen, Laszlo Ersek, Leif Lindholm, Zeng, Star

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Thursday, September 6, 2018 9:45 PM
To: edk2-devel@lists.01.org
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>
Subject: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on IPF

Now that Itanium support has been dropped, we can remove the various occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 61 +++-----------------
 1 file changed, 9 insertions(+), 52 deletions(-)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 32eca0ad2ef4..c57816a80887 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -46,36 +46,6 @@ PeCoffLoaderAdjustOffsetForTeImage (
   SectionHeader->PointerToRawData -= TeStrippedOffset;  }
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-PeCoffLoaderGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic; -}
-
-
 /**
   Retrieves the PE or TE Header from a PE/COFF or TE image.
 
@@ -101,7 +71,6 @@ PeCoffLoaderGetPeHeader (
   EFI_IMAGE_DOS_HEADER  DosHdr;
   UINTN                 Size;
   UINTN                 ReadSize;
-  UINT16                Magic;
   UINT32                SectionHeaderOffset;
   UINT32                Index;
   UINT32                HeaderWithoutDataDir;
@@ -222,9 +191,7 @@ PeCoffLoaderGetPeHeader (
     ImageContext->IsTeImage = FALSE;
     ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // 1. Check OptionalHeader.NumberOfRvaAndSizes filed.
       //
@@ -339,7 +306,7 @@ PeCoffLoaderGetPeHeader (
       ImageContext->SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
       ImageContext->SizeOfHeaders     = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
 
-    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+    } else if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
       //
       // 1. Check FileHeader.NumberOfRvaAndSizes filed.
       //
@@ -605,7 +572,6 @@ PeCoffLoaderGetImageInfo (
   EFI_IMAGE_SECTION_HEADER              SectionHeader;
   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   UINT32                                TeStrippedOffset;
 
   if (ImageContext == NULL) {
@@ -622,14 +588,12 @@ PeCoffLoaderGetImageInfo (
     return Status;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
   //
   // Retrieve the base address of the image
   //
   if (!(ImageContext->IsTeImage)) {
     TeStrippedOffset = 0;
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -678,7 +642,7 @@ PeCoffLoaderGetImageInfo (
   }
 
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -952,7 +916,6 @@ PeCoffLoaderRelocateImage (
   CHAR8                                 *FixupData;
   PHYSICAL_ADDRESS                      BaseAddress;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   UINT32                                TeStrippedOffset;
 
   ASSERT (ImageContext != NULL);
@@ -985,9 +948,8 @@ PeCoffLoaderRelocateImage (
   if (!(ImageContext->IsTeImage)) {
     Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext->ImageAddress + ImageContext->PeCoffHeaderOffset);
     TeStrippedOffset = 0;
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
 
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1230,7 +1192,6 @@ PeCoffLoaderLoadImage (
   UINTN                                 Size;
   UINT32                                TempDebugEntryRva;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
   EFI_IMAGE_RESOURCE_DIRECTORY          *ResourceDirectory;
   EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY    *ResourceDirectoryEntry;
   EFI_IMAGE_RESOURCE_DIRECTORY_STRING   *ResourceDirectoryString;
@@ -1404,12 +1365,11 @@ PeCoffLoaderLoadImage (
   //
   // Get image's entry point
   //
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
   if (!(ImageContext->IsTeImage)) {
     //
     // Sizes of AddressOfEntryPoint are different so we need to do this safely
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1444,7 +1404,7 @@ PeCoffLoaderLoadImage (
   // the optional header to verify a desired directory entry is there.
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1565,7 +1525,7 @@ PeCoffLoaderLoadImage (
   //
   ImageContext->HiiResourceData = 0;
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1721,7 +1681,6 @@ PeCoffLoaderRelocateImageForRuntime (
   CHAR8                               *FixupData;
   UINTN                               Adjust;
   RETURN_STATUS                       Status;
-  UINT16                              Magic;
 
   OldBase = (CHAR8 *)((UINTN)ImageBase);
   NewBase = (CHAR8 *)((UINTN)VirtImageBase); @@ -1750,9 +1709,7 @@ PeCoffLoaderRelocateImageForRuntime (
     return ;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == 
+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
--
2.18.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on IPF
  2018-09-07  5:15   ` Zeng, Star
@ 2018-09-07  5:36     ` Gao, Liming
  0 siblings, 0 replies; 18+ messages in thread
From: Gao, Liming @ 2018-09-07  5:36 UTC (permalink / raw)
  To: Zeng, Star, Ard Biesheuvel, edk2-devel@lists.01.org
  Cc: Wang, Jian J, Kinney, Michael D, Zhang, Chao B, Yao, Jiewen,
	Laszlo Ersek, Leif Lindholm

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Zeng, Star
>Sent: Friday, September 07, 2018 1:15 PM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
>Cc: Wang, Jian J <jian.j.wang@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhang,
>Chao B <chao.b.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
>Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>;
>Zeng, Star <star.zeng@intel.com>
>Subject: RE: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header
>workaround for ELILO on IPF
>
>Reviewed-by: Star Zeng <star.zeng@intel.com>
>
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Thursday, September 6, 2018 9:45 PM
>To: edk2-devel@lists.01.org
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zeng, Star
><star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Kinney,
>Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Yao,
>Jiewen <jiewen.yao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>Lindholm <leif.lindholm@linaro.org>
>Subject: [PATCH 2/4] MdePkg/BasePeCoffLib: remove PE/COFF header
>workaround for ELILO on IPF
>
>Now that Itanium support has been dropped, we can remove the various
>occurrences of the ELILO on Itanium PE/COFF header workaround.
>
>Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 61 +++-----------------
> 1 file changed, 9 insertions(+), 52 deletions(-)
>
>diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>index 32eca0ad2ef4..c57816a80887 100644
>--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>@@ -46,36 +46,6 @@ PeCoffLoaderAdjustOffsetForTeImage (
>   SectionHeader->PointerToRawData -= TeStrippedOffset;  }
>
>-/**
>-  Retrieves the magic value from the PE/COFF header.
>-
>-  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE
>header.
>-
>-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
>-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
>-
>-**/
>-UINT16
>-PeCoffLoaderGetPeHeaderMagicValue (
>-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
>-  )
>-{
>-  //
>-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic
>value
>-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the
>-  //       Magic value in the OptionalHeader is
>EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>-  //       then override the returned value to
>EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>-  //
>-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 &&
>Hdr.Pe32->OptionalHeader.Magic ==
>EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>-  }
>-  //
>-  // Return the magic value from the PC/COFF Optional Header
>-  //
>-  return Hdr.Pe32->OptionalHeader.Magic; -}
>-
>-
> /**
>   Retrieves the PE or TE Header from a PE/COFF or TE image.
>
>@@ -101,7 +71,6 @@ PeCoffLoaderGetPeHeader (
>   EFI_IMAGE_DOS_HEADER  DosHdr;
>   UINTN                 Size;
>   UINTN                 ReadSize;
>-  UINT16                Magic;
>   UINT32                SectionHeaderOffset;
>   UINT32                Index;
>   UINT32                HeaderWithoutDataDir;
>@@ -222,9 +191,7 @@ PeCoffLoaderGetPeHeader (
>     ImageContext->IsTeImage = FALSE;
>     ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
>
>-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>-
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // 1. Check OptionalHeader.NumberOfRvaAndSizes filed.
>       //
>@@ -339,7 +306,7 @@ PeCoffLoaderGetPeHeader (
>       ImageContext->SectionAlignment  = Hdr.Pe32-
>>OptionalHeader.SectionAlignment;
>       ImageContext->SizeOfHeaders     = Hdr.Pe32-
>>OptionalHeader.SizeOfHeaders;
>
>-    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>+    } else if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>       //
>       // 1. Check FileHeader.NumberOfRvaAndSizes filed.
>       //
>@@ -605,7 +572,6 @@ PeCoffLoaderGetImageInfo (
>   EFI_IMAGE_SECTION_HEADER              SectionHeader;
>   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
>   UINT32                                NumberOfRvaAndSizes;
>-  UINT16                                Magic;
>   UINT32                                TeStrippedOffset;
>
>   if (ImageContext == NULL) {
>@@ -622,14 +588,12 @@ PeCoffLoaderGetImageInfo (
>     return Status;
>   }
>
>-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>-
>   //
>   // Retrieve the base address of the image
>   //
>   if (!(ImageContext->IsTeImage)) {
>     TeStrippedOffset = 0;
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -678,7 +642,7 @@ PeCoffLoaderGetImageInfo (
>   }
>
>   if (!(ImageContext->IsTeImage)) {
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -952,7 +916,6 @@ PeCoffLoaderRelocateImage (
>   CHAR8                                 *FixupData;
>   PHYSICAL_ADDRESS                      BaseAddress;
>   UINT32                                NumberOfRvaAndSizes;
>-  UINT16                                Magic;
>   UINT32                                TeStrippedOffset;
>
>   ASSERT (ImageContext != NULL);
>@@ -985,9 +948,8 @@ PeCoffLoaderRelocateImage (
>   if (!(ImageContext->IsTeImage)) {
>     Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext-
>>ImageAddress + ImageContext->PeCoffHeaderOffset);
>     TeStrippedOffset = 0;
>-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -1230,7 +1192,6 @@ PeCoffLoaderLoadImage (
>   UINTN                                 Size;
>   UINT32                                TempDebugEntryRva;
>   UINT32                                NumberOfRvaAndSizes;
>-  UINT16                                Magic;
>   EFI_IMAGE_RESOURCE_DIRECTORY          *ResourceDirectory;
>   EFI_IMAGE_RESOURCE_DIRECTORY_ENTRY    *ResourceDirectoryEntry;
>   EFI_IMAGE_RESOURCE_DIRECTORY_STRING   *ResourceDirectoryString;
>@@ -1404,12 +1365,11 @@ PeCoffLoaderLoadImage (
>   //
>   // Get image's entry point
>   //
>-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>   if (!(ImageContext->IsTeImage)) {
>     //
>     // Sizes of AddressOfEntryPoint are different so we need to do this safely
>     //
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -1444,7 +1404,7 @@ PeCoffLoaderLoadImage (
>   // the optional header to verify a desired directory entry is there.
>   //
>   if (!(ImageContext->IsTeImage)) {
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -1565,7 +1525,7 @@ PeCoffLoaderLoadImage (
>   //
>   ImageContext->HiiResourceData = 0;
>   if (!(ImageContext->IsTeImage)) {
>-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+    if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>       //
>       // Use PE32 offset
>       //
>@@ -1721,7 +1681,6 @@ PeCoffLoaderRelocateImageForRuntime (
>   CHAR8                               *FixupData;
>   UINTN                               Adjust;
>   RETURN_STATUS                       Status;
>-  UINT16                              Magic;
>
>   OldBase = (CHAR8 *)((UINTN)ImageBase);
>   NewBase = (CHAR8 *)((UINTN)VirtImageBase); @@ -1750,9 +1709,7 @@
>PeCoffLoaderRelocateImageForRuntime (
>     return ;
>   }
>
>-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
>-
>-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>+  if (Hdr.Pe32->OptionalHeader.Magic ==
>+ EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>     //
>     // Use PE32 offset
>     //
>--
>2.18.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/4] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
  2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
  2018-09-06 13:45 ` [PATCH 2/4] MdePkg/BasePeCoffLib: " Ard Biesheuvel
@ 2018-09-06 13:45 ` Ard Biesheuvel
  2018-09-06 16:47   ` Laszlo Ersek
  2018-09-06 13:45 ` [PATCH 4/4] EdkCompatibilityPkg: " Ard Biesheuvel
  2018-09-06 14:05 ` [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Yao, Jiewen
  4 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 13:45 UTC (permalink / raw)
  To: edk2-devel
  Cc: Ard Biesheuvel, Star Zeng, Jian J Wang, Michael D Kinney,
	Liming Gao, Chao Zhang, Jiewen Yao, Laszlo Ersek, Leif Lindholm

Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c        | 47 ++++----------------
 SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c              | 27 +++--------
 SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c                                  | 27 +++--------
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 25 +++--------
 4 files changed, 25 insertions(+), 101 deletions(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 0f795c0af125..66d96a9396b9 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -295,7 +295,6 @@ HashPeImage (
   )
 {
   BOOLEAN                   Status;
-  UINT16                    Magic;
   EFI_IMAGE_SECTION_HEADER  *Section;
   VOID                      *HashCtx;
   UINTN                     CtxSize;
@@ -367,33 +366,19 @@ HashPeImage (
   // Measuring PE/COFF Image Header;
   // But CheckSum field and SECURITY data directory (certificate) are excluded
   //
-  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic =  mNtHeader.Pe32->OptionalHeader.Magic;
-  }
 
   //
   // 3.  Calculate the distance from the base of the image header to the image checksum address.
   // 4.  Hash the image header from its base to beginning of the image checksum.
   //
   HashBase = mImageBase;
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
     HashSize = (UINTN) (&mNtHeader.Pe32->OptionalHeader.CheckSum) - (UINTN) HashBase;
     NumberOfRvaAndSizes = mNtHeader.Pe32->OptionalHeader.NumberOfRvaAndSizes;
-  } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+  } else if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
     //
     // Use PE32+ offset.
     //
@@ -420,7 +405,7 @@ HashPeImage (
     // 6.  Since there is no Cert Directory in optional header, hash everything
     //     from the end of the checksum to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset.
       //
@@ -444,7 +429,7 @@ HashPeImage (
     //
     // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset.
       //
@@ -469,7 +454,7 @@ HashPeImage (
     // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
     // 9.  Hash everything from the end of the Cert Directory to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -494,7 +479,7 @@ HashPeImage (
   //
   // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
@@ -577,7 +562,7 @@ HashPeImage (
     if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
       CertSize = 0;
     } else {
-      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
         //
         // Use PE32 offset.
         //
@@ -1583,7 +1568,6 @@ DxeImageVerificationHandler (
   )
 {
   EFI_STATUS                           Status;
-  UINT16                               Magic;
   EFI_IMAGE_DOS_HEADER                 *DosHdr;
   EFI_STATUS                           VerifyStatus;
   EFI_SIGNATURE_LIST                   *SignatureList;
@@ -1723,22 +1707,7 @@ DxeImageVerificationHandler (
     goto Done;
   }
 
-  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
-  }
-
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
index c54ab62e2745..4e4a90f9da62 100644
--- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
+++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
@@ -320,7 +320,6 @@ TcgMeasurePeImage (
   EFI_IMAGE_SECTION_HEADER             *SectionHeader;
   UINTN                                Index;
   UINTN                                Pos;
-  UINT16                               Magic;
   UINT32                               EventSize;
   UINT32                               EventNumber;
   EFI_PHYSICAL_ADDRESS                 EventLogLastEntry;
@@ -418,27 +417,13 @@ TcgMeasurePeImage (
   // Measuring PE/COFF Image Header;
   // But CheckSum field and SECURITY data directory (certificate) are excluded
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
 
   //
   // 3.  Calculate the distance from the base of the image header to the image checksum address.
   // 4.  Hash the image header from its base to beginning of the image checksum.
   //
   HashBase = (UINT8 *) (UINTN) ImageAddress;
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
@@ -465,7 +450,7 @@ TcgMeasurePeImage (
     // 6.  Since there is no Cert Directory in optional header, hash everything
     //     from the end of the checksum to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset.
       //
@@ -489,7 +474,7 @@ TcgMeasurePeImage (
     //
     // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -514,7 +499,7 @@ TcgMeasurePeImage (
     // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
     // 9.  Hash everything from the end of the Cert Directory to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -539,7 +524,7 @@ TcgMeasurePeImage (
   //
   // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
@@ -621,7 +606,7 @@ TcgMeasurePeImage (
     if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
       CertSize = 0;
     } else {
-      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+      if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
         //
         // Use PE32 offset.
         //
diff --git a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
index 29da2d70e699..61195e6041f7 100644
--- a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
+++ b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
@@ -116,7 +116,6 @@ MeasurePeImageAndExtend (
   EFI_IMAGE_SECTION_HEADER             *SectionHeader;
   UINTN                                Index;
   UINTN                                Pos;
-  UINT16                               Magic;
   EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
   UINT32                               NumberOfRvaAndSizes;
   UINT32                               CertSize;
@@ -181,27 +180,13 @@ MeasurePeImageAndExtend (
   // Measuring PE/COFF Image Header;
   // But CheckSum field and SECURITY data directory (certificate) are excluded
   //
-  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = Hdr.Pe32->OptionalHeader.Magic;
-  }
 
   //
   // 3.  Calculate the distance from the base of the image header to the image checksum address.
   // 4.  Hash the image header from its base to beginning of the image checksum.
   //
   HashBase = (UINT8 *) (UINTN) ImageAddress;
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
@@ -228,7 +213,7 @@ MeasurePeImageAndExtend (
     // 6.  Since there is no Cert Directory in optional header, hash everything
     //     from the end of the checksum to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset.
       //
@@ -252,7 +237,7 @@ MeasurePeImageAndExtend (
     //
     // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -277,7 +262,7 @@ MeasurePeImageAndExtend (
     // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
     // 9.  Hash everything from the end of the Cert Directory to the end of image header.
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -302,7 +287,7 @@ MeasurePeImageAndExtend (
   //
   // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
@@ -384,7 +369,7 @@ MeasurePeImageAndExtend (
     if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
       CertSize = 0;
     } else {
-      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
         //
         // Use PE32 offset.
         //
diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index 9acaa7b97507..f96325e978a5 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -1831,7 +1831,6 @@ HashPeImage (
   )
 {
   BOOLEAN                   Status;
-  UINT16                    Magic;
   EFI_IMAGE_SECTION_HEADER  *Section;
   VOID                      *HashCtx;
   UINTN                     CtxSize;
@@ -1874,27 +1873,13 @@ HashPeImage (
   // Measuring PE/COFF Image Header;
   // But CheckSum field and SECURITY data directory (certificate) are excluded
   //
-  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    //
-    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
-    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
-    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-    //
-    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  } else {
-    //
-    // Get the magic value from the PE/COFF Optional Header
-    //
-    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
-  }
 
   //
   // 3.  Calculate the distance from the base of the image header to the image checksum address.
   // 4.  Hash the image header from its base to beginning of the image checksum.
   //
   HashBase = mImageBase;
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
@@ -1915,7 +1900,7 @@ HashPeImage (
   // 6.  Get the address of the beginning of the Cert Directory.
   // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
@@ -1937,7 +1922,7 @@ HashPeImage (
   // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
   // 9.  Hash everything from the end of the Cert Directory to the end of image header.
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
@@ -1958,7 +1943,7 @@ HashPeImage (
   //
   // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
   //
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
@@ -2032,7 +2017,7 @@ HashPeImage (
   //
   if (mImageSize > SumOfBytesHashed) {
     HashBase = mImageBase + SumOfBytesHashed;
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset.
       //
-- 
2.18.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 3/4] SecurityPkg: " Ard Biesheuvel
@ 2018-09-06 16:47   ` Laszlo Ersek
  2018-09-06 17:25     ` Ard Biesheuvel
  0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 16:47 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel
  Cc: Liming Gao, Jiewen Yao, Star Zeng, Michael D Kinney, Chao Zhang

On 09/06/18 15:45, Ard Biesheuvel wrote:
> Now that Itanium support has been dropped, we can remove the various
> occurrences of the ELILO on Itanium PE/COFF header workaround.
> 
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c        | 47 ++++----------------
>  SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c              | 27 +++--------
>  SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c                                  | 27 +++--------
>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 25 +++--------
>  4 files changed, 25 insertions(+), 101 deletions(-)
> 
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index 0f795c0af125..66d96a9396b9 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -295,7 +295,6 @@ HashPeImage (
>    )
>  {
>    BOOLEAN                   Status;
> -  UINT16                    Magic;
>    EFI_IMAGE_SECTION_HEADER  *Section;
>    VOID                      *HashCtx;
>    UINTN                     CtxSize;
> @@ -367,33 +366,19 @@ HashPeImage (
>    // Measuring PE/COFF Image Header;
>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>    //
> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic =  mNtHeader.Pe32->OptionalHeader.Magic;
> -  }
>  
>    //
>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>    // 4.  Hash the image header from its base to beginning of the image checksum.
>    //
>    HashBase = mImageBase;
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
>      HashSize = (UINTN) (&mNtHeader.Pe32->OptionalHeader.CheckSum) - (UINTN) HashBase;
>      NumberOfRvaAndSizes = mNtHeader.Pe32->OptionalHeader.NumberOfRvaAndSizes;
> -  } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
> +  } else if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>      //
>      // Use PE32+ offset.
>      //
> @@ -420,7 +405,7 @@ HashPeImage (
>      // 6.  Since there is no Cert Directory in optional header, hash everything
>      //     from the end of the checksum to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset.
>        //
> @@ -444,7 +429,7 @@ HashPeImage (
>      //
>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset.
>        //
> @@ -469,7 +454,7 @@ HashPeImage (
>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -494,7 +479,7 @@ HashPeImage (
>    //
>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
> @@ -577,7 +562,7 @@ HashPeImage (
>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>        CertSize = 0;
>      } else {
> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>          //
>          // Use PE32 offset.
>          //
> @@ -1583,7 +1568,6 @@ DxeImageVerificationHandler (
>    )
>  {
>    EFI_STATUS                           Status;
> -  UINT16                               Magic;
>    EFI_IMAGE_DOS_HEADER                 *DosHdr;
>    EFI_STATUS                           VerifyStatus;
>    EFI_SIGNATURE_LIST                   *SignatureList;
> @@ -1723,22 +1707,7 @@ DxeImageVerificationHandler (
>      goto Done;
>    }
>  
> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
> -  }
> -
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
> diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> index c54ab62e2745..4e4a90f9da62 100644
> --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
> @@ -320,7 +320,6 @@ TcgMeasurePeImage (
>    EFI_IMAGE_SECTION_HEADER             *SectionHeader;
>    UINTN                                Index;
>    UINTN                                Pos;
> -  UINT16                               Magic;
>    UINT32                               EventSize;
>    UINT32                               EventNumber;
>    EFI_PHYSICAL_ADDRESS                 EventLogLastEntry;
> @@ -418,27 +417,13 @@ TcgMeasurePeImage (
>    // Measuring PE/COFF Image Header;
>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>    //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
> -  }
>  
>    //
>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>    // 4.  Hash the image header from its base to beginning of the image checksum.
>    //
>    HashBase = (UINT8 *) (UINTN) ImageAddress;
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset
>      //
> @@ -465,7 +450,7 @@ TcgMeasurePeImage (
>      // 6.  Since there is no Cert Directory in optional header, hash everything
>      //     from the end of the checksum to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset.
>        //
> @@ -489,7 +474,7 @@ TcgMeasurePeImage (
>      //
>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -514,7 +499,7 @@ TcgMeasurePeImage (
>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -539,7 +524,7 @@ TcgMeasurePeImage (
>    //
>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset
>      //
> @@ -621,7 +606,7 @@ TcgMeasurePeImage (
>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>        CertSize = 0;
>      } else {
> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +      if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>          //
>          // Use PE32 offset.
>          //
> diff --git a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
> index 29da2d70e699..61195e6041f7 100644
> --- a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
> +++ b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
> @@ -116,7 +116,6 @@ MeasurePeImageAndExtend (
>    EFI_IMAGE_SECTION_HEADER             *SectionHeader;
>    UINTN                                Index;
>    UINTN                                Pos;
> -  UINT16                               Magic;
>    EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
>    UINT32                               NumberOfRvaAndSizes;
>    UINT32                               CertSize;
> @@ -181,27 +180,13 @@ MeasurePeImageAndExtend (
>    // Measuring PE/COFF Image Header;
>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>    //
> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
> -  }
>  
>    //
>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>    // 4.  Hash the image header from its base to beginning of the image checksum.
>    //
>    HashBase = (UINT8 *) (UINTN) ImageAddress;
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {

I think this is a bug. Here "Magic" used to be set based on "Hdr.Pe32->OptionalHeader.Magic", not based on "mNtHeader.Pe32->OptionalHeader.Magic".

And, I'm not convinced (mNtHeader.Pe32 == Hdr.Pe32). In earlier parts of this function, "Hdr.Pe32" is set as follows:

  Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *) (UINTN) ImageAddress + PeCoffHeaderOffset);

where "ImageAddress" is an input parameter to the function.

I wonder why this code compiles; no "mNtHeader" declaration should be in scope here.

$ git grep -w -l mNtHeader

SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c

More of the same below:

>      //
>      // Use PE32 offset
>      //
> @@ -228,7 +213,7 @@ MeasurePeImageAndExtend (
>      // 6.  Since there is no Cert Directory in optional header, hash everything
>      //     from the end of the checksum to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset.
>        //
> @@ -252,7 +237,7 @@ MeasurePeImageAndExtend (
>      //
>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -277,7 +262,7 @@ MeasurePeImageAndExtend (
>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>      //
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset
>        //
> @@ -302,7 +287,7 @@ MeasurePeImageAndExtend (
>    //
>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset
>      //
> @@ -384,7 +369,7 @@ MeasurePeImageAndExtend (
>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>        CertSize = 0;
>      } else {
> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>          //
>          // Use PE32 offset.
>          //

Until here.

For build-testing, I suggest

  build -p SecurityPkg/SecurityPkg.dsc ...

It will build all SecurityPkg modules, without putting them in a flash device (FD).

The rest looks good to me.

Thanks,
Laszlo

> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
> index 9acaa7b97507..f96325e978a5 100644
> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
> @@ -1831,7 +1831,6 @@ HashPeImage (
>    )
>  {
>    BOOLEAN                   Status;
> -  UINT16                    Magic;
>    EFI_IMAGE_SECTION_HEADER  *Section;
>    VOID                      *HashCtx;
>    UINTN                     CtxSize;
> @@ -1874,27 +1873,13 @@ HashPeImage (
>    // Measuring PE/COFF Image Header;
>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>    //
> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -    //
> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> -    //
> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
> -  } else {
> -    //
> -    // Get the magic value from the PE/COFF Optional Header
> -    //
> -    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
> -  }
>  
>    //
>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>    // 4.  Hash the image header from its base to beginning of the image checksum.
>    //
>    HashBase = mImageBase;
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
> @@ -1915,7 +1900,7 @@ HashPeImage (
>    // 6.  Get the address of the beginning of the Cert Directory.
>    // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
> @@ -1937,7 +1922,7 @@ HashPeImage (
>    // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>    // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset
>      //
> @@ -1958,7 +1943,7 @@ HashPeImage (
>    //
>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
>    //
> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>      //
>      // Use PE32 offset.
>      //
> @@ -2032,7 +2017,7 @@ HashPeImage (
>    //
>    if (mImageSize > SumOfBytesHashed) {
>      HashBase = mImageBase + SumOfBytesHashed;
> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>        //
>        // Use PE32 offset.
>        //
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 16:47   ` Laszlo Ersek
@ 2018-09-06 17:25     ` Ard Biesheuvel
  2018-09-06 18:22       ` Laszlo Ersek
  0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 17:25 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: edk2-devel@lists.01.org, Liming Gao, Jiewen Yao, Star Zeng,
	Michael D Kinney, Chao Zhang

On 6 September 2018 at 18:47, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/06/18 15:45, Ard Biesheuvel wrote:
>> Now that Itanium support has been dropped, we can remove the various
>> occurrences of the ELILO on Itanium PE/COFF header workaround.
>>
>> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c        | 47 ++++----------------
>>  SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c              | 27 +++--------
>>  SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c                                  | 27 +++--------
>>  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 25 +++--------
>>  4 files changed, 25 insertions(+), 101 deletions(-)
>>
>> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> index 0f795c0af125..66d96a9396b9 100644
>> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
>> @@ -295,7 +295,6 @@ HashPeImage (
>>    )
>>  {
>>    BOOLEAN                   Status;
>> -  UINT16                    Magic;
>>    EFI_IMAGE_SECTION_HEADER  *Section;
>>    VOID                      *HashCtx;
>>    UINTN                     CtxSize;
>> @@ -367,33 +366,19 @@ HashPeImage (
>>    // Measuring PE/COFF Image Header;
>>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>>    //
>> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> -    //
>> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
>> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
>> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>> -    //
>> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>> -  } else {
>> -    //
>> -    // Get the magic value from the PE/COFF Optional Header
>> -    //
>> -    Magic =  mNtHeader.Pe32->OptionalHeader.Magic;
>> -  }
>>
>>    //
>>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>>    // 4.  Hash the image header from its base to beginning of the image checksum.
>>    //
>>    HashBase = mImageBase;
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>>      HashSize = (UINTN) (&mNtHeader.Pe32->OptionalHeader.CheckSum) - (UINTN) HashBase;
>>      NumberOfRvaAndSizes = mNtHeader.Pe32->OptionalHeader.NumberOfRvaAndSizes;
>> -  } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>> +  } else if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>>      //
>>      // Use PE32+ offset.
>>      //
>> @@ -420,7 +405,7 @@ HashPeImage (
>>      // 6.  Since there is no Cert Directory in optional header, hash everything
>>      //     from the end of the checksum to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset.
>>        //
>> @@ -444,7 +429,7 @@ HashPeImage (
>>      //
>>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset.
>>        //
>> @@ -469,7 +454,7 @@ HashPeImage (
>>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset
>>        //
>> @@ -494,7 +479,7 @@ HashPeImage (
>>    //
>>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>> @@ -577,7 +562,7 @@ HashPeImage (
>>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>>        CertSize = 0;
>>      } else {
>> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>          //
>>          // Use PE32 offset.
>>          //
>> @@ -1583,7 +1568,6 @@ DxeImageVerificationHandler (
>>    )
>>  {
>>    EFI_STATUS                           Status;
>> -  UINT16                               Magic;
>>    EFI_IMAGE_DOS_HEADER                 *DosHdr;
>>    EFI_STATUS                           VerifyStatus;
>>    EFI_SIGNATURE_LIST                   *SignatureList;
>> @@ -1723,22 +1707,7 @@ DxeImageVerificationHandler (
>>      goto Done;
>>    }
>>
>> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> -    //
>> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
>> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
>> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>> -    //
>> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>> -  } else {
>> -    //
>> -    // Get the magic value from the PE/COFF Optional Header
>> -    //
>> -    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
>> -  }
>> -
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>> diff --git a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
>> index c54ab62e2745..4e4a90f9da62 100644
>> --- a/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
>> +++ b/SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c
>> @@ -320,7 +320,6 @@ TcgMeasurePeImage (
>>    EFI_IMAGE_SECTION_HEADER             *SectionHeader;
>>    UINTN                                Index;
>>    UINTN                                Pos;
>> -  UINT16                               Magic;
>>    UINT32                               EventSize;
>>    UINT32                               EventNumber;
>>    EFI_PHYSICAL_ADDRESS                 EventLogLastEntry;
>> @@ -418,27 +417,13 @@ TcgMeasurePeImage (
>>    // Measuring PE/COFF Image Header;
>>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>>    //
>> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> -    //
>> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
>> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
>> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>> -    //
>> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>> -  } else {
>> -    //
>> -    // Get the magic value from the PE/COFF Optional Header
>> -    //
>> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
>> -  }
>>
>>    //
>>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>>    // 4.  Hash the image header from its base to beginning of the image checksum.
>>    //
>>    HashBase = (UINT8 *) (UINTN) ImageAddress;
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset
>>      //
>> @@ -465,7 +450,7 @@ TcgMeasurePeImage (
>>      // 6.  Since there is no Cert Directory in optional header, hash everything
>>      //     from the end of the checksum to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset.
>>        //
>> @@ -489,7 +474,7 @@ TcgMeasurePeImage (
>>      //
>>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset
>>        //
>> @@ -514,7 +499,7 @@ TcgMeasurePeImage (
>>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset
>>        //
>> @@ -539,7 +524,7 @@ TcgMeasurePeImage (
>>    //
>>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset
>>      //
>> @@ -621,7 +606,7 @@ TcgMeasurePeImage (
>>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>>        CertSize = 0;
>>      } else {
>> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +      if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>          //
>>          // Use PE32 offset.
>>          //
>> diff --git a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
>> index 29da2d70e699..61195e6041f7 100644
>> --- a/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
>> +++ b/SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c
>> @@ -116,7 +116,6 @@ MeasurePeImageAndExtend (
>>    EFI_IMAGE_SECTION_HEADER             *SectionHeader;
>>    UINTN                                Index;
>>    UINTN                                Pos;
>> -  UINT16                               Magic;
>>    EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
>>    UINT32                               NumberOfRvaAndSizes;
>>    UINT32                               CertSize;
>> @@ -181,27 +180,13 @@ MeasurePeImageAndExtend (
>>    // Measuring PE/COFF Image Header;
>>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>>    //
>> -  if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> -    //
>> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
>> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
>> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>> -    //
>> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>> -  } else {
>> -    //
>> -    // Get the magic value from the PE/COFF Optional Header
>> -    //
>> -    Magic = Hdr.Pe32->OptionalHeader.Magic;
>> -  }
>>
>>    //
>>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>>    // 4.  Hash the image header from its base to beginning of the image checksum.
>>    //
>>    HashBase = (UINT8 *) (UINTN) ImageAddress;
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>
> I think this is a bug. Here "Magic" used to be set based on "Hdr.Pe32->OptionalHeader.Magic", not based on "mNtHeader.Pe32->OptionalHeader.Magic".
>
> And, I'm not convinced (mNtHeader.Pe32 == Hdr.Pe32). In earlier parts of this function, "Hdr.Pe32" is set as follows:
>
>   Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *) (UINTN) ImageAddress + PeCoffHeaderOffset);
>
> where "ImageAddress" is an input parameter to the function.
>
> I wonder why this code compiles; no "mNtHeader" declaration should be in scope here.
>
> $ git grep -w -l mNtHeader
>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>
> More of the same below:
>
>>      //
>>      // Use PE32 offset
>>      //
>> @@ -228,7 +213,7 @@ MeasurePeImageAndExtend (
>>      // 6.  Since there is no Cert Directory in optional header, hash everything
>>      //     from the end of the checksum to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset.
>>        //
>> @@ -252,7 +237,7 @@ MeasurePeImageAndExtend (
>>      //
>>      // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset
>>        //
>> @@ -277,7 +262,7 @@ MeasurePeImageAndExtend (
>>      // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>>      // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>>      //
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset
>>        //
>> @@ -302,7 +287,7 @@ MeasurePeImageAndExtend (
>>    //
>>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset
>>      //
>> @@ -384,7 +369,7 @@ MeasurePeImageAndExtend (
>>      if (NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
>>        CertSize = 0;
>>      } else {
>> -      if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +      if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>          //
>>          // Use PE32 offset.
>>          //
>
> Until here.
>

Ah yes. Thanks for spotting that.

> For build-testing, I suggest
>
>   build -p SecurityPkg/SecurityPkg.dsc ...
>
> It will build all SecurityPkg modules, without putting them in a flash device (FD).
>

That is what I used to perform the build test, which completed without
error. But the logic is indeed incorrect.

> The rest looks good to me.
>

Thanks

>> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>> index 9acaa7b97507..f96325e978a5 100644
>> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
>> @@ -1831,7 +1831,6 @@ HashPeImage (
>>    )
>>  {
>>    BOOLEAN                   Status;
>> -  UINT16                    Magic;
>>    EFI_IMAGE_SECTION_HEADER  *Section;
>>    VOID                      *HashCtx;
>>    UINTN                     CtxSize;
>> @@ -1874,27 +1873,13 @@ HashPeImage (
>>    // Measuring PE/COFF Image Header;
>>    // But CheckSum field and SECURITY data directory (certificate) are excluded
>>    //
>> -  if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> -    //
>> -    // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
>> -    //       in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
>> -    //       Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
>> -    //       then override the magic value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
>> -    //
>> -    Magic = EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
>> -  } else {
>> -    //
>> -    // Get the magic value from the PE/COFF Optional Header
>> -    //
>> -    Magic = mNtHeader.Pe32->OptionalHeader.Magic;
>> -  }
>>
>>    //
>>    // 3.  Calculate the distance from the base of the image header to the image checksum address.
>>    // 4.  Hash the image header from its base to beginning of the image checksum.
>>    //
>>    HashBase = mImageBase;
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>> @@ -1915,7 +1900,7 @@ HashPeImage (
>>    // 6.  Get the address of the beginning of the Cert Directory.
>>    // 7.  Hash everything from the end of the checksum to the start of the Cert Directory.
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>> @@ -1937,7 +1922,7 @@ HashPeImage (
>>    // 8.  Skip over the Cert Directory. (It is sizeof(IMAGE_DATA_DIRECTORY) bytes.)
>>    // 9.  Hash everything from the end of the Cert Directory to the end of image header.
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset
>>      //
>> @@ -1958,7 +1943,7 @@ HashPeImage (
>>    //
>>    // 10. Set the SUM_OF_BYTES_HASHED to the size of the header.
>>    //
>> -  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +  if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>      //
>>      // Use PE32 offset.
>>      //
>> @@ -2032,7 +2017,7 @@ HashPeImage (
>>    //
>>    if (mImageSize > SumOfBytesHashed) {
>>      HashBase = mImageBase + SumOfBytesHashed;
>> -    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> +    if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>        //
>>        // Use PE32 offset.
>>        //
>>
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/4] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 17:25     ` Ard Biesheuvel
@ 2018-09-06 18:22       ` Laszlo Ersek
  0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 18:22 UTC (permalink / raw)
  To: Ard Biesheuvel, Jiewen Yao
  Cc: edk2-devel@lists.01.org, Liming Gao, Star Zeng, Michael D Kinney,
	Chao Zhang

On 09/06/18 19:25, Ard Biesheuvel wrote:
> On 6 September 2018 at 18:47, Laszlo Ersek <lersek@redhat.com> wrote:

>> For build-testing, I suggest
>>
>>   build -p SecurityPkg/SecurityPkg.dsc ...
>>
>> It will build all SecurityPkg modules, without putting them in a flash device (FD).
>>
> 
> That is what I used to perform the build test, which completed without
> error. [...]

That's spooky! In "SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c", no
declaration of "mNtHeader" should be visible. Hm...

Arrgh, if you check "SecurityPkg/SecurityPkg.dsc", the
"SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf" file is listed *only* under

[Components.IA32, Components.X64]

Thus, assuming you built natively on AARCH64, this module was not built. :/

Jiewen, is there any particular reason for not building Tcg2Dxe on
AARCH64? Is it just "historical lack of AARCH64 test environment
providing TPM2 hardware", or something more "architectural"?

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2018-09-06 13:45 ` [PATCH 3/4] SecurityPkg: " Ard Biesheuvel
@ 2018-09-06 13:45 ` Ard Biesheuvel
  2018-09-06 16:53   ` Laszlo Ersek
  2018-09-06 14:05 ` [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Yao, Jiewen
  4 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 13:45 UTC (permalink / raw)
  To: edk2-devel
  Cc: Ard Biesheuvel, Star Zeng, Jian J Wang, Michael D Kinney,
	Liming Gao, Chao Zhang, Jiewen Yao, Laszlo Ersek, Leif Lindholm

Now that Itanium support has been dropped, we can remove the various
occurrences of the ELILO on Itanium PE/COFF header workaround.

Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------
 1 file changed, 8 insertions(+), 52 deletions(-)

diff --git a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
index f99c23e5ee4c..28d39b342afd 100644
--- a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
+++ b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c
@@ -22,36 +22,6 @@ Abstract:
 
 #include "BasePeCoffLibInternals.h"
 
-/**
-  Retrieves the magic value from the PE/COFF header.
-
-  @param  Hdr             The buffer in which to return the PE32, PE32+, or TE header.
-
-  @return EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC - Image is PE32
-  @return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC - Image is PE32+
-
-**/
-UINT16
-PeCoffLoaderGetPeHeaderMagicValue (
-  IN  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr
-  )
-{
-  //
-  // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value 
-  //       in the PE/COFF Header.  If the MachineType is Itanium(IA64) and the 
-  //       Magic value in the OptionalHeader is  EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
-  //       then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
-  //
-  if (Hdr.Pe32->FileHeader.Machine == EFI_IMAGE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-    return EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC;
-  }
-  //
-  // Return the magic value from the PC/COFF Optional Header
-  //
-  return Hdr.Pe32->OptionalHeader.Magic;
-}
-
-
 /**
   Retrieves the PE or TE Header from a PE/COFF or TE image.
 
@@ -71,7 +41,6 @@ GluePeCoffLoaderGetPeHeader (
   RETURN_STATUS         Status;
   EFI_IMAGE_DOS_HEADER  DosHdr;
   UINTN                 Size;
-  UINT16                Magic;
 
   //
   // Read the DOS image header to check for it's existance
@@ -130,9 +99,7 @@ GluePeCoffLoaderGetPeHeader (
     ImageContext->IsTeImage = FALSE;
     ImageContext->Machine = Hdr.Pe32->FileHeader.Machine;
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -141,7 +108,7 @@ GluePeCoffLoaderGetPeHeader (
       ImageContext->SectionAlignment  = Hdr.Pe32->OptionalHeader.SectionAlignment;
       ImageContext->SizeOfHeaders     = Hdr.Pe32->OptionalHeader.SizeOfHeaders;
 
-    } else if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
+    } else if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
       //
       // Use PE32+ offset
       //
@@ -209,7 +176,6 @@ GluePeCoffLoaderGetImageInfo (
   EFI_IMAGE_SECTION_HEADER              SectionHeader;
   EFI_IMAGE_DEBUG_DIRECTORY_ENTRY       DebugEntry;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   if (NULL == ImageContext) {
     return RETURN_INVALID_PARAMETER;
@@ -225,13 +191,11 @@ GluePeCoffLoaderGetImageInfo (
     return Status;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
   //
   // Retrieve the base address of the image
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -276,7 +240,7 @@ GluePeCoffLoaderGetImageInfo (
   }
 
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -520,7 +484,6 @@ GluePeCoffLoaderRelocateImage (
   CHAR8                                 *FixupData;
   PHYSICAL_ADDRESS                      BaseAddress;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   ASSERT (ImageContext != NULL);
 
@@ -552,9 +515,7 @@ GluePeCoffLoaderRelocateImage (
   if (!(ImageContext->IsTeImage)) {
     Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN)ImageContext->ImageAddress + ImageContext->PeCoffHeaderOffset);
 
-    Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -777,7 +738,6 @@ GluePeCoffLoaderLoadImage (
   UINTN                                 Size;
   UINT32                                TempDebugEntryRva;
   UINT32                                NumberOfRvaAndSizes;
-  UINT16                                Magic;
 
   ASSERT (ImageContext != NULL);
 
@@ -965,12 +925,11 @@ GluePeCoffLoaderLoadImage (
   //
   // Get image's entry point
   //
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
   if (!(ImageContext->IsTeImage)) {
     //
     // Sizes of AddressOfEntryPoint are different so we need to do this safely
     //
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1004,7 +963,7 @@ GluePeCoffLoaderLoadImage (
   // the optional header to verify a desired directory entry is there.
   //
   if (!(ImageContext->IsTeImage)) {
-    if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+    if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
       //
       // Use PE32 offset
       //
@@ -1172,7 +1131,6 @@ PeCoffLoaderRelocateImageForRuntime (
   CHAR8                               *FixupData;
   UINTN                               Adjust;
   RETURN_STATUS                       Status;
-  UINT16                              Magic;
 
   OldBase = (CHAR8 *)((UINTN)ImageBase);
   NewBase = (CHAR8 *)((UINTN)VirtImageBase);
@@ -1201,9 +1159,7 @@ PeCoffLoaderRelocateImageForRuntime (
     return ;
   }
 
-  Magic = PeCoffLoaderGetPeHeaderMagicValue (Hdr);
-
-  if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+  if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset
     //
-- 
2.18.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 13:45 ` [PATCH 4/4] EdkCompatibilityPkg: " Ard Biesheuvel
@ 2018-09-06 16:53   ` Laszlo Ersek
  2018-09-06 17:27     ` Ard Biesheuvel
  0 siblings, 1 reply; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 16:53 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel
  Cc: Liming Gao, Jiewen Yao, Star Zeng, Michael D Kinney, Chao Zhang

On 09/06/18 15:45, Ard Biesheuvel wrote:
> Now that Itanium support has been dropped, we can remove the various
> occurrences of the ELILO on Itanium PE/COFF header workaround.
>
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------
>  1 file changed, 8 insertions(+), 52 deletions(-)

Should we care about EdkCompatibilityPkg at all? Because:

* IPF removal seems not to have occurred to EdkCompatibilityPkg:

  $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'
  [bunch of hits]

* In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:

> [...] there is a big difference between IPF drivers that are never
> referenced by modern platforms, and workarounds in generic code that
> are present in every modern build for every platform, and are only
> intended for a specific build of ELILO.

> The former is essentially dead code. The latter gets executed many
> times on every boot of every modern UEFI platform in existence.

Under that distinction, I would classify EdkCompatibilityPkg as the
first category, i.e., essentially dead code.

(I'm pointing this out in the hope that it'll save me the review of this
patch! :) )

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 16:53   ` Laszlo Ersek
@ 2018-09-06 17:27     ` Ard Biesheuvel
  2018-09-06 18:01       ` Carsey, Jaben
  0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2018-09-06 17:27 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: edk2-devel@lists.01.org, Liming Gao, Jiewen Yao, Star Zeng,
	Michael D Kinney, Chao Zhang

On 6 September 2018 at 18:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/06/18 15:45, Ard Biesheuvel wrote:
>> Now that Itanium support has been dropped, we can remove the various
>> occurrences of the ELILO on Itanium PE/COFF header workaround.
>>
>> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------
>>  1 file changed, 8 insertions(+), 52 deletions(-)
>
> Should we care about EdkCompatibilityPkg at all? Because:
>
> * IPF removal seems not to have occurred to EdkCompatibilityPkg:
>
>   $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'
>   [bunch of hits]
>
> * In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:
>
>> [...] there is a big difference between IPF drivers that are never
>> referenced by modern platforms, and workarounds in generic code that
>> are present in every modern build for every platform, and are only
>> intended for a specific build of ELILO.
>
>> The former is essentially dead code. The latter gets executed many
>> times on every boot of every modern UEFI platform in existence.
>
> Under that distinction, I would classify EdkCompatibilityPkg as the
> first category, i.e., essentially dead code.
>

OK, fair enough. I don't care about EdkCompatibilityPkg at all, I just
wanted to be thorough, but if others don't care either, I'll drop this
from v2.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 17:27     ` Ard Biesheuvel
@ 2018-09-06 18:01       ` Carsey, Jaben
  2018-09-06 18:15         ` Laszlo Ersek
  0 siblings, 1 reply; 18+ messages in thread
From: Carsey, Jaben @ 2018-09-06 18:01 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Laszlo Ersek, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen,
	Zeng, Star, Kinney, Michael D, Zhang, Chao B

We have a BZ to remove the compatibility package... I’d call updating it redundant and not worth your time.

Jaben

> On Sep 6, 2018, at 10:27 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
>> On 6 September 2018 at 18:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 09/06/18 15:45, Ard Biesheuvel wrote:
>>> Now that Itanium support has been dropped, we can remove the various
>>> occurrences of the ELILO on Itanium PE/COFF header workaround.
>>> 
>>> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>> EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePeCoffLib/BasePeCoff.c | 60 +++-----------------
>>> 1 file changed, 8 insertions(+), 52 deletions(-)
>> 
>> Should we care about EdkCompatibilityPkg at all? Because:
>> 
>> * IPF removal seems not to have occurred to EdkCompatibilityPkg:
>> 
>>  $ git grep -w IPF -- 'EdkCompatibilityPkg/*inf'
>>  [bunch of hits]
>> 
>> * In <https://bugzilla.tianocore.org/show_bug.cgi?id=816#c7>, you wrote:
>> 
>>> [...] there is a big difference between IPF drivers that are never
>>> referenced by modern platforms, and workarounds in generic code that
>>> are present in every modern build for every platform, and are only
>>> intended for a specific build of ELILO.
>> 
>>> The former is essentially dead code. The latter gets executed many
>>> times on every boot of every modern UEFI platform in existence.
>> 
>> Under that distinction, I would classify EdkCompatibilityPkg as the
>> first category, i.e., essentially dead code.
>> 
> 
> OK, fair enough. I don't care about EdkCompatibilityPkg at all, I just
> wanted to be thorough, but if others don't care either, I'll drop this
> from v2.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/4] EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
  2018-09-06 18:01       ` Carsey, Jaben
@ 2018-09-06 18:15         ` Laszlo Ersek
  0 siblings, 0 replies; 18+ messages in thread
From: Laszlo Ersek @ 2018-09-06 18:15 UTC (permalink / raw)
  To: Carsey, Jaben, Ard Biesheuvel
  Cc: edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Zeng, Star,
	Kinney, Michael D, Zhang, Chao B

On 09/06/18 20:01, Carsey, Jaben wrote:
> We have a BZ to remove the compatibility package... I’d call updating it redundant and not worth your time.

Ah, very good point!

https://bugzilla.tianocore.org/show_bug.cgi?id=1103

Laszlo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack
  2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2018-09-06 13:45 ` [PATCH 4/4] EdkCompatibilityPkg: " Ard Biesheuvel
@ 2018-09-06 14:05 ` Yao, Jiewen
  4 siblings, 0 replies; 18+ messages in thread
From: Yao, Jiewen @ 2018-09-06 14:05 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel@lists.01.org
  Cc: Zeng, Star, Wang, Jian J, Kinney, Michael D, Gao, Liming,
	Zhang, Chao B, Laszlo Ersek, Leif Lindholm

Thanks for the clean up.

Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, September 6, 2018 9:45 PM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Zeng, Star
> <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zhang,
> Chao B <chao.b.zhang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header
> hack
> 
> Now that Itanium support has been dropped from EDK2, we can remove all 11
> (!)
> occurrences of the ELILO PE/COFF loader hack from the code base.
> 
> Note that SecurityPkg appears to have four mostly identical implementations
> of the PE/COFF measuring routine, so this may be another area for cleanup
> later.
> 
> Link: https://bugzilla.tianocore.org/show_bug.cgi?id=816
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Ard Biesheuvel (4):
>   MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF
>   MdePkg/BasePeCoffLib: remove PE/COFF header workaround for ELILO on
>     IPF
>   SecurityPkg: remove PE/COFF header workaround for ELILO on IPF
>   EdkCompatibilityPkg: remove PE/COFF header workaround for ELILO on IPF
> 
>  .../Library/BasePeCoffLib/BasePeCoff.c        | 60 +++---------------
>  .../Core/Dxe/Mem/MemoryProfileRecord.c        | 31 +---------
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 17 +-----
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c  | 17 +-----
>  .../Core/PiSmmCore/MemoryAttributesTable.c    | 17 +-----
>  .../Core/PiSmmCore/SmramProfileRecord.c       | 31 +---------
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c     | 61 +++----------------
>  .../DxeImageVerificationLib.c                 | 47 +++-----------
>  .../DxeTpmMeasureBootLib.c                    | 27 ++------
>  SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c   | 27 ++------
>  .../SecureBootConfigImpl.c                    | 25 ++------
>  11 files changed, 47 insertions(+), 313 deletions(-)
> 
> --
> 2.18.0



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-09-07  5:36 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-06 13:45 [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Ard Biesheuvel
2018-09-06 13:45 ` [PATCH 1/4] MdeModulePkg: remove PE/COFF header workaround for ELILO on IPF Ard Biesheuvel
2018-09-06 16:29   ` Laszlo Ersek
2018-09-07  5:15   ` Zeng, Star
2018-09-06 13:45 ` [PATCH 2/4] MdePkg/BasePeCoffLib: " Ard Biesheuvel
2018-09-06 16:31   ` Laszlo Ersek
2018-09-07  5:15   ` Zeng, Star
2018-09-07  5:36     ` Gao, Liming
2018-09-06 13:45 ` [PATCH 3/4] SecurityPkg: " Ard Biesheuvel
2018-09-06 16:47   ` Laszlo Ersek
2018-09-06 17:25     ` Ard Biesheuvel
2018-09-06 18:22       ` Laszlo Ersek
2018-09-06 13:45 ` [PATCH 4/4] EdkCompatibilityPkg: " Ard Biesheuvel
2018-09-06 16:53   ` Laszlo Ersek
2018-09-06 17:27     ` Ard Biesheuvel
2018-09-06 18:01       ` Carsey, Jaben
2018-09-06 18:15         ` Laszlo Ersek
2018-09-06 14:05 ` [PATCH 0/4] remove all 11 occurrences of ELILO on IPF PE/COFF header hack Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox