post-process-pe: show section names in warnings/errors and strip COFF symbol tables

Make.defaults

@@ -150,7 +150,7 @@ CFLAGS		= $(FEATUREFLAGS) \
 		  $(INCLUDES) \
 		  $(DEFINES)
 
-POST_PROCESS_PE_FLAGS =
+POST_PROCESS_PE_FLAGS ?= -c -vv
 
 ifneq ($(origin OVERRIDE_SECURITY_POLICY), undefined)
 	DEFINES	+= -DOVERRIDE_SECURITY_POLICY

Makefile

@@ -436,7 +436,7 @@ endif
 		--file-alignment 0x1000 \
 		--section-alignment $(ARCH_SECTION_ALIGNMENT) \
 		$(FORMAT) $< $@
-	./post-process-pe -vv $(POST_PROCESS_PE_FLAGS) $@
+	./post-process-pe $(POST_PROCESS_PE_FLAGS) $@
 
 ifneq ($(origin ENABLE_SHIM_HASH),undefined)
 %.hash : %.efi

include/peimage.h

@@ -828,6 +828,9 @@ typedef struct {
 	EFI_IMAGE_DATA_DIRECTORY *SecDir;
 	UINT64 NumberOfRvaAndSizes;
 	UINT16 DllCharacteristics;
+	UINT64 SymbolTable;
+	UINT16 NumberOfSymbols;
+	EFI_IMAGE_FILE_HEADER *FileHdr;
 	EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr;
 } PE_COFF_LOADER_IMAGE_CONTEXT;
 

mkosi/mkosi.conf

@@ -1,5 +1,5 @@
-[Config]
-MinimumVersion=commit:dec7c3e754810ae6d2382d0b5e4762d7250b0254
+#[Config]
+#MinimumVersion=commit:dec7c3e754810ae6d2382d0b5e4762d7250b0254
 
 [Output]
 RepartDirectories=mkosi.repart

post-process-pe.c

@@ -6,6 +6,7 @@
 
 #define _GNU_SOURCE 1
 
+#include <ctype.h>
 #include <err.h>
 #include <fcntl.h>
 #include <getopt.h>
@@ -44,6 +45,7 @@ static int verbosity;
 
 static bool set_nx_compat = false;
 static bool require_nx_compat = false;
+static bool strip_coff_symbols = false;
 
 typedef uint8_t UINT8;
 typedef uint16_t UINT16;
@@ -164,6 +166,7 @@ load_pe(const char *const file, void *const data, const size_t datasize,
 			PEHdr->Pe32Plus.OptionalHeader.SectionAlignment;
 		ctx->DllCharacteristics = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics;
 		ctx->FileAlignment = PEHdr->Pe32Plus.OptionalHeader.FileAlignment;
+		ctx->FileHdr = &PEHdr->Pe32Plus.FileHeader;
 		OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER64);
 	} else {
 		debug(NOISE, "image is 32bit\n");
@@ -175,6 +178,7 @@ load_pe(const char *const file, void *const data, const size_t datasize,
 			PEHdr->Pe32.OptionalHeader.SectionAlignment;
 		ctx->DllCharacteristics = PEHdr->Pe32.OptionalHeader.DllCharacteristics;
 		ctx->FileAlignment = PEHdr->Pe32.OptionalHeader.FileAlignment;
+		ctx->FileHdr = &PEHdr->Pe32.FileHeader;
 		OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER32);
 	}
 
@@ -190,6 +194,9 @@ load_pe(const char *const file, void *const data, const size_t datasize,
 
 	ctx->NumberOfSections = PEHdr->Pe32.FileHeader.NumberOfSections;
 
+	ctx->NumberOfSymbols = PEHdr->Pe32.FileHeader.NumberOfSymbols;
+	ctx->SymbolTable = PEHdr->Pe32.FileHeader.PointerToSymbolTable;
+
 	debug(NOISE,
 	      "Number of RVAs:%"PRIu64" EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES:%d\n",
 	      ctx->NumberOfRvaAndSizes, EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES);
@@ -292,15 +299,15 @@ load_pe(const char *const file, void *const data, const size_t datasize,
 		errx(1, "%s: Unsupported image - Relocations have been stripped", file);
 
 	if (image_is_64_bit(PEHdr)) {
-		ctx->ImageAddress = PEHdr->Pe32Plus.OptionalHeader.ImageBase;
+		ctx->ImageAddress = (uintptr_t)data + PEHdr->Pe32Plus.OptionalHeader.ImageBase;
 		ctx->EntryPoint =
 			PEHdr->Pe32Plus.OptionalHeader.AddressOfEntryPoint;
 		ctx->RelocDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory
 		                         [EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
 		ctx->SecDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory
 		                       [EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
 	} else {
-		ctx->ImageAddress = PEHdr->Pe32.OptionalHeader.ImageBase;
+		ctx->ImageAddress = (uintptr_t)data + PEHdr->Pe32.OptionalHeader.ImageBase;
 		ctx->EntryPoint =
 			PEHdr->Pe32.OptionalHeader.AddressOfEntryPoint;
 		ctx->RelocDir = &PEHdr->Pe32.OptionalHeader.DataDirectory
@@ -363,6 +370,48 @@ set_dll_characteristics(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 	ctx->DllCharacteristics = newflags;
 }
 
+static int
+get_section_name_offset(UINT8 section_name[8], uint32_t *section_name_offset)
+{
+	if (section_name[0] != '/')
+		return -1;
+	for (size_t i = 1; i < 8 && section_name[i] != '\0'; i++) {
+		if (!isdigit(section_name[i]))
+			return -1;
+	}
+
+	*section_name_offset = strtoull((char *)&section_name[1], NULL, 10);
+	return 0;
+}
+
+static void
+get_section_name(PE_COFF_LOADER_IMAGE_CONTEXT *ctx,
+		 EFI_IMAGE_SECTION_HEADER *Section,
+		 char **section_name)
+{
+	const uint32_t * const strtab_size = (uint32_t *)((uintptr_t)ctx->ImageAddress +
+							  (uintptr_t)ctx->SymbolTable +
+							  (ctx->NumberOfSymbols * EFI_IMAGE_SIZEOF_SYMBOL));
+	const char * const strtab = (char *)strtab_size;
+	uint32_t section_name_offset;
+	int rc;
+	char tmpname[9], *newname;
+
+	rc = get_section_name_offset(Section->Name, &section_name_offset);
+	if (rc < 0) {
+		memcpy(tmpname, (char *)&Section->Name[0], 8);
+		tmpname[8] = '\0';
+		newname = strdup(tmpname);
+		if (newname)
+			*section_name = newname;
+		return;
+	}
+
+	*section_name = strdup(&strtab[section_name_offset]);
+	if (!*section_name)
+		err(5, "Couldn't allocate section name");
+}
+
 static int
 validate_nx_compat(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 {
@@ -400,28 +449,109 @@ validate_nx_compat(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 
 	Section = ctx->FirstSection;
 	for (i=0, Section = ctx->FirstSection; i < ctx->NumberOfSections; i++, Section++) {
-		debug(NOISE, "Section %d has WRITE=%d and EXECUTE=%d\n", i,
+		char *section_name = NULL;
+		get_section_name(ctx, Section, &section_name);
+		debug(NOISE, "Section %d is \"%s\"\n", i, section_name);
+		debug(NOISE, "Section %d (\"%s\") has WRITE=%d and EXECUTE=%d\n", i,
+		      section_name,
 		      (Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) ? 1 : 0,
 		      (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) ? 1 : 0);
 
 		if ((Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) &&
 		    (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)) {
-			debug(level, "Section %d is writable and executable\n", i);
+			debug(level, "Section %d (\"%s\") is writable and executable\n", i,
+			      section_name);
 			if (require_nx_compat)
 				ret = -1;
 		}
 
-		debug(NOISE, "Section %d has VA of 0x%08x\n", i, Section->VirtualAddress);
+		debug(NOISE, "Section %d (\"%s\") has VA of 0x%08x\n", i, section_name,
+		      Section->VirtualAddress);
 		if (Section->VirtualAddress != 0 &&
 		    ((Section->VirtualAddress) & (ctx->SectionAlignment - 1))) {
-			debug(level, "Section %d has Virtual Address 0x%08x that isn't section aligned (0x%08x)\n",
-			      i, Section->VirtualAddress, ctx->SectionAlignment);
+			debug(level, "Section %d (\"%s\") has Virtual Address 0x%08x that isn't section aligned (0x%08x)\n",
+			      i, section_name, Section->VirtualAddress, ctx->SectionAlignment);
 		}
+		free(section_name);
 	}
 
 	return ret;
 }
 
+static int
+strip_coff_syms(PE_COFF_LOADER_IMAGE_CONTEXT *ctx, size_t *file_size)
+{
+	EFI_IMAGE_SECTION_HEADER *Section;
+	int i;
+
+	uint16_t pos = 0;
+	char *new_strtab = NULL;
+	uint32_t new_strtab_size = 0;
+
+	uint32_t old_symtab_size = ctx->NumberOfSymbols * EFI_IMAGE_SIZEOF_SYMBOL;
+	uint32_t old_strtab_size = 0;
+	uintptr_t old_strtab_location = 0;
+
+	/*
+	 * On arches where we don't have objcopy support for efi-app-$arch,
+	 * and are thus using -O binary with hacks galore to build our
+	 * binaries, there isn't a coff symbol table.
+	 *
+	 * We also don't have indirect symbol names for our sections, which
+	 * nobody has ever noticed because "objdump -h" doesn't work.
+	 *
+	 * Just call it done on those architectures.
+	 */
+	if (ctx->SymbolTable == 0) {
+		debug(INFO, "This binary has no COFF symbol or string table; skipping.\n");
+		return 0;
+	}
+
+	char *old_symtab = (void *)((uintptr_t)ctx->ImageAddress + ctx->SymbolTable);
+
+	Section = ctx->FirstSection;
+	for (i=0, Section = ctx->FirstSection; i < ctx->NumberOfSections; i++, Section++) {
+		char *section_name = "";
+		char *tmp;
+		size_t len;
+
+		get_section_name(ctx, Section, &section_name);
+
+		len = strlen(section_name);
+
+		if (len > 8 || section_name[0] == '/') {
+			char new_name[9] = { 0, 0, 0, 0, 0, 0, 0, 0, 0 };
+
+			snprintf(new_name, 9, "/%hu", pos+4);
+			new_strtab_size += len + 1;
+
+			tmp = realloc(new_strtab, new_strtab_size);
+			if (!tmp)
+				err(5, "Could not allocate new string table");
+			strcpy(&tmp[pos], section_name);
+			pos += len + 1;
+
+			new_strtab = tmp;
+			memcpy(Section->Name, new_name, 8);
+		}
+		free(section_name);
+	}
+
+	old_strtab_location = (uintptr_t)old_symtab + old_symtab_size;
+	old_strtab_size = *(uint32_t *)old_strtab_location;
+
+	debug(INFO, "Old COFF symtab:%"PRIu32" bytes strtab:%"PRIu32" bytes. New strtab:%"PRIu32" bytes.\n",
+	      old_symtab_size, old_strtab_size, new_strtab_size);
+	ctx->NumberOfSymbols = 0;
+	ctx->FileHdr->NumberOfSymbols = 0;
+	*(uint32_t *)old_symtab = new_strtab_size;
+	memcpy(&((char *)old_symtab)[4], new_strtab, new_strtab_size);
+	*file_size -= old_strtab_size + old_symtab_size;
+	*file_size += sizeof(new_strtab_size) + new_strtab_size;
+
+	return 0;
+}
+
 static void
 fix_timestamp(PE_COFF_LOADER_IMAGE_CONTEXT *ctx)
 {
@@ -515,6 +645,12 @@ handle_one(char *f)
 	if (rc < 0)
 		err(2, "NX compatibility check failed\n");
 
+	if (strip_coff_symbols) {
+		rc = strip_coff_syms(&ctx, &sz);
+		if (rc < 0)
+			err(3, "Stripping COFF symbols failed\n");
+	}
+
 	fix_timestamp(&ctx);
 
 	fix_checksum(&ctx, map, sz);
@@ -529,6 +665,13 @@ handle_one(char *f)
 		warn("munmap(%p, %zu) failed", map, sz);
 		failed = 1;
 	}
+
+	rc = ftruncate(fd, sz);
+	if (rc < 0) {
+		warn("ftruncte(%d, %zu) failed", fd, sz);
+		failed = 1;
+	}
+
 	rc = close(fd);
 	if (rc < 0) {
 		warn("close(%d) failed", fd);
@@ -547,6 +690,8 @@ static void __attribute__((__noreturn__)) usage(int status)
 	fprintf(out, "Options:\n");
 	fprintf(out, "       -q    Be more quiet\n");
 	fprintf(out, "       -v    Be more verbose\n");
+	fprintf(out, "       -C    Disable stripping COFF symbols\n");
+	fprintf(out, "       -c    Enable stripping COFF symbols\n");
 	fprintf(out, "       -N    Disable the NX compatibility flag\n");
 	fprintf(out, "       -n    Enable the NX compatibility flag\n");
 	fprintf(out, "       -x    Error on NX incompatibility\n");
@@ -565,6 +710,12 @@ int main(int argc, char **argv)
 		{.name = "usage",
 		 .val = '?',
 		 },
+		{.name = "strip-coff",
+		 .val = 'C',
+		},
+		{.name = "no-strip-coff",
+		 .val = 'c',
+		},
 		{.name = "disable-nx-compat",
 		 .val = 'N',
 		},
@@ -584,12 +735,18 @@ int main(int argc, char **argv)
 	};
 	int longindex = -1;
 
-	while ((i = getopt_long(argc, argv, "hNnqvx", options, &longindex)) != -1) {
+	while ((i = getopt_long(argc, argv, "hCcNnqvx", options, &longindex)) != -1) {
 		switch (i) {
 		case 'h':
 		case '?':
 			usage(longindex == -1 ? 1 : 0);
 			break;
+		case 'C':
+			strip_coff_symbols = false;
+			break;
+		case 'c':
+			strip_coff_symbols = true;
+			break;
 		case 'N':
 			set_nx_compat = false;
 			break;