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 <[email protected]>
> ---
> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
