[edk2,3/4] SecurityPkg: remove PE/COFF header workaround for ELILO on IPF

Message ID 20180906134523.2036-4-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series
  • remove all 11 occurrences of ELILO on IPF PE/COFF header hack
Related show

Commit Message

Ard Biesheuvel Sept. 6, 2018, 1:45 p.m.
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(-)

-- 
2.18.0

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

Comments

Laszlo Ersek Sept. 6, 2018, 4:47 p.m. | #1
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.

>        //

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 6, 2018, 5:25 p.m. | #2
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.

>>        //

>>

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Sept. 6, 2018, 6:22 p.m. | #3
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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.
       //