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