Syslinux - Nov 2013 - [PATCH 2/4] efi: Location, size and alignment of .text section

The PE headers of the generated efi file were quite buggy. And since OVMF
perform a few consistency checks, syslinux was unable to run on it. I don't
pretend to have a thorough understanding of the PE+ headers, some bugs may
remain. :)

Celelibi (4):
  efi: Fix PE header field rva_and_sizes_nr
  efi: Location, size and alignment of .text section
  efi: Useless relocations in PE file
  efi: PE file size differ from in-memory size

 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 206 ++++++++++++++++---------------------------------
 efi/wrapper.h          |  28 ++++---
 efi/x86_64/syslinux.ld |   4 +-
 4 files changed, 87 insertions(+), 155 deletions(-)

-- 
1.8.4.3

The value of the field rva_and_sizes_nr is used by OVMF to check the
consistency of the PE file with respect to the field optional_hdr_sz. It
now have the right value.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/wrapper.c |  6 +++---
 efi/wrapper.h | 28 ++++++++++++++++------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 04c895f..ec77271 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -102,7 +102,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr.image_sz = total_sz;
 		e_hdr.headers_sz = 512;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr.rva_and_sizes_nr = 1;
+		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
@@ -130,7 +130,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr_pe32p.image_sz = total_sz;
 		e_hdr_pe32p.headers_sz = 512;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr_pe32p.rva_and_sizes_nr = 1;
+		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
 	}
 
@@ -237,7 +237,7 @@ int main(int argc, char **argv)
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
 	}
-		
+
 	if (id[EI_MAG0] != ELFMAG0 ||
 	    id[EI_MAG1] != ELFMAG1 ||
 	    id[EI_MAG2] != ELFMAG2 ||
diff --git a/efi/wrapper.h b/efi/wrapper.h
index 4f76991..0e6b38e 100644
--- a/efi/wrapper.h
+++ b/efi/wrapper.h
@@ -102,12 +102,14 @@ struct extra_hdr {
 	__uint32_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 /* Extra header for PE32+ format 
@@ -136,12 +138,14 @@ struct extra_hdr_pe32p {
 	__uint64_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 struct section {
-- 
1.8.4.3

In the generated PE file, the section header for the .text section used
to address more than the whole file. Starting at offset 0 (before the
end of the headers) is illegal and is rejected by OVMF. Giving a size
greater than the actual file size is also illegal and rejected.

Moreover, the actual data inside the PE file have to be aligned to at
least 512 bytes. Hence, .text need to be aligned as well.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/wrapper.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index ec77271..bd2c175 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -54,11 +54,15 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	struct coff_hdr c_hdr;
 	struct header hdr;
 	struct coff_reloc c_rel;
-	__uint32_t total_sz = so_size;
+	__uint32_t total_sz = data_size;
 	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
+	hdr_sz = 512;
+	total_sz += hdr_sz;
+	entry += hdr_sz;
+
 	memset(&hdr, 0, sizeof(hdr));
 	hdr.msdos_signature = MSDOS_SIGNATURE;
 
@@ -77,11 +81,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	c_hdr.nr_sections = 2;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
-		hdr_sz = sizeof(o_hdr) + sizeof(t_sec) + sizeof(e_hdr) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
 		c_hdr.characteristics = IMAGE_FILE_32BIT_MACHINE |
 			IMAGE_FILE_DEBUG_STRIPPED | IMAGE_FILE_EXECUTABLE_IMAGE |
@@ -92,25 +91,20 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr.format = PE32_FORMAT;
 		o_hdr.major_linker_version = 0x02;
 		o_hdr.minor_linker_version = 0x14;
-		o_hdr.code_sz = total_sz;
+		o_hdr.code_sz = data_size;
 		o_hdr.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr, sizeof(o_hdr), 1, f);
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = total_sz;
-		e_hdr.headers_sz = 512;
+		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
-		hdr_sz = sizeof(o_hdr_pe32p) + sizeof(t_sec) + sizeof(e_hdr_pe32p) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_X86_64;
 		c_hdr.characteristics = IMAGE_FILE_DEBUG_STRIPPED |
IMAGE_FILE_EXECUTABLE_IMAGE |
 			IMAGE_FILE_LINE_NUMBERS_STRIPPED;
@@ -120,15 +114,15 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.format = PE32P_FORMAT;
 		o_hdr_pe32p.major_linker_version = 0x02;
 		o_hdr_pe32p.minor_linker_version = 0x14;
-		o_hdr_pe32p.code_sz = total_sz;
+		o_hdr_pe32p.code_sz = data_size;
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = total_sz;
-		e_hdr_pe32p.headers_sz = 512;
+		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
@@ -136,8 +130,10 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 
 	memset(&t_sec, 0, sizeof(t_sec));
 	strcpy((char *)t_sec.name, ".text");
-	t_sec.virtual_sz = total_sz;
-	t_sec.raw_data_sz = total_sz;
+	t_sec.virtual_sz = data_size;
+	t_sec.virtual_address = hdr_sz;
+	t_sec.raw_data_sz = t_sec.virtual_sz;
+	t_sec.raw_data = t_sec.virtual_address;
 	t_sec.characteristics = IMAGE_SCN_CNT_CODE |
 		IMAGE_SCN_ALIGN_16BYTES | IMAGE_SCN_MEM_EXECUTE |
 		IMAGE_SCN_MEM_READ;
@@ -163,6 +159,16 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&c_rel, sizeof(c_rel), 1, f);
 	fwrite(&dummy, sizeof(dummy), 1, f);
 
+	/*
+	 * Add some padding to align the ELF as needed
+	 */
+	if (ftell(f) > t_sec.virtual_address) {
+		// A fseek that rewind would be a bug hard to track
+		fprintf(stderr, "PE+ headers are too large.\n");
+		exit(EXIT_FAILURE);
+	}
+
+	fseek(f, t_sec.virtual_address, SEEK_SET);
 }
 
 static void usage(char *progname)
-- 
1.8.4.3

There is no need to have a relocation section that nothing points at.
The image is still seen as relocatable as long as the Characteristics of
the FileHeader do not say otherwise.

Moreover, the field base_relocation_table wasn't initialized properly
leading to unpredictable bugs.

Signed-off-by: Celelibi <celelibi at gmail.com>
---

I'm not 100% positive about the uselessness of the relocation section.
However:
1) it works on my real hardware;
2) OVMF doesn't check the section headers at all;
3) all the docs I could find say that the section names are arbitrary;
4) the only way to make the relocation actually happen is by setting the
base_relocation_table field of the extra headers to point to this section;
5) The right way to indicate the image is not relocatable would be by setting
the characteristics IMAGE_FILE_RELOCS_STRIPPED in the coff header.

Moreover, the relocation entry cannot be inserted before the end of the headers
(512 bytes), this mean we would have to push everything a few bytes further.
And I think it's better if it can be avoided. :)

 efi/wrapper.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index bd2c175..9652368 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -48,14 +48,12 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
-	struct section t_sec, r_sec;
+	struct section t_sec;
 	struct extra_hdr e_hdr;
 	struct extra_hdr_pe32p e_hdr_pe32p;
 	struct coff_hdr c_hdr;
 	struct header hdr;
-	struct coff_reloc c_rel;
 	__uint32_t total_sz = data_size;
-	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
@@ -78,7 +76,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&hdr, sizeof(hdr), 1, f);
 
 	memset(&c_hdr, 0, sizeof(c_hdr));
-	c_hdr.nr_sections = 2;
+	c_hdr.nr_sections = 1;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
@@ -118,7 +116,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
-		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
+		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
 		e_hdr_pe32p.image_sz = hdr_sz + so_size;
@@ -140,26 +138,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&t_sec, sizeof(t_sec), 1, f);
 
 	/*
-	 * Write our dummy relocation and reloc section.
-	 */
-	memset(&r_sec, 0, sizeof(r_sec));
-	strcpy((char *)r_sec.name, ".reloc");
-	r_sec.virtual_sz = sizeof(c_rel);
-	r_sec.virtual_address = ftell(f) + sizeof(r_sec);
-	r_sec.raw_data_sz = r_sec.virtual_sz;
-	r_sec.raw_data = r_sec.virtual_address;
-	r_sec.characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
-		IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_MEM_DISCARDABLE |
-		IMAGE_SCN_MEM_READ;
-	fwrite(&r_sec, sizeof(r_sec), 1, f);
-
-	memset(&c_rel, 0, sizeof(c_rel));
-	c_rel.virtual_address = ftell(f) + sizeof(c_rel);
-	c_rel.symtab_index = 10;
-	fwrite(&c_rel, sizeof(c_rel), 1, f);
-	fwrite(&dummy, sizeof(dummy), 1, f);
-
-	/*
 	 * Add some padding to align the ELF as needed
 	 */
 	if (ftell(f) > t_sec.virtual_address) {
-- 
1.8.4.3

PE headers code_sz and image_sz indicate more or less, the size of the
file and the size of the in-memory image. They are now given the right
value.

In the ELF format, only the program headers are reliable to determine
the actually needed part of the file and the in-memory size.

The .bss section should always be marked as NOLOAD for ld since its
content shouldn't be included into the binary file.

Signed-off-by: Celelibi <celelibi at gmail.com>
---

Again, I'm not 100% positive about the semantics of NOLOAD in a linker
script.
However I found this make things work better.
Actually the file core/fs/fs.o have a .bss16 section that is not empty. If I
understood correctly, only .bss can be empty in a .o. Thus ld generates the
zeros that belong to that section in the .o. Then, when merging the section
.bss16 into the .bss of syslinux.so, ld is forced to generate ALL the zeros
that belong to the .bss section. Marking this section as NOLOAD make ld to
ignore its content but still produce a PT_LOAD program header with the right
memory size, as needed for the bss.

 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 138 +++++++++++++++----------------------------------
 efi/x86_64/syslinux.ld |   4 +-
 3 files changed, 45 insertions(+), 101 deletions(-)

diff --git a/efi/i386/syslinux.ld b/efi/i386/syslinux.ld
index 523a9b9..bab3fc7 100644
--- a/efi/i386/syslinux.ld
+++ b/efi/i386/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
diff --git a/efi/wrapper.c b/efi/wrapper.c
index 9652368..a5247ae 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -35,7 +35,7 @@ typedef Elf64_Addr Elf_Addr;
 #endif
 
 /*
- * 'so_size' is the file size of the ELF shared object.
+ * 'so_memsz' is the size of the ELF shared object once loaded.
  * 'data_size' is the size of initialised data in the shared object.
  *  'class' dictates how the header is written
  * 	For 32bit machines (class == ELFCLASS32), the optional
@@ -44,7 +44,7 @@ typedef Elf64_Addr Elf_Addr;
  * 	header includes PE32+header fields
  */
 static void write_header(FILE *f, __uint32_t entry, size_t data_size,
-			 __uint32_t so_size, __uint8_t class)
+			 __uint32_t so_memsz, __uint8_t class)
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
@@ -96,7 +96,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.image_sz = hdr_sz + so_memsz;
 		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
@@ -119,7 +119,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.image_sz = hdr_sz + so_memsz;
 		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
@@ -157,17 +157,16 @@ static void usage(char *progname)
 
 int main(int argc, char **argv)
 {
-	struct stat st;
 	Elf32_Ehdr e32_hdr;
 	Elf64_Ehdr e64_hdr;
 	__uint32_t entry;
 	__uint8_t class;
-	__uint64_t shoff;
-	__uint16_t shnum, shentsize, shstrndx;
+	__uint64_t phoff = 0;
+	__uint16_t phnum = 0, phentsize = 0;
 	unsigned char *id;
 	FILE *f_in, *f_out;
 	void *buf;
-	size_t datasz, rv;
+	size_t datasz, memsz, rv;
 
 	if (argc < 3) {
 		usage(argv[0]);
@@ -180,11 +179,6 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (stat(argv[1], &st) != 0) {
-		perror("stat");
-		exit(EXIT_FAILURE);
-	}
-
 	f_out = fopen(argv[2], "w");
 	if (!f_out) {
 		perror("fopen");
@@ -194,15 +188,14 @@ int main(int argc, char **argv)
 	/*
 	 * Parse the ELF header and find the entry point.
 	 */
- 	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
+	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
 		id = e32_hdr.e_ident;
 		class = ELFCLASS32;
 		entry = e32_hdr.e_entry;
-		shoff = e32_hdr.e_shoff;
-		shnum = e32_hdr.e_shnum;
-		shstrndx = e32_hdr.e_shstrndx;
-		shentsize = e32_hdr.e_shentsize;
+		phoff = e32_hdr.e_phoff;
+		phnum = e32_hdr.e_phnum;
+		phentsize = e32_hdr.e_phentsize;
 	}
 	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
 		/* read the header again for x86_64 
@@ -213,10 +206,9 @@ int main(int argc, char **argv)
 		fread((void *)&e64_hdr, sizeof(e64_hdr), 1, f_in);
 		id = e64_hdr.e_ident;
 		entry = e64_hdr.e_entry;
-		shoff = e64_hdr.e_shoff;
-		shnum = e64_hdr.e_shnum;
-		shstrndx = e64_hdr.e_shstrndx;
-		shentsize = e64_hdr.e_shentsize;
+		phoff = e64_hdr.e_phoff;
+		phnum = e64_hdr.e_phnum;
+		phentsize = e64_hdr.e_phentsize;
 	} else {
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
@@ -230,98 +222,47 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (!shoff || !shnum || (shstrndx == SHN_UNDEF)) {
-		fprintf(stderr, "Cannot find section table\n");
+	if (!phoff || !phnum) {
+		fprintf(stderr, "Cannot find segment table\n");
 		exit(EXIT_FAILURE);
 	}
 
 	/*
-	 * Find the beginning of the .bss section. Everything preceding
-	 * it is copied verbatim to the output file.
+	 * Find the LOAD program header. Everything in this segment
+	 * is copied verbatim to the output file.
+	 * Although there may be several LOAD program headers, only
+	 * one is currently copied.
 	 */
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
-		const char *shstrtab, *name;
-		Elf32_Shdr shdr;
+		Elf32_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
-
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
 
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
-
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
-	}
-	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
-		const char *shstrtab, *name;
-		Elf64_Shdr shdr;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
+	} else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
+		Elf64_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
 
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
-
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
-
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
 	}
 
 	buf = malloc(datasz);
@@ -330,7 +271,7 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	write_header(f_out, entry, datasz, st.st_size, class);
+	write_header(f_out, entry, datasz, memsz, class);
 
 	/* Write out the entire ELF shared object */
 	rewind(f_in);
@@ -341,5 +282,8 @@ int main(int argc, char **argv)
 	}
 
 	fwrite(buf, datasz, rv, f_out);
+	free(buf);
+	fclose(f_out);
+	fclose(f_in);
 	return 0;
 }
diff --git a/efi/x86_64/syslinux.ld b/efi/x86_64/syslinux.ld
index 95160bd..450641c 100644
--- a/efi/x86_64/syslinux.ld
+++ b/efi/x86_64/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
-- 
1.8.4.3

2013/11/27, Celelibi <celelibi at gmail.com>:
> The PE headers of the generated efi file were quite buggy. And since OVMF
> perform a few consistency checks, syslinux was unable to run on it. I
don't
> pretend to have a thorough understanding of the PE+ headers, some bugs may
> remain. :)
>
> Celelibi (4):
>   efi: Fix PE header field rva_and_sizes_nr
>   efi: Location, size and alignment of .text section
>   efi: Useless relocations in PE file
>   efi: PE file size differ from in-memory size
>
>  efi/i386/syslinux.ld   |   4 +-
>  efi/wrapper.c          | 206
> ++++++++++++++++---------------------------------
>  efi/wrapper.h          |  28 ++++---
>  efi/x86_64/syslinux.ld |   4 +-
>  4 files changed, 87 insertions(+), 155 deletions(-)
>
> --
> 1.8.4.3
>
>
I didn't see those on the bug tracker before, but these patches may
actually resolve bugs #8 and #23.

On Wed, 27 Nov, at 02:38:28PM, Celelibi wrote:
> The value of the field rva_and_sizes_nr is used by OVMF to check the
> consistency of the PE file with respect to the field optional_hdr_sz. It
> now have the right value.
> 
> Signed-off-by: Celelibi <celelibi at gmail.com>
> ---
>  efi/wrapper.c |  6 +++---
>  efi/wrapper.h | 28 ++++++++++++++++------------
>  2 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/efi/wrapper.c b/efi/wrapper.c
> index 04c895f..ec77271 100644
> --- a/efi/wrapper.c
> +++ b/efi/wrapper.c
> @@ -102,7 +102,7 @@ static void write_header(FILE *f, __uint32_t entry,
size_t data_size,
>  		e_hdr.image_sz = total_sz;
>  		e_hdr.headers_sz = 512;
>  		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
> -		e_hdr.rva_and_sizes_nr = 1;
> +		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) /
sizeof(__uint64_t);

Since the data directories aren't ever used, can't we just delete them
and set rva_and_sizes_nr to 0?

-- 
Matt Fleming, Intel Open Source Technology Center

On Wed, 27 Nov, at 02:38:29PM, Celelibi wrote:
> In the generated PE file, the section header for the .text section used
> to address more than the whole file. Starting at offset 0 (before the
> end of the headers) is illegal and is rejected by OVMF. Giving a size
> greater than the actual file size is also illegal and rejected.
> 
> Moreover, the actual data inside the PE file have to be aligned to at
> least 512 bytes. Hence, .text need to be aligned as well.
> 
> Signed-off-by: Celelibi <celelibi at gmail.com>
> ---
>  efi/wrapper.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/efi/wrapper.c b/efi/wrapper.c
> index ec77271..bd2c175 100644
> --- a/efi/wrapper.c
> +++ b/efi/wrapper.c
> @@ -54,11 +54,15 @@ static void write_header(FILE *f, __uint32_t entry,
size_t data_size,
>  	struct coff_hdr c_hdr;
>  	struct header hdr;
>  	struct coff_reloc c_rel;
> -	__uint32_t total_sz = so_size;
> +	__uint32_t total_sz = data_size;
>  	__uint32_t dummy = 0;
>  	__uint32_t hdr_sz;
>  	__uint32_t reloc_start, reloc_end;
>  
> +	hdr_sz = 512;
Please include a comment to explain the magic 512.
> @@ -163,6 +159,16 @@ static void write_header(FILE *f, __uint32_t entry,
size_t data_size,
>  	fwrite(&c_rel, sizeof(c_rel), 1, f);
>  	fwrite(&dummy, sizeof(dummy), 1, f);
>  
> +	/*
> +	 * Add some padding to align the ELF as needed
> +	 */
> +	if (ftell(f) > t_sec.virtual_address) {
> +		// A fseek that rewind would be a bug hard to track
Use C-style comments please.

Other than that, the patch looks good. Thanks!

-- 
Matt Fleming, Intel Open Source Technology Center

On Wed, 27 Nov, at 02:38:30PM, Celelibi wrote:
> There is no need to have a relocation section that nothing points at.
> The image is still seen as relocatable as long as the Characteristics of
> the FileHeader do not say otherwise.
> 
> Moreover, the field base_relocation_table wasn't initialized properly
> leading to unpredictable bugs.
> 
> Signed-off-by: Celelibi <celelibi at gmail.com>
> ---
> 
> I'm not 100% positive about the uselessness of the relocation section.
> However:
The fix looks pretty straight forward. I'll try it out on my hardware
here.

-- 
Matt Fleming, Intel Open Source Technology Center

On Wed, 27 Nov, at 02:38:31PM, Celelibi wrote:
> PE headers code_sz and image_sz indicate more or less, the size of the
> file and the size of the in-memory image. They are now given the right
> value.
> 
> In the ELF format, only the program headers are reliable to determine
> the actually needed part of the file and the in-memory size.
> 
> The .bss section should always be marked as NOLOAD for ld since its
> content shouldn't be included into the binary file.
> 
> Signed-off-by: Celelibi <celelibi at gmail.com>
Looks good.

-- 
Matt Fleming, Intel Open Source Technology Center

Here are the fix as suggested.

For patch 2/4 I chosed to hardcore the value 512 instead of computing the
fittest needed value. There are 3 reasons for that.
- The header size may not need to be bigger than this in a near future.
- There is an test in case increasing the header size would be needed.
- It would make the code quite more complex to keep a consistent value
for all the fields when the header size is not known from start.

Celelibi (4):
  efi: Fix PE header field rva_and_sizes_nr
  efi: Location, size and alignment of .text section
  efi: Useless relocations in PE file
  efi: PE file size differ from in-memory size

 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 210 +++++++++++++++++--------------------------------
 efi/wrapper.h          |  28 ++++---
 efi/x86_64/syslinux.ld |   4 +-
 4 files changed, 91 insertions(+), 155 deletions(-)

-- 
1.8.4.3

The value of the field rva_and_sizes_nr is used by OVMF to check the
consistency of the PE file with respect to the field optional_hdr_sz. It
now have the right value.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/wrapper.c |  6 +++---
 efi/wrapper.h | 28 ++++++++++++++++------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 04c895f..ec77271 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -102,7 +102,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr.image_sz = total_sz;
 		e_hdr.headers_sz = 512;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr.rva_and_sizes_nr = 1;
+		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
@@ -130,7 +130,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr_pe32p.image_sz = total_sz;
 		e_hdr_pe32p.headers_sz = 512;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr_pe32p.rva_and_sizes_nr = 1;
+		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
 	}
 
@@ -237,7 +237,7 @@ int main(int argc, char **argv)
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
 	}
-		
+
 	if (id[EI_MAG0] != ELFMAG0 ||
 	    id[EI_MAG1] != ELFMAG1 ||
 	    id[EI_MAG2] != ELFMAG2 ||
diff --git a/efi/wrapper.h b/efi/wrapper.h
index 4f76991..0e6b38e 100644
--- a/efi/wrapper.h
+++ b/efi/wrapper.h
@@ -102,12 +102,14 @@ struct extra_hdr {
 	__uint32_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 /* Extra header for PE32+ format 
@@ -136,12 +138,14 @@ struct extra_hdr_pe32p {
 	__uint64_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 struct section {
-- 
1.8.4.3

In the generated PE file, the section header for the .text section used
to address more than the whole file. Starting at offset 0 (before the
end of the headers) is illegal and is rejected by OVMF. Giving a size
greater than the actual file size is also illegal and rejected.

Moreover, the body of the PE file have to be aligned to at least 512
bytes. Hence, .text need to be aligned as well.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/wrapper.c | 48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index ec77271..8b553f8 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -54,11 +54,19 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	struct coff_hdr c_hdr;
 	struct header hdr;
 	struct coff_reloc c_rel;
-	__uint32_t total_sz = so_size;
+	__uint32_t total_sz = data_size;
 	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
+	/*
+	 * The header size have to be a multiple of file_align, which currently
+	 * is 512.
+	 */
+	hdr_sz = 512;
+	total_sz += hdr_sz;
+	entry += hdr_sz;
+
 	memset(&hdr, 0, sizeof(hdr));
 	hdr.msdos_signature = MSDOS_SIGNATURE;
 
@@ -77,11 +85,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	c_hdr.nr_sections = 2;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
-		hdr_sz = sizeof(o_hdr) + sizeof(t_sec) + sizeof(e_hdr) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
 		c_hdr.characteristics = IMAGE_FILE_32BIT_MACHINE |
 			IMAGE_FILE_DEBUG_STRIPPED | IMAGE_FILE_EXECUTABLE_IMAGE |
@@ -92,25 +95,20 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr.format = PE32_FORMAT;
 		o_hdr.major_linker_version = 0x02;
 		o_hdr.minor_linker_version = 0x14;
-		o_hdr.code_sz = total_sz;
+		o_hdr.code_sz = data_size;
 		o_hdr.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr, sizeof(o_hdr), 1, f);
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = total_sz;
-		e_hdr.headers_sz = 512;
+		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
-		hdr_sz = sizeof(o_hdr_pe32p) + sizeof(t_sec) + sizeof(e_hdr_pe32p) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_X86_64;
 		c_hdr.characteristics = IMAGE_FILE_DEBUG_STRIPPED |
IMAGE_FILE_EXECUTABLE_IMAGE |
 			IMAGE_FILE_LINE_NUMBERS_STRIPPED;
@@ -120,15 +118,15 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.format = PE32P_FORMAT;
 		o_hdr_pe32p.major_linker_version = 0x02;
 		o_hdr_pe32p.minor_linker_version = 0x14;
-		o_hdr_pe32p.code_sz = total_sz;
+		o_hdr_pe32p.code_sz = data_size;
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = total_sz;
-		e_hdr_pe32p.headers_sz = 512;
+		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
@@ -136,8 +134,10 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 
 	memset(&t_sec, 0, sizeof(t_sec));
 	strcpy((char *)t_sec.name, ".text");
-	t_sec.virtual_sz = total_sz;
-	t_sec.raw_data_sz = total_sz;
+	t_sec.virtual_sz = data_size;
+	t_sec.virtual_address = hdr_sz;
+	t_sec.raw_data_sz = t_sec.virtual_sz;
+	t_sec.raw_data = t_sec.virtual_address;
 	t_sec.characteristics = IMAGE_SCN_CNT_CODE |
 		IMAGE_SCN_ALIGN_16BYTES | IMAGE_SCN_MEM_EXECUTE |
 		IMAGE_SCN_MEM_READ;
@@ -163,6 +163,16 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&c_rel, sizeof(c_rel), 1, f);
 	fwrite(&dummy, sizeof(dummy), 1, f);
 
+	/*
+	 * Add some padding to align the ELF as needed
+	 */
+	if (ftell(f) > t_sec.virtual_address) {
+		/* Don't rewind! hdr_sz need to be increased. */
+		fprintf(stderr, "PE32+ headers are too large.\n");
+		exit(EXIT_FAILURE);
+	}
+
+	fseek(f, t_sec.virtual_address, SEEK_SET);
 }
 
 static void usage(char *progname)
-- 
1.8.4.3

There is no need to have a relocation section that nothing points at.
The image is still seen as relocatable as long as the Characteristics of
the FileHeader do not say otherwise.

Moreover, the field base_relocation_table wasn't initialized properly
leading to unpredictable bugs.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/wrapper.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 8b553f8..4839e52 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -48,14 +48,12 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
-	struct section t_sec, r_sec;
+	struct section t_sec;
 	struct extra_hdr e_hdr;
 	struct extra_hdr_pe32p e_hdr_pe32p;
 	struct coff_hdr c_hdr;
 	struct header hdr;
-	struct coff_reloc c_rel;
 	__uint32_t total_sz = data_size;
-	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
@@ -82,7 +80,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&hdr, sizeof(hdr), 1, f);
 
 	memset(&c_hdr, 0, sizeof(c_hdr));
-	c_hdr.nr_sections = 2;
+	c_hdr.nr_sections = 1;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
@@ -122,7 +120,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
-		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
+		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
 		e_hdr_pe32p.image_sz = hdr_sz + so_size;
@@ -144,26 +142,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&t_sec, sizeof(t_sec), 1, f);
 
 	/*
-	 * Write our dummy relocation and reloc section.
-	 */
-	memset(&r_sec, 0, sizeof(r_sec));
-	strcpy((char *)r_sec.name, ".reloc");
-	r_sec.virtual_sz = sizeof(c_rel);
-	r_sec.virtual_address = ftell(f) + sizeof(r_sec);
-	r_sec.raw_data_sz = r_sec.virtual_sz;
-	r_sec.raw_data = r_sec.virtual_address;
-	r_sec.characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
-		IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_MEM_DISCARDABLE |
-		IMAGE_SCN_MEM_READ;
-	fwrite(&r_sec, sizeof(r_sec), 1, f);
-
-	memset(&c_rel, 0, sizeof(c_rel));
-	c_rel.virtual_address = ftell(f) + sizeof(c_rel);
-	c_rel.symtab_index = 10;
-	fwrite(&c_rel, sizeof(c_rel), 1, f);
-	fwrite(&dummy, sizeof(dummy), 1, f);
-
-	/*
 	 * Add some padding to align the ELF as needed
 	 */
 	if (ftell(f) > t_sec.virtual_address) {
-- 
1.8.4.3

PE headers code_sz and image_sz indicate more or less, the size of the
file and the size of the in-memory image. They are now given the right
value.

In the ELF format, only the program headers are reliable to determine
the actually needed part of the file and the in-memory size.

The .bss section should always be marked as NOLOAD for ld since its
content shouldn't be included into the binary file.

Signed-off-by: Celelibi <celelibi at gmail.com>
---
 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 138 +++++++++++++++----------------------------------
 efi/x86_64/syslinux.ld |   4 +-
 3 files changed, 45 insertions(+), 101 deletions(-)

diff --git a/efi/i386/syslinux.ld b/efi/i386/syslinux.ld
index 523a9b9..bab3fc7 100644
--- a/efi/i386/syslinux.ld
+++ b/efi/i386/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
diff --git a/efi/wrapper.c b/efi/wrapper.c
index 4839e52..1988124 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -35,7 +35,7 @@ typedef Elf64_Addr Elf_Addr;
 #endif
 
 /*
- * 'so_size' is the file size of the ELF shared object.
+ * 'so_memsz' is the size of the ELF shared object once loaded.
  * 'data_size' is the size of initialised data in the shared object.
  *  'class' dictates how the header is written
  * 	For 32bit machines (class == ELFCLASS32), the optional
@@ -44,7 +44,7 @@ typedef Elf64_Addr Elf_Addr;
  * 	header includes PE32+header fields
  */
 static void write_header(FILE *f, __uint32_t entry, size_t data_size,
-			 __uint32_t so_size, __uint8_t class)
+			 __uint32_t so_memsz, __uint8_t class)
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
@@ -100,7 +100,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.image_sz = hdr_sz + so_memsz;
 		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
@@ -123,7 +123,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.image_sz = hdr_sz + so_memsz;
 		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
@@ -161,17 +161,16 @@ static void usage(char *progname)
 
 int main(int argc, char **argv)
 {
-	struct stat st;
 	Elf32_Ehdr e32_hdr;
 	Elf64_Ehdr e64_hdr;
 	__uint32_t entry;
 	__uint8_t class;
-	__uint64_t shoff;
-	__uint16_t shnum, shentsize, shstrndx;
+	__uint64_t phoff = 0;
+	__uint16_t phnum = 0, phentsize = 0;
 	unsigned char *id;
 	FILE *f_in, *f_out;
 	void *buf;
-	size_t datasz, rv;
+	size_t datasz, memsz, rv;
 
 	if (argc < 3) {
 		usage(argv[0]);
@@ -184,11 +183,6 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (stat(argv[1], &st) != 0) {
-		perror("stat");
-		exit(EXIT_FAILURE);
-	}
-
 	f_out = fopen(argv[2], "w");
 	if (!f_out) {
 		perror("fopen");
@@ -198,15 +192,14 @@ int main(int argc, char **argv)
 	/*
 	 * Parse the ELF header and find the entry point.
 	 */
- 	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
+	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
 		id = e32_hdr.e_ident;
 		class = ELFCLASS32;
 		entry = e32_hdr.e_entry;
-		shoff = e32_hdr.e_shoff;
-		shnum = e32_hdr.e_shnum;
-		shstrndx = e32_hdr.e_shstrndx;
-		shentsize = e32_hdr.e_shentsize;
+		phoff = e32_hdr.e_phoff;
+		phnum = e32_hdr.e_phnum;
+		phentsize = e32_hdr.e_phentsize;
 	}
 	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
 		/* read the header again for x86_64 
@@ -217,10 +210,9 @@ int main(int argc, char **argv)
 		fread((void *)&e64_hdr, sizeof(e64_hdr), 1, f_in);
 		id = e64_hdr.e_ident;
 		entry = e64_hdr.e_entry;
-		shoff = e64_hdr.e_shoff;
-		shnum = e64_hdr.e_shnum;
-		shstrndx = e64_hdr.e_shstrndx;
-		shentsize = e64_hdr.e_shentsize;
+		phoff = e64_hdr.e_phoff;
+		phnum = e64_hdr.e_phnum;
+		phentsize = e64_hdr.e_phentsize;
 	} else {
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
@@ -234,98 +226,47 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (!shoff || !shnum || (shstrndx == SHN_UNDEF)) {
-		fprintf(stderr, "Cannot find section table\n");
+	if (!phoff || !phnum) {
+		fprintf(stderr, "Cannot find segment table\n");
 		exit(EXIT_FAILURE);
 	}
 
 	/*
-	 * Find the beginning of the .bss section. Everything preceding
-	 * it is copied verbatim to the output file.
+	 * Find the LOAD program header. Everything in this segment
+	 * is copied verbatim to the output file.
+	 * Although there may be several LOAD program headers, only
+	 * one is currently copied.
 	 */
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
-		const char *shstrtab, *name;
-		Elf32_Shdr shdr;
+		Elf32_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
-
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
 
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
-
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
-	}
-	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
-		const char *shstrtab, *name;
-		Elf64_Shdr shdr;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
+	} else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
+		Elf64_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
 
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
-
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
-
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
 	}
 
 	buf = malloc(datasz);
@@ -334,7 +275,7 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	write_header(f_out, entry, datasz, st.st_size, class);
+	write_header(f_out, entry, datasz, memsz, class);
 
 	/* Write out the entire ELF shared object */
 	rewind(f_in);
@@ -345,5 +286,8 @@ int main(int argc, char **argv)
 	}
 
 	fwrite(buf, datasz, rv, f_out);
+	free(buf);
+	fclose(f_out);
+	fclose(f_in);
 	return 0;
 }
diff --git a/efi/x86_64/syslinux.ld b/efi/x86_64/syslinux.ld
index 95160bd..450641c 100644
--- a/efi/x86_64/syslinux.ld
+++ b/efi/x86_64/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
-- 
1.8.4.3

This patch series is the lastest version that fix the PE headers generated for
syslinux for UEFI.

Compared to the previous versions of this patch series, the relocation fixing
has been made less brutal. Namely, patch 3/5 remove the useless relocation that
is rejected by OVMF and the whole .reloc section removal has been made a
separate patch 5/5. This last patch is optional.

Of course they are now based on the lastest master branch.

And last but not least, I signed those patches with my real name. :)

Sylvain Gault (5):
  efi: Fix PE header field rva_and_sizes_nr
  efi: Location, size and alignment of .text section
  efi: Remove buggy relocation in PE file
  efi: PE file size differ from in-memory size
  efi: Useless relocation section in PE file

 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 210 +++++++++++++++++--------------------------------
 efi/wrapper.h          |  28 ++++---
 efi/x86_64/syslinux.ld |   4 +-
 4 files changed, 91 insertions(+), 155 deletions(-)

-- 
1.8.5.2

The value of the field rva_and_sizes_nr is used by OVMF to check the
consistency of the PE file with respect to the field optional_hdr_sz. It
now have the right value.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/wrapper.c |  6 +++---
 efi/wrapper.h | 28 ++++++++++++++++------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 04c895f..ec77271 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -102,7 +102,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr.image_sz = total_sz;
 		e_hdr.headers_sz = 512;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr.rva_and_sizes_nr = 1;
+		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
@@ -130,7 +130,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		e_hdr_pe32p.image_sz = total_sz;
 		e_hdr_pe32p.headers_sz = 512;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
-		e_hdr_pe32p.rva_and_sizes_nr = 1;
+		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
 	}
 
@@ -237,7 +237,7 @@ int main(int argc, char **argv)
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
 	}
-		
+
 	if (id[EI_MAG0] != ELFMAG0 ||
 	    id[EI_MAG1] != ELFMAG1 ||
 	    id[EI_MAG2] != ELFMAG2 ||
diff --git a/efi/wrapper.h b/efi/wrapper.h
index 4f76991..0e6b38e 100644
--- a/efi/wrapper.h
+++ b/efi/wrapper.h
@@ -102,12 +102,14 @@ struct extra_hdr {
 	__uint32_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 /* Extra header for PE32+ format 
@@ -136,12 +138,14 @@ struct extra_hdr_pe32p {
 	__uint64_t heap_commit_sz;
 	__uint32_t loader_flags;
 	__uint32_t rva_and_sizes_nr;
-	__uint64_t export_table;
-	__uint64_t import_table;
-	__uint64_t resource_table;
-	__uint64_t exception_table;
-	__uint64_t certification_table;
-	__uint64_t base_relocation_table;
+	struct {
+		__uint64_t export_table;
+		__uint64_t import_table;
+		__uint64_t resource_table;
+		__uint64_t exception_table;
+		__uint64_t certification_table;
+		__uint64_t base_relocation_table;
+	} data_directory;
 } __packed;
 
 struct section {
-- 
1.8.5.2

In the generated PE file, the section header for the .text section used
to address more than the whole file. Starting at offset 0 (before the
end of the headers) is illegal and is rejected by OVMF. Giving a size
greater than the actual file size is also illegal and rejected.

Moreover, the body of the PE file have to be aligned to at least 512
bytes. Hence, .text need to be aligned as well.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/wrapper.c | 48 +++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index ec77271..8b553f8 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -54,11 +54,19 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	struct coff_hdr c_hdr;
 	struct header hdr;
 	struct coff_reloc c_rel;
-	__uint32_t total_sz = so_size;
+	__uint32_t total_sz = data_size;
 	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
+	/*
+	 * The header size have to be a multiple of file_align, which currently
+	 * is 512
+	 */
+	hdr_sz = 512;
+	total_sz += hdr_sz;
+	entry += hdr_sz;
+
 	memset(&hdr, 0, sizeof(hdr));
 	hdr.msdos_signature = MSDOS_SIGNATURE;
 
@@ -77,11 +85,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	c_hdr.nr_sections = 2;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
-		hdr_sz = sizeof(o_hdr) + sizeof(t_sec) + sizeof(e_hdr) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
 		c_hdr.characteristics = IMAGE_FILE_32BIT_MACHINE |
 			IMAGE_FILE_DEBUG_STRIPPED | IMAGE_FILE_EXECUTABLE_IMAGE |
@@ -92,25 +95,20 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr.format = PE32_FORMAT;
 		o_hdr.major_linker_version = 0x02;
 		o_hdr.minor_linker_version = 0x14;
-		o_hdr.code_sz = total_sz;
+		o_hdr.code_sz = data_size;
 		o_hdr.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr, sizeof(o_hdr), 1, f);
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = total_sz;
-		e_hdr.headers_sz = 512;
+		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
 		fwrite(&e_hdr, sizeof(e_hdr), 1, f);
 	}
 	else if (class == ELFCLASS64) {
-		hdr_sz = sizeof(o_hdr_pe32p) + sizeof(t_sec) + sizeof(e_hdr_pe32p) +
-				sizeof(r_sec) + sizeof(c_hdr) + sizeof(hdr) + sizeof(c_rel)
-				+ sizeof(dummy);
-		total_sz += hdr_sz;
-		entry += hdr_sz;
 		c_hdr.arch = IMAGE_FILE_MACHINE_X86_64;
 		c_hdr.characteristics = IMAGE_FILE_DEBUG_STRIPPED |
IMAGE_FILE_EXECUTABLE_IMAGE |
 			IMAGE_FILE_LINE_NUMBERS_STRIPPED;
@@ -120,15 +118,15 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.format = PE32P_FORMAT;
 		o_hdr_pe32p.major_linker_version = 0x02;
 		o_hdr_pe32p.minor_linker_version = 0x14;
-		o_hdr_pe32p.code_sz = total_sz;
+		o_hdr_pe32p.code_sz = data_size;
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = total_sz;
-		e_hdr_pe32p.headers_sz = 512;
+		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
 		fwrite(&e_hdr_pe32p, sizeof(e_hdr_pe32p), 1, f);
@@ -136,8 +134,10 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 
 	memset(&t_sec, 0, sizeof(t_sec));
 	strcpy((char *)t_sec.name, ".text");
-	t_sec.virtual_sz = total_sz;
-	t_sec.raw_data_sz = total_sz;
+	t_sec.virtual_sz = data_size;
+	t_sec.virtual_address = hdr_sz;
+	t_sec.raw_data_sz = t_sec.virtual_sz;
+	t_sec.raw_data = t_sec.virtual_address;
 	t_sec.characteristics = IMAGE_SCN_CNT_CODE |
 		IMAGE_SCN_ALIGN_16BYTES | IMAGE_SCN_MEM_EXECUTE |
 		IMAGE_SCN_MEM_READ;
@@ -163,6 +163,16 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&c_rel, sizeof(c_rel), 1, f);
 	fwrite(&dummy, sizeof(dummy), 1, f);
 
+	/*
+	 * Add some padding to align the ELF as needed
+	 */
+	if (ftell(f) > t_sec.virtual_address) {
+		/* Don't rewind! hdr_sz need to be increased. */
+		fprintf(stderr, "PE32+ headers are too large.\n");
+		exit(EXIT_FAILURE);
+	}
+
+	fseek(f, t_sec.virtual_address, SEEK_SET);
 }
 
 static void usage(char *progname)
-- 
1.8.5.2

OVMF and probably other UEFI implementation refuse a file with a
relocation entry inside the headers. And since the dummy relocation
wasn't used at all, better remove it.

Moreover, the field base_relocation_table wasn't initialized properly,
leading to unpredictable bugs.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/wrapper.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 8b553f8..1630bbb 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -53,9 +53,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	struct extra_hdr_pe32p e_hdr_pe32p;
 	struct coff_hdr c_hdr;
 	struct header hdr;
-	struct coff_reloc c_rel;
 	__uint32_t total_sz = data_size;
-	__uint32_t dummy = 0;
 	__uint32_t hdr_sz;
 	__uint32_t reloc_start, reloc_end;
 
@@ -122,7 +120,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		o_hdr_pe32p.entry_point = entry;
 		o_hdr.initialized_data_sz = data_size;
 		fwrite(&o_hdr_pe32p, sizeof(o_hdr_pe32p), 1, f);
-		memset(&e_hdr_pe32p, 0, sizeof(e_hdr));
+		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
 		e_hdr_pe32p.image_sz = hdr_sz + so_size;
@@ -144,12 +142,14 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&t_sec, sizeof(t_sec), 1, f);
 
 	/*
-	 * Write our dummy relocation and reloc section.
+	 * Write our empty reloc section.
 	 */
 	memset(&r_sec, 0, sizeof(r_sec));
 	strcpy((char *)r_sec.name, ".reloc");
-	r_sec.virtual_sz = sizeof(c_rel);
-	r_sec.virtual_address = ftell(f) + sizeof(r_sec);
+	r_sec.virtual_sz = 0;
+	/* /!\ Attention! Some UEFI implementations require a valid virtual
+	 * address despite the virtual_sz = 0. */
+	r_sec.virtual_address = t_sec.virtual_address;
 	r_sec.raw_data_sz = r_sec.virtual_sz;
 	r_sec.raw_data = r_sec.virtual_address;
 	r_sec.characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
@@ -157,12 +157,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		IMAGE_SCN_MEM_READ;
 	fwrite(&r_sec, sizeof(r_sec), 1, f);
 
-	memset(&c_rel, 0, sizeof(c_rel));
-	c_rel.virtual_address = ftell(f) + sizeof(c_rel);
-	c_rel.symtab_index = 10;
-	fwrite(&c_rel, sizeof(c_rel), 1, f);
-	fwrite(&dummy, sizeof(dummy), 1, f);
-
 	/*
 	 * Add some padding to align the ELF as needed
 	 */
-- 
1.8.5.2

PE headers code_sz and image_sz indicate more or less, the size of the
file and the size of the in-memory image. They are now given the right
value.

In the ELF format, only the program headers are reliable to determine
the actually needed part of the file and the in-memory size.

The .bss section should always be marked as NOLOAD for ld since its
content shouldn't be included into the binary file.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/i386/syslinux.ld   |   4 +-
 efi/wrapper.c          | 138 +++++++++++++++----------------------------------
 efi/x86_64/syslinux.ld |   4 +-
 3 files changed, 45 insertions(+), 101 deletions(-)

diff --git a/efi/i386/syslinux.ld b/efi/i386/syslinux.ld
index 523a9b9..bab3fc7 100644
--- a/efi/i386/syslinux.ld
+++ b/efi/i386/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
diff --git a/efi/wrapper.c b/efi/wrapper.c
index 1630bbb..2b96665 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -35,7 +35,7 @@ typedef Elf64_Addr Elf_Addr;
 #endif
 
 /*
- * 'so_size' is the file size of the ELF shared object.
+ * 'so_memsz' is the size of the ELF shared object once loaded.
  * 'data_size' is the size of initialised data in the shared object.
  *  'class' dictates how the header is written
  * 	For 32bit machines (class == ELFCLASS32), the optional
@@ -44,7 +44,7 @@ typedef Elf64_Addr Elf_Addr;
  * 	header includes PE32+header fields
  */
 static void write_header(FILE *f, __uint32_t entry, size_t data_size,
-			 __uint32_t so_size, __uint8_t class)
+			 __uint32_t so_memsz, __uint8_t class)
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
@@ -100,7 +100,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr, 0, sizeof(e_hdr));
 		e_hdr.section_align = 4096;
 		e_hdr.file_align = 512;
-		e_hdr.image_sz = hdr_sz + so_size;
+		e_hdr.image_sz = hdr_sz + so_memsz;
 		e_hdr.headers_sz = hdr_sz;
 		e_hdr.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr.rva_and_sizes_nr = sizeof(e_hdr.data_directory) / sizeof(__uint64_t);
@@ -123,7 +123,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 		memset(&e_hdr_pe32p, 0, sizeof(e_hdr_pe32p));
 		e_hdr_pe32p.section_align = 4096;
 		e_hdr_pe32p.file_align = 512;
-		e_hdr_pe32p.image_sz = hdr_sz + so_size;
+		e_hdr_pe32p.image_sz = hdr_sz + so_memsz;
 		e_hdr_pe32p.headers_sz = hdr_sz;
 		e_hdr_pe32p.subsystem = IMAGE_SUBSYSTEM_EFI_APPLICATION;
 		e_hdr_pe32p.rva_and_sizes_nr = sizeof(e_hdr_pe32p.data_directory) /
sizeof(__uint64_t);
@@ -177,17 +177,16 @@ static void usage(char *progname)
 
 int main(int argc, char **argv)
 {
-	struct stat st;
 	Elf32_Ehdr e32_hdr;
 	Elf64_Ehdr e64_hdr;
 	__uint32_t entry;
 	__uint8_t class;
-	__uint64_t shoff;
-	__uint16_t shnum, shentsize, shstrndx;
+	__uint64_t phoff = 0;
+	__uint16_t phnum = 0, phentsize = 0;
 	unsigned char *id;
 	FILE *f_in, *f_out;
 	void *buf;
-	size_t datasz, rv;
+	size_t datasz, memsz, rv;
 
 	if (argc < 3) {
 		usage(argv[0]);
@@ -200,11 +199,6 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (stat(argv[1], &st) != 0) {
-		perror("stat");
-		exit(EXIT_FAILURE);
-	}
-
 	f_out = fopen(argv[2], "w");
 	if (!f_out) {
 		perror("fopen");
@@ -214,15 +208,14 @@ int main(int argc, char **argv)
 	/*
 	 * Parse the ELF header and find the entry point.
 	 */
- 	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
+	fread((void *)&e32_hdr, sizeof(e32_hdr), 1, f_in);
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
 		id = e32_hdr.e_ident;
 		class = ELFCLASS32;
 		entry = e32_hdr.e_entry;
-		shoff = e32_hdr.e_shoff;
-		shnum = e32_hdr.e_shnum;
-		shstrndx = e32_hdr.e_shstrndx;
-		shentsize = e32_hdr.e_shentsize;
+		phoff = e32_hdr.e_phoff;
+		phnum = e32_hdr.e_phnum;
+		phentsize = e32_hdr.e_phentsize;
 	}
 	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
 		/* read the header again for x86_64 
@@ -233,10 +226,9 @@ int main(int argc, char **argv)
 		fread((void *)&e64_hdr, sizeof(e64_hdr), 1, f_in);
 		id = e64_hdr.e_ident;
 		entry = e64_hdr.e_entry;
-		shoff = e64_hdr.e_shoff;
-		shnum = e64_hdr.e_shnum;
-		shstrndx = e64_hdr.e_shstrndx;
-		shentsize = e64_hdr.e_shentsize;
+		phoff = e64_hdr.e_phoff;
+		phnum = e64_hdr.e_phnum;
+		phentsize = e64_hdr.e_phentsize;
 	} else {
 		fprintf(stderr, "Unsupported architecture\n");
 		exit(EXIT_FAILURE);
@@ -250,98 +242,47 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	if (!shoff || !shnum || (shstrndx == SHN_UNDEF)) {
-		fprintf(stderr, "Cannot find section table\n");
+	if (!phoff || !phnum) {
+		fprintf(stderr, "Cannot find segment table\n");
 		exit(EXIT_FAILURE);
 	}
 
 	/*
-	 * Find the beginning of the .bss section. Everything preceding
-	 * it is copied verbatim to the output file.
+	 * Find the LOAD program header. Everything in this segment
+	 * is copied verbatim to the output file.
+	 * Although there may be several LOAD program headers, only
+	 * one is currently copied.
 	 */
 	if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS32) {
-		const char *shstrtab, *name;
-		Elf32_Shdr shdr;
+		Elf32_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
-
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
 
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
-
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
-	}
-	else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
-		const char *shstrtab, *name;
-		Elf64_Shdr shdr;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
+	} else if (e32_hdr.e_ident[EI_CLASS] == ELFCLASS64) {
+		Elf64_Phdr phdr;
 		int i;
-		void *strtab;
-
-		fseek(f_in, shoff, SEEK_SET);
 
-		/* First find the strtab section */
-		fseek(f_in, shstrndx * shentsize, SEEK_CUR);
-		fread(&shdr, sizeof(shdr), 1, f_in);
-
-		strtab = malloc(shdr.sh_size);
-		if (!strtab) {
-			fprintf(stderr, "Failed to malloc strtab\n");
-			exit(EXIT_FAILURE);
-		}
+		/* Find the first LOAD program header */
+		for (i = 0; i < phnum; i++) {
+			fseek(f_in, phoff + i * phentsize, SEEK_SET);
+			fread(&phdr, sizeof(phdr), 1, f_in);
 
-		fseek(f_in, shdr.sh_offset, SEEK_SET);
-		fread(strtab, shdr.sh_size, 1, f_in);
-
-		/* Now search for the .bss section */
-		fseek(f_in, shoff, SEEK_SET);
-		for (i = 0; i < shnum; i++) {
-			rv = fread(&shdr, sizeof(shdr), 1, f_in);
-			if (!rv) {
-				fprintf(stderr, "Failed to read section table\n");
-				exit(EXIT_FAILURE);
-			}
-
-			name = strtab + shdr.sh_name;
-			if (!strcmp(name, ".bss"))
+			if (phdr.p_type == PT_LOAD)
 				break;
 		}
 
-		if (i == shnum) {
-			fprintf(stderr, "Failed to find .bss section\n");
-			exit(EXIT_FAILURE);
-		}
-
-		datasz = shdr.sh_offset;
+		datasz = phdr.p_filesz;
+		memsz = phdr.p_memsz;
 	}
 
 	buf = malloc(datasz);
@@ -350,7 +291,7 @@ int main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	}
 
-	write_header(f_out, entry, datasz, st.st_size, class);
+	write_header(f_out, entry, datasz, memsz, class);
 
 	/* Write out the entire ELF shared object */
 	rewind(f_in);
@@ -361,5 +302,8 @@ int main(int argc, char **argv)
 	}
 
 	fwrite(buf, datasz, rv, f_out);
+	free(buf);
+	fclose(f_out);
+	fclose(f_in);
 	return 0;
 }
diff --git a/efi/x86_64/syslinux.ld b/efi/x86_64/syslinux.ld
index 95160bd..450641c 100644
--- a/efi/x86_64/syslinux.ld
+++ b/efi/x86_64/syslinux.ld
@@ -136,7 +136,7 @@ SECTIONS
 		*(.strtab)
 	}
 
-	.bss : {
+	.bss (NOLOAD) : {
 		/* the EFI loader doesn't seem to like a .bss section,
 		   so we stick it all into .data: */
 		__bss_start = .;
@@ -153,7 +153,7 @@ SECTIONS
 	__bss_dwords = (__bss_len + 3) >> 2;
 
 	. = ALIGN(128);
-	
+
 	/* Very large objects which don't need to be zeroed */
 
 	.hugebss : {
-- 
1.8.5.2

There is no need to have a relocation section that nothing points at.
The image is still seen as relocatable as long as the Characteristics of
the FileHeader do not say otherwise.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/wrapper.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/efi/wrapper.c b/efi/wrapper.c
index 2b96665..1988124 100644
--- a/efi/wrapper.c
+++ b/efi/wrapper.c
@@ -48,7 +48,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 {
 	struct optional_hdr o_hdr;
 	struct optional_hdr_pe32p o_hdr_pe32p;
-	struct section t_sec, r_sec;
+	struct section t_sec;
 	struct extra_hdr e_hdr;
 	struct extra_hdr_pe32p e_hdr_pe32p;
 	struct coff_hdr c_hdr;
@@ -80,7 +80,7 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&hdr, sizeof(hdr), 1, f);
 
 	memset(&c_hdr, 0, sizeof(c_hdr));
-	c_hdr.nr_sections = 2;
+	c_hdr.nr_sections = 1;
 	c_hdr.nr_syms = 1;
 	if (class == ELFCLASS32) {
 		c_hdr.arch = IMAGE_FILE_MACHINE_I386;
@@ -142,22 +142,6 @@ static void write_header(FILE *f, __uint32_t entry, size_t
data_size,
 	fwrite(&t_sec, sizeof(t_sec), 1, f);
 
 	/*
-	 * Write our empty reloc section.
-	 */
-	memset(&r_sec, 0, sizeof(r_sec));
-	strcpy((char *)r_sec.name, ".reloc");
-	r_sec.virtual_sz = 0;
-	/* /!\ Attention! Some UEFI implementations require a valid virtual
-	 * address despite the virtual_sz = 0. */
-	r_sec.virtual_address = t_sec.virtual_address;
-	r_sec.raw_data_sz = r_sec.virtual_sz;
-	r_sec.raw_data = r_sec.virtual_address;
-	r_sec.characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
-		IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_MEM_DISCARDABLE |
-		IMAGE_SCN_MEM_READ;
-	fwrite(&r_sec, sizeof(r_sec), 1, f);
-
-	/*
 	 * Add some padding to align the ELF as needed
 	 */
 	if (ftell(f) > t_sec.virtual_address) {
-- 
1.8.5.2