On Wed, 4 Dec 2024, Pali Rohár wrote:
PE format allows to have variable length of Data Directories in PE Optional
Header. The exact number of Data Directories is stored in NumberOfRvaAndSizes
field. Size of the optional header depends on the number of Data Directories.
Constants IMAGE_SIZEOF_NT_OPTIONAL32_HEADER and
IMAGE_SIZEOF_NT_OPTIONAL64_HEADER
are for PE images with all 16 Data Directories. And so cannot be used for
checking validity of SizeOfOptionalHeader.
Older PE linkers were generating PE binaries with less number of Data
Directories than 16 if trailing entries were empty. And gendef cannot
recognize such PE binaries.
Relax check for PE SizeOfOptionalHeader field. Allow any number of Data
Directories in PE Optional Header, including zero.
At the same time add checks for NumberOfRvaAndSizes member before
dereferencing DataDirectory[] array, which ensures that the entry is present.
---
mingw-w64-tools/gendef/src/gendef.c | 55 +++++++++++++++++++++--------
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/mingw-w64-tools/gendef/src/gendef.c
b/mingw-w64-tools/gendef/src/gendef.c
index 7440d60f35df..1f34edea2f07 100644
--- a/mingw-w64-tools/gendef/src/gendef.c
+++ b/mingw-w64-tools/gendef/src/gendef.c
@@ -18,6 +18,7 @@
#ifdef HAVE_CONFIG_H
#include "config.h"
#endif
+#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <memory.h>
@@ -313,19 +314,19 @@ load_pep (void)
gPEDta = NULL;
return 0;
}
- if (gPEDta->FileHeader.SizeOfOptionalHeader ==
IMAGE_SIZEOF_NT_OPTIONAL32_HEADER
+ if (gPEDta->FileHeader.SizeOfOptionalHeader >=
offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory)
&& gPEDta->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC)
{
gPEPDta = NULL;
fprintf (stderr, " * [%s] Found PE image\n", fninput);
}
- else if (gPEDta->FileHeader.SizeOfOptionalHeader ==
IMAGE_SIZEOF_NT_OPTIONAL32_HEADER
+ else if (gPEDta->FileHeader.SizeOfOptionalHeader >=
offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory)
&& gPEDta->OptionalHeader.Magic == 0 && gPEDta->FileHeader.Machine ==
0x014c /* IMAGE_FILE_MACHINE_I386 */)
{
gPEPDta = NULL;
fprintf (stderr, " * [%s] Found PE image for I386 with zeroed NT
magic\n", fninput);
}
- else if (gPEPDta->FileHeader.SizeOfOptionalHeader ==
IMAGE_SIZEOF_NT_OPTIONAL64_HEADER
+ else if (gPEPDta->FileHeader.SizeOfOptionalHeader >=
offsetof(IMAGE_OPTIONAL_HEADER64, DataDirectory)
&& gPEPDta->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC)
{
gPEDta = NULL;
Thanks, these changes look more readable to me now.
@@ -386,19 +387,27 @@ is_data (uint32_t va)
static int
is_reloc (uint32_t va)
{
- uint32_t va_rel, sz_rel, pos;
+ uint32_t va_rel = 0;
+ uint32_t sz_rel = 0;
+ uint32_t pos;
unsigned char *p;
PIMAGE_BASE_RELOCATION brel;
if (gPEDta)
{
- va_rel =
gPEDta->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress;
- sz_rel =
gPEDta->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].Size;
+ if (gPEDta->OptionalHeader.NumberOfRvaAndSizes >
IMAGE_DIRECTORY_ENTRY_BASERELOC)
+ {
+ va_rel =
gPEDta->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress;
+ sz_rel =
gPEDta->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].Size;
+ }
}
For these changes, isn't the > comparison wrong? Wouldn't it be enough if
we have OptionalHeader.NumberOfRvaAndSizes >=
IMAGE_DIRECTORY_ENTRY_BASERELOC?
(Other than that, the patch is ok with me; if you agree with this I can
push it with these comparisons changed into >=.)
I see that do_pedef changes cases for both IMAGE_DIRECTORY_ENTRY_IMPORT
and IMAGE_DIRECTORY_ENTRY_EXPORT, but do_pepdef only changes cases for
IMAGE_DIRECTORY_ENTRY_EXPORT. Is there no matching case for
IMAGE_DIRECTORY_ENTRY_IMPORT for PE+ binaries?
// Martin
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
