[edk2] [PATCH] MdePkg/BasePeCoffLib: SizeOfImage must be multiple of SectionAlignment

[Matt Fleming]

From: Matt Fleming <***@intel.com>

The PE/COFF specification states that the SizeOfImage field must be a
multiple of the SectionAlignment field. Add checks to verify this when
loading an image in PeCoffLoaderGetPeHeader().

This issue was reported by Linn because he discovered that the Linux
kernel's EFI boot stub violates this alignment requirement, and his
firmware refused to load his kernel image.

Reported-by: Linn Crosetto <***@hp.com>
Cc: Michael Brown <***@fensystems.co.uk>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Matt Fleming <***@intel.com>
---
MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 33cad23..f7b740c 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -275,6 +275,16 @@ PeCoffLoaderGetPeHeader (
}

//
+ // 4.1 Check that the SizeOfImage field is a multiple of
+ // SectionAlignment, since this is required by the PE/COFF
+ // specification.
+ //
+ if (Hdr.Pe32->OptionalHeader.SizeOfImage % Hdr.Pe32->OptionalHeader.SectionAlignment) {
+ ImageContext->ImageError = IMAGE_ERROR_UNSUPPORTED;
+ return RETURN_UNSUPPORTED;
+ }
+
+ //
// 4.2 Read last byte of Hdr.Pe32.OptionalHeader.SizeOfHeaders from the file.
//
Size = 1;
@@ -389,6 +399,16 @@ PeCoffLoaderGetPeHeader (
}

//
+ // 4.1 Check that the SizeOfImage field is a multiple of
+ // SectionAlignment, since this is required by the PE/COFF
+ // specification.
+ //
+ if (Hdr.Pe32Plus->OptionalHeader.SizeOfImage % Hdr.Pe32Plus->OptionalHeader.SectionAlignment) {
+ ImageContext->ImageError = IMAGE_ERROR_UNSUPPORTED;
+ return RETURN_UNSUPPORTED;
+ }
+
+ //
// 4.2 Read last byte of Hdr.Pe32Plus.OptionalHeader.SizeOfHeaders from the file.
//
Size = 1;

[Gao, Liming]

Fleming:
Thanks for catching this issue. My minor comment is to change the below code to compare with zero.

if (Hdr.Pe32->OptionalHeader.SizeOfImage % Hdr.Pe32->OptionalHeader.SectionAlignment) ==>
if ((Hdr.Pe32->OptionalHeader.SizeOfImage % Hdr.Pe32->OptionalHeader.SectionAlignment) != 0)

Thanks
Liming
-----Original Message-----
From: Matt Fleming [mailto:***@codeblueprint.co.uk]
Sent: Friday, June 19, 2015 6:06 AM
To: edk2-***@lists.sourceforge.net
Cc: Fleming, Matt; Linn Crosetto; Michael Brown
Subject: [edk2] [PATCH] MdePkg/BasePeCoffLib: SizeOfImage must be multiple of SectionAlignment

From: Matt Fleming <***@intel.com>

The PE/COFF specification states that the SizeOfImage field must be a multiple of the SectionAlignment field. Add checks to verify this when loading an image in PeCoffLoaderGetPeHeader().

This issue was reported by Linn because he discovered that the Linux kernel's EFI boot stub violates this alignment requirement, and his firmware refused to load his kernel image.

Reported-by: Linn Crosetto <***@hp.com>
Cc: Michael Brown <***@fensystems.co.uk>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Matt Fleming <***@intel.com>
---
MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 33cad23..f7b740c 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -275,6 +275,16 @@ PeCoffLoaderGetPeHeader (
}

//
+ // 4.1 Check that the SizeOfImage field is a multiple of
+ // SectionAlignment, since this is required by the PE/COFF
+ // specification.
+ //
+ if (Hdr.Pe32->OptionalHeader.SizeOfImage % Hdr.Pe32->OptionalHeader.SectionAlignment) {
+ ImageContext->ImageError = IMAGE_ERROR_UNSUPPORTED;
+ return RETURN_UNSUPPORTED;
+ }
+
+ //
// 4.2 Read last byte of Hdr.Pe32.OptionalHeader.SizeOfHeaders from the file.
//
Size = 1;
@@ -389,6 +399,16 @@ PeCoffLoaderGetPeHeader (
}

//
+ // 4.1 Check that the SizeOfImage field is a multiple of
+ // SectionAlignment, since this is required by the PE/COFF
+ // specification.
+ //
+ if (Hdr.Pe32Plus->OptionalHeader.SizeOfImage % Hdr.Pe32Plus->OptionalHeader.SectionAlignment) {
+ ImageContext->ImageError = IMAGE_ERROR_UNSUPPORTED;
+ return RETURN_UNSUPPORTED;
+ }
+
+ //
// 4.2 Read last byte of Hdr.Pe32Plus.OptionalHeader.SizeOfHeaders from the file.
//
Size = 1;
--
2.1.0

[Linn Crosetto]

However, there doesn't seem to be to be much value in breaking the
ability of EDK2-based builds to load such kernels. In particular, this
is likely to mean that future versions of OVMF will be unable to boot
some current Linux distributions.

Yeah, OK, that's a very good point.
My concern here is that we have enough divergence between different UEFI
implementations as it is, so if we get a chance to reduce that (by
making things fail consistently across platforms) I'm all for it.
Perhaps what we should do is hide these checks inside a DEBUG/DIAGNOSTIC
guard? That way I can enable it on my machines when verifying new EFI
kernel patches, and at the same time we won't go breaking people's
working setups.

Sounds good to me!

Sounds good to me as well. I did not actually have a problem on a system
refusing to load the kernel, rather I noticed it while using another application
that was checking PE/COFF headers.

------------------------------------------------------------------------------

[Linn Crosetto]

Post by Linn Crosetto
Sounds good to me as well. I did not actually have a problem on a system
refusing to load the kernel, rather I noticed it while using another application
that was checking PE/COFF headers.

Hmm.. that definitely changes the severity of the issue. I thought you
experiencing a booting issue, but it sounds like you are not.
What was the application that reported the error?

Signtool. The problem that is actually causing the failure is that FileAlignment
is less than 512 and not equal to SectionAlignment. I was going to look at
creating a patch to keep these values in line with the spec.