From 05875f3aed1c90fe071c66de05744ca2bcbc2b9e Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Thu, 13 May 2021 20:42:18 -0400 Subject: Post-process our PE to be sure. On some versions of binutils[0], including binutils-2.23.52.0.1-55.el7, do not correctly initialize the data when computing the PE optional header checksum. Unfortunately, this means that any time you get a build that reproduces correctly using the version of objcopy from those versions, it's just a matter of luck. This patch introduces a new utility program, post-process-pe, which does some basic validation of the resulting binaries, and if necessary, performs some minor repairs: - sets the timestamp to 0 - this was previously done with dd using constant offsets that aren't really safe. - re-computes the checksum. [0] I suspect, but have not yet fully verified, that this is accidentally fixed by the following upstream binutils commit: commit cf7a3c01d82abdf110ef85ab770e5997d8ac28ac Author: Alan Modra Date: Tue Dec 15 22:09:30 2020 +1030 Lose some COFF/PE static vars, and peicode.h constify This patch tidies some COFF and PE code that unnecessarily used static variables to communicate between functions. v2 - MAP_PRIVATE was totally wrong... Signed-off-by: Peter Jones --- post-process-pe.c | 501 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 501 insertions(+) create mode 100644 post-process-pe.c (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c new file mode 100644 index 00000000..8414a5fa --- /dev/null +++ b/post-process-pe.c @@ -0,0 +1,501 @@ +// SPDX-License-Identifier: BSD-2-Clause-Patent +/* + * post-process-pe.c - fix up timestamps and checksums in broken PE files + * Copyright Peter Jones + */ + +#define _GNU_SOURCE 1 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + +static int verbosity; +#define ERROR 0 +#define WARNING 1 +#define INFO 2 +#define NOISE 3 +#define MIN_VERBOSITY ERROR +#define MAX_VERBOSITY NOISE +#define debug(level, ...) \ + ({ \ + if (verbosity >= (level)) { \ + printf("%s():%d: ", __func__, __LINE__); \ + printf(__VA_ARGS__); \ + } \ + 0; \ + }) + +typedef uint8_t UINT8; +typedef uint16_t UINT16; +typedef uint32_t UINT32; +typedef uint64_t UINT64; + +typedef uint16_t CHAR16; + +typedef unsigned long UINTN; + +typedef struct { + UINT32 Data1; + UINT16 Data2; + UINT16 Data3; + UINT8 Data4[8]; +} EFI_GUID; + +#include "include/peimage.h" + +#if defined(__GNUC__) && defined(__GNUC_MINOR__) +#define GNUC_PREREQ(maj, min) \ + ((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min)) +#else +#define GNUC_PREREQ(maj, min) 0 +#endif + +#if defined(__clang__) && defined(__clang_major__) && defined(__clang_minor__) +#define CLANG_PREREQ(maj, min) \ + ((__clang_major__ > (maj)) || \ + (__clang_major__ == (maj) && __clang_minor__ >= (min))) +#else +#define CLANG_PREREQ(maj, min) 0 +#endif + +#if GNUC_PREREQ(5, 1) || CLANG_PREREQ(3, 8) +#define add(a0, a1, s) __builtin_add_overflow(a0, a1, s) +#define sub(s0, s1, d) __builtin_sub_overflow(s0, s1, d) +#define mul(f0, f1, p) __builtin_mul_overflow(f0, f1, p) +#else +#define add(a0, a1, s) \ + ({ \ + (*s) = ((a0) + (a1)); \ + 0; \ + }) +#define sub(s0, s1, d) \ + ({ \ + (*d) = ((s0) - (s1)); \ + 0; \ + }) +#define mul(f0, f1, p) \ + ({ \ + (*p) = ((f0) * (f1)); \ + 0; \ + }) +#endif +#define div(d0, d1, q) \ + ({ \ + unsigned int ret_ = ((d1) == 0); \ + if (ret_ == 0) \ + (*q) = ((d0) / (d1)); \ + ret_; \ + }) + +static int +image_is_64_bit(EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr) +{ + /* .Magic is the same offset in all cases */ + if (PEHdr->Pe32Plus.OptionalHeader.Magic == + EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC) + return 1; + return 0; +} + +static void +load_pe(const char *const file, void *const data, const size_t datasize, + PE_COFF_LOADER_IMAGE_CONTEXT *ctx) +{ + EFI_IMAGE_DOS_HEADER *DOSHdr = data; + EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr = data; + size_t HeaderWithoutDataDir, SectionHeaderOffset, OptHeaderSize; + size_t FileAlignment = 0; + size_t sz0 = 0, sz1 = 0; + uintptr_t loc = 0; + + debug(NOISE, "datasize:%zu sizeof(PEHdr->Pe32):%zu\n", datasize, + sizeof(PEHdr->Pe32)); + if (datasize < sizeof(PEHdr->Pe32)) + errx(1, "%s: Invalid image size %zu (%zu < %zu)", file, + datasize, datasize, sizeof(PEHdr->Pe32)); + + debug(NOISE, + "DOSHdr->e_magic:0x%02hx EFI_IMAGE_DOS_SIGNATURE:0x%02hx\n", + DOSHdr->e_magic, EFI_IMAGE_DOS_SIGNATURE); + if (DOSHdr->e_magic != EFI_IMAGE_DOS_SIGNATURE) + errx(1, + "%s: Invalid DOS header signature 0x%04hx (expected 0x%04hx)", + file, DOSHdr->e_magic, EFI_IMAGE_DOS_SIGNATURE); + + debug(NOISE, "DOSHdr->e_lfanew:%u datasize:%zu\n", DOSHdr->e_lfanew, + datasize); + if (DOSHdr->e_lfanew >= datasize || + add((uintptr_t)data, DOSHdr->e_lfanew, &loc)) + errx(1, "%s: invalid pe header location", file); + + ctx->PEHdr = PEHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)loc; + debug(NOISE, "PE signature:0x%04x EFI_IMAGE_NT_SIGNATURE:0x%04x\n", + PEHdr->Pe32.Signature, EFI_IMAGE_NT_SIGNATURE); + if (PEHdr->Pe32.Signature != EFI_IMAGE_NT_SIGNATURE) + errx(1, "%s: Unsupported image type", file); + + if (image_is_64_bit(PEHdr)) { + debug(NOISE, "image is 64bit\n"); + ctx->NumberOfRvaAndSizes = + PEHdr->Pe32Plus.OptionalHeader.NumberOfRvaAndSizes; + ctx->SizeOfHeaders = + PEHdr->Pe32Plus.OptionalHeader.SizeOfHeaders; + ctx->ImageSize = PEHdr->Pe32Plus.OptionalHeader.SizeOfImage; + ctx->SectionAlignment = + PEHdr->Pe32Plus.OptionalHeader.SectionAlignment; + FileAlignment = PEHdr->Pe32Plus.OptionalHeader.FileAlignment; + OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER64); + } else { + debug(NOISE, "image is 32bit\n"); + ctx->NumberOfRvaAndSizes = + PEHdr->Pe32.OptionalHeader.NumberOfRvaAndSizes; + ctx->SizeOfHeaders = PEHdr->Pe32.OptionalHeader.SizeOfHeaders; + ctx->ImageSize = (UINT64)PEHdr->Pe32.OptionalHeader.SizeOfImage; + ctx->SectionAlignment = + PEHdr->Pe32.OptionalHeader.SectionAlignment; + FileAlignment = PEHdr->Pe32.OptionalHeader.FileAlignment; + OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER32); + } + + if (FileAlignment % 2 != 0) + errx(1, "%s: Invalid file alignment %ld", file, FileAlignment); + + if (FileAlignment == 0) + FileAlignment = 0x200; + if (ctx->SectionAlignment == 0) + ctx->SectionAlignment = PAGE_SIZE; + if (ctx->SectionAlignment < FileAlignment) + ctx->SectionAlignment = FileAlignment; + + ctx->NumberOfSections = PEHdr->Pe32.FileHeader.NumberOfSections; + + debug(NOISE, + "Number of RVAs:%"PRIu64" EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES:%d\n", + ctx->NumberOfRvaAndSizes, EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES); + if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < ctx->NumberOfRvaAndSizes) + errx(1, "%s: invalid number of RVAs (%lu entries, max is %d)", + file, ctx->NumberOfRvaAndSizes, + EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES); + + if (mul(sizeof(EFI_IMAGE_DATA_DIRECTORY), + EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES, &sz0) || + sub(OptHeaderSize, sz0, &HeaderWithoutDataDir) || + sub(PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + HeaderWithoutDataDir, &sz0) || + mul(ctx->NumberOfRvaAndSizes, sizeof(EFI_IMAGE_DATA_DIRECTORY), + &sz1) || + (sz0 != sz1)) { + if (mul(sizeof(EFI_IMAGE_DATA_DIRECTORY), + EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES, &sz0)) + debug(ERROR, + "sizeof(EFI_IMAGE_DATA_DIRECTORY) * EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES overflows\n"); + else + debug(ERROR, + "sizeof(EFI_IMAGE_DATA_DIRECTORY) * EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES = %zu\n", + sz0); + if (sub(OptHeaderSize, sz0, &HeaderWithoutDataDir)) + debug(ERROR, + "OptHeaderSize (%zu) - HeaderWithoutDataDir (%zu) overflows\n", + OptHeaderSize, HeaderWithoutDataDir); + else + debug(ERROR, + "OptHeaderSize (%zu) - HeaderWithoutDataDir (%zu) = %zu\n", + OptHeaderSize, sz0, HeaderWithoutDataDir); + + if (sub(PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + HeaderWithoutDataDir, &sz0)) { + debug(ERROR, + "PEHdr->Pe32.FileHeader.SizeOfOptionalHeader (%d) - %zu overflows\n", + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + HeaderWithoutDataDir); + } else { + debug(ERROR, + "PEHdr->Pe32.FileHeader.SizeOfOptionalHeader (%d)- %zu = %zu\n", + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + HeaderWithoutDataDir, sz0); + } + if (mul(ctx->NumberOfRvaAndSizes, + sizeof(EFI_IMAGE_DATA_DIRECTORY), &sz1)) + debug(ERROR, + "ctx->NumberOfRvaAndSizes (%zu) * sizeof(EFI_IMAGE_DATA_DIRECTORY) overflows\n", + ctx->NumberOfRvaAndSizes); + else + debug(ERROR, + "ctx->NumberOfRvaAndSizes (%zu) * sizeof(EFI_IMAGE_DATA_DIRECTORY) = %zu\n", + ctx->NumberOfRvaAndSizes, sz1); + debug(ERROR, + "space after image header:%zu data directory size:%zu\n", + sz0, sz1); + + errx(1, "%s: image header overflows data directory", file); + } + + if (add(DOSHdr->e_lfanew, sizeof(UINT32), &SectionHeaderOffset) || + add(SectionHeaderOffset, sizeof(EFI_IMAGE_FILE_HEADER), + &SectionHeaderOffset) || + add(SectionHeaderOffset, + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + &SectionHeaderOffset)) { + debug(ERROR, "SectionHeaderOffset:%" PRIu32 " + %zu + %zu + %d", + DOSHdr->e_lfanew, sizeof(UINT32), + sizeof(EFI_IMAGE_FILE_HEADER), + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader); + errx(1, "%s: SectionHeaderOffset would overflow", file); + } + + if (sub(ctx->ImageSize, SectionHeaderOffset, &sz0) || + div(sz0, EFI_IMAGE_SIZEOF_SECTION_HEADER, &sz0) || + (sz0 <= ctx->NumberOfSections)) { + debug(ERROR, "(%" PRIu64 " - %zu) / %d > %d\n", ctx->ImageSize, + SectionHeaderOffset, EFI_IMAGE_SIZEOF_SECTION_HEADER, + ctx->NumberOfSections); + errx(1, "%s: image sections overflow image size", file); + } + + if (sub(ctx->SizeOfHeaders, SectionHeaderOffset, &sz0) || + div(sz0, EFI_IMAGE_SIZEOF_SECTION_HEADER, &sz0) || + (sz0 < ctx->NumberOfSections)) { + debug(ERROR, "(%zu - %zu) / %d >= %d\n", ctx->SizeOfHeaders, + SectionHeaderOffset, EFI_IMAGE_SIZEOF_SECTION_HEADER, + ctx->NumberOfSections); + errx(1, "%s: image sections overflow section headers", file); + } + + if (sub((uintptr_t)PEHdr, (uintptr_t)data, &sz0) || + add(sz0, sizeof(EFI_IMAGE_OPTIONAL_HEADER_UNION), &sz0) || + (sz0 > datasize)) { + errx(1, "%s: PE Image size %zu > %zu", file, sz0, datasize); + } + + if (PEHdr->Pe32.FileHeader.Characteristics & EFI_IMAGE_FILE_RELOCS_STRIPPED) + errx(1, "%s: Unsupported image - Relocations have been stripped", file); + + if (image_is_64_bit(PEHdr)) { + ctx->ImageAddress = 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->EntryPoint = + PEHdr->Pe32.OptionalHeader.AddressOfEntryPoint; + ctx->RelocDir = &PEHdr->Pe32.OptionalHeader.DataDirectory + [EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC]; + ctx->SecDir = &PEHdr->Pe32.OptionalHeader.DataDirectory + [EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]; + } + + if (add((uintptr_t)PEHdr, PEHdr->Pe32.FileHeader.SizeOfOptionalHeader, + &loc) || + add(loc, sizeof(UINT32), &loc) || + add(loc, sizeof(EFI_IMAGE_FILE_HEADER), &loc)) + errx(1, "%s: invalid location for first section", file); + + ctx->FirstSection = (EFI_IMAGE_SECTION_HEADER *)loc; + + if (ctx->ImageSize < ctx->SizeOfHeaders) + errx(1, + "%s: Image size %"PRIu64" is smaller than header size %lu", + file, ctx->ImageSize, ctx->SizeOfHeaders); + + if (sub((uintptr_t)ctx->SecDir, (uintptr_t)data, &sz0) || + sub(datasize, sizeof(EFI_IMAGE_DATA_DIRECTORY), &sz1) || + sz0 > sz1) + errx(1, + "%s: security direcory offset %zu past data directory at %zu", + file, sz0, sz1); + + if (ctx->SecDir->VirtualAddress > datasize || + (ctx->SecDir->VirtualAddress == datasize && + ctx->SecDir->Size > 0)) + errx(1, "%s: Security directory extends past end", file); +} + +static void +fix_timestamp(PE_COFF_LOADER_IMAGE_CONTEXT *ctx) +{ + uint32_t ts; + + if (image_is_64_bit(ctx->PEHdr)) { + ts = ctx->PEHdr->Pe32Plus.FileHeader.TimeDateStamp; + } else { + ts = ctx->PEHdr->Pe32.FileHeader.TimeDateStamp; + } + + if (ts != 0) { + debug(INFO, "Updating timestamp from 0x%08x to 0\n", ts); + if (image_is_64_bit(ctx->PEHdr)) { + ctx->PEHdr->Pe32Plus.FileHeader.TimeDateStamp = 0; + } else { + ctx->PEHdr->Pe32.FileHeader.TimeDateStamp = 0; + } + } +} + +static void +fix_checksum(PE_COFF_LOADER_IMAGE_CONTEXT *ctx, void *map, size_t mapsize) +{ + uint32_t old; + uint32_t checksum = 0; + uint16_t word; + uint8_t *data = map; + + if (image_is_64_bit(ctx->PEHdr)) { + old = ctx->PEHdr->Pe32Plus.OptionalHeader.CheckSum; + ctx->PEHdr->Pe32Plus.OptionalHeader.CheckSum = 0; + } else { + old = ctx->PEHdr->Pe32.OptionalHeader.CheckSum; + ctx->PEHdr->Pe32.OptionalHeader.CheckSum = 0; + } + debug(NOISE, "old checksum was 0x%08x\n", old); + + for (size_t i = 0; i < mapsize - 1; i += 2) { + word = (data[i + 1] << 8ul) | data[i]; + checksum += word; + checksum = 0xffff & (checksum + (checksum >> 0x10)); + } + debug(NOISE, "checksum = 0x%08x + 0x%08zx = 0x%08zx\n", checksum, + mapsize, checksum + mapsize); + + checksum += mapsize; + + if (checksum != old) + debug(INFO, "Updating checksum from 0x%08x to 0x%08x\n", + old, checksum); + + if (image_is_64_bit(ctx->PEHdr)) { + ctx->PEHdr->Pe32Plus.OptionalHeader.CheckSum = checksum; + } else { + ctx->PEHdr->Pe32.OptionalHeader.CheckSum = checksum; + } +} + +static void +handle_one(char *f) +{ + int fd; + int rc; + struct stat statbuf; + size_t sz; + void *map; + int failed = 0; + + PE_COFF_LOADER_IMAGE_CONTEXT ctx = { 0, 0 }; + + fd = open(f, O_RDWR | O_EXCL); + if (fd < 0) + err(1, "Could not open \"%s\"", f); + + rc = fstat(fd, &statbuf); + if (rc < 0) + err(1, "Could not stat \"%s\"", f); + + sz = statbuf.st_size; + + map = mmap(NULL, sz, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + if (map == MAP_FAILED) + err(1, "Could not map \"%s\"", f); + + load_pe(f, map, sz, &ctx); + + fix_timestamp(&ctx); + + fix_checksum(&ctx, map, sz); + + rc = msync(map, sz, MS_SYNC); + if (rc < 0) { + warn("msync(%p, %zu, MS_SYNC) failed", map, sz); + failed = 1; + } + munmap(map, sz); + if (rc < 0) { + warn("munmap(%p, %zu) failed", map, sz); + failed = 1; + } + rc = close(fd); + if (rc < 0) { + warn("close(%d) failed", fd); + failed = 1; + } + if (failed) + exit(1); +} + +static void __attribute__((__noreturn__)) usage(int status) +{ + FILE *out = status ? stderr : stdout; + + fprintf(out, + "Usage: post-process-pe [OPTIONS] file0 [file1 [.. fileN]]\n"); + fprintf(out, "Options:\n"); + fprintf(out, " -q Be more quiet\n"); + fprintf(out, " -v Be more verbose\n"); + fprintf(out, " -h Print this help text and exit\n"); + + exit(status); +} + +int main(int argc, char **argv) +{ + int i; + struct option options[] = { + {.name = "help", + .val = '?', + }, + {.name = "usage", + .val = '?', + }, + {.name = "quiet", + .val = 'q', + }, + {.name = "verbose", + .val = 'v', + }, + {.name = ""} + }; + int longindex = -1; + + while ((i = getopt_long(argc, argv, "hqsv", options, &longindex)) != -1) { + switch (i) { + case 'h': + case '?': + usage(longindex == -1 ? 1 : 0); + break; + case 'q': + verbosity = MAX(verbosity - 1, MIN_VERBOSITY); + break; + case 'v': + verbosity = MIN(verbosity + 1, MAX_VERBOSITY); + break; + } + } + + if (optind == argc) + usage(1); + + for (i = optind; i < argc; i++) + handle_one(argv[i]); + + return 0; +} + +// vim:fenc=utf-8:tw=75:noet -- cgit v1.2.3 From bda03b8454210afca9c3c43ee84902205187ac36 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Tue, 19 Apr 2022 14:44:24 -0400 Subject: post-process-pe: Fix a missing return code check Signed-off-by: Peter Jones --- post-process-pe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c index 8414a5fa..44077bc5 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -426,7 +426,7 @@ handle_one(char *f) warn("msync(%p, %zu, MS_SYNC) failed", map, sz); failed = 1; } - munmap(map, sz); + rc = munmap(map, sz); if (rc < 0) { warn("munmap(%p, %zu) failed", map, sz); failed = 1; -- cgit v1.2.3 From b5185cbddd8cc678414d3fddcbbcff366444b21a Mon Sep 17 00:00:00 2001 From: Steve McIntyre Date: Thu, 28 Apr 2022 11:37:12 +0100 Subject: post-process-pe: Fix format string warnings on 32-bit platforms With -Werror these were causing build failures with stricter gcc: .../post-process-pe.c: In function 'load_pe': .../post-process-pe.c:177:55: error: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Werror=format=] .../post-process-pe.c:192:56: error: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'UINT64' {aka 'long long unsigned int'} [-Werror=format=] .../post-process-pe.c:236:31: error: format '%zu' expects argument of type 'size_t', but argument 2 has type 'UINT64' {aka 'long long unsigned int'} [-Werror=format=] .../post-process-pe.c:39:32: note: in definition of macro 'debug' .../post-process-pe.c:236:60: note: format string is defined here .../post-process-pe.c:240:31: error: format '%zu' expects argument of type 'size_t', but argument 2 has type 'UINT64' {aka 'long long unsigned int'} [-Werror=format=] .../post-process-pe.c:39:32: note: in definition of macro 'debug' .../post-process-pe.c:240:60: note: format string is defined here .../post-process-pe.c:274:30: error: format '%zu' expects argument of type 'size_t', but argument 2 has type 'UINTN' {aka 'long unsigned int'} [-Werror=format=] .../post-process-pe.c:39:32: note: in definition of macro 'debug' .../post-process-pe.c:274:34: note: format string is defined here Signed-off-by: Steve McIntyre --- post-process-pe.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c index 44077bc5..daacf5ef 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -174,7 +174,7 @@ load_pe(const char *const file, void *const data, const size_t datasize, } if (FileAlignment % 2 != 0) - errx(1, "%s: Invalid file alignment %ld", file, FileAlignment); + errx(1, "%s: Invalid file alignment %zu", file, FileAlignment); if (FileAlignment == 0) FileAlignment = 0x200; @@ -190,7 +190,7 @@ load_pe(const char *const file, void *const data, const size_t datasize, ctx->NumberOfRvaAndSizes, EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES); if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < ctx->NumberOfRvaAndSizes) errx(1, "%s: invalid number of RVAs (%lu entries, max is %d)", - file, ctx->NumberOfRvaAndSizes, + file, (unsigned long)ctx->NumberOfRvaAndSizes, EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES); if (mul(sizeof(EFI_IMAGE_DATA_DIRECTORY), @@ -233,12 +233,12 @@ load_pe(const char *const file, void *const data, const size_t datasize, if (mul(ctx->NumberOfRvaAndSizes, sizeof(EFI_IMAGE_DATA_DIRECTORY), &sz1)) debug(ERROR, - "ctx->NumberOfRvaAndSizes (%zu) * sizeof(EFI_IMAGE_DATA_DIRECTORY) overflows\n", - ctx->NumberOfRvaAndSizes); + "ctx->NumberOfRvaAndSizes (%ld) * sizeof(EFI_IMAGE_DATA_DIRECTORY) overflows\n", + (unsigned long)ctx->NumberOfRvaAndSizes); else debug(ERROR, - "ctx->NumberOfRvaAndSizes (%zu) * sizeof(EFI_IMAGE_DATA_DIRECTORY) = %zu\n", - ctx->NumberOfRvaAndSizes, sz1); + "ctx->NumberOfRvaAndSizes (%ld) * sizeof(EFI_IMAGE_DATA_DIRECTORY) = %zu\n", + (unsigned long)ctx->NumberOfRvaAndSizes, sz1); debug(ERROR, "space after image header:%zu data directory size:%zu\n", sz0, sz1); @@ -271,7 +271,7 @@ load_pe(const char *const file, void *const data, const size_t datasize, if (sub(ctx->SizeOfHeaders, SectionHeaderOffset, &sz0) || div(sz0, EFI_IMAGE_SIZEOF_SECTION_HEADER, &sz0) || (sz0 < ctx->NumberOfSections)) { - debug(ERROR, "(%zu - %zu) / %d >= %d\n", ctx->SizeOfHeaders, + debug(ERROR, "(%zu - %zu) / %d >= %d\n", (size_t)ctx->SizeOfHeaders, SectionHeaderOffset, EFI_IMAGE_SIZEOF_SECTION_HEADER, ctx->NumberOfSections); errx(1, "%s: image sections overflow section headers", file); -- cgit v1.2.3 From 8ce2832e3c9edebb4b9e2932f5f1dd79d8feec59 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Thu, 2 Dec 2021 17:14:51 -0500 Subject: post-process-pe: there is no 's' argument. There is no 's' argument to post-process-pe, so we shouldn't tell getopt that there is. This patch takes the 's' out of the getopt short option list. Signed-off-by: Peter Jones --- post-process-pe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c index daacf5ef..e848f009 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -474,7 +474,7 @@ int main(int argc, char **argv) }; int longindex = -1; - while ((i = getopt_long(argc, argv, "hqsv", options, &longindex)) != -1) { + while ((i = getopt_long(argc, argv, "hqv", options, &longindex)) != -1) { switch (i) { case 'h': case '?': -- cgit v1.2.3 From b104fc480aae94eaa8d79717d0b7cf6dcc2253d3 Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Thu, 2 Dec 2021 15:51:00 -0500 Subject: post-process-pe: set EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT Currently, system firmware has no means to discover that an EFI Application is compatible with the security feature variously known as NX or w^x. Since at least Revision 8.1, the PE spec supports setting a flag the Optional Header's DllCharacteristics field to inform loaders that an application supports being loaded with NX enabled. In the case of UEFI, there are several things that should be enabled if this flag is set: - EFI_BOOT_SERVICES.AllocatePages() with MemoryType = EfiLoaderCode, EfiBootServicesCode, EfiRuntimeServicesCode, etc, currently must set memory as rwx. This flag set implies that rw- is appropriate, and that the application knows how to use the EFI_MEMORY_ATTRIBUTE protocol to change that to r-x. - EFI_BOOT_SERVICES.AllocatePool() - same as AllocatePages() - EFI_BOOT_SERVICES.LoadImage() - currently must set the stack as rwx. This flag states that it is allowed to be rw- - currently a binary can probably have writable PLTs? This flag allows the loader to not set them writable - I have heard that some firmwares have the 0 page mapped rwx. Obviously this should not be done. Signed-off-by: Peter Jones --- post-process-pe.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c index e848f009..de8f4a38 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,8 @@ static int verbosity; 0; \ }) +static bool set_nx_compat = false; + typedef uint8_t UINT8; typedef uint16_t UINT16; typedef uint32_t UINT32; @@ -330,6 +333,33 @@ load_pe(const char *const file, void *const data, const size_t datasize, errx(1, "%s: Security directory extends past end", file); } +static void +set_dll_characteristics(PE_COFF_LOADER_IMAGE_CONTEXT *ctx) +{ + uint16_t oldflags, newflags; + + if (image_is_64_bit(ctx->PEHdr)) { + oldflags = ctx->PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics; + } else { + oldflags = ctx->PEHdr->Pe32.OptionalHeader.DllCharacteristics; + } + + if (set_nx_compat) + newflags = oldflags | EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT; + else + newflags = oldflags & ~(uint16_t)EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT; + if (oldflags == newflags) + return; + + debug(INFO, "Updating DLL Characteristics from 0x%04hx to 0x%04hx\n", + oldflags, newflags); + if (image_is_64_bit(ctx->PEHdr)) { + ctx->PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics = newflags; + } else { + ctx->PEHdr->Pe32.OptionalHeader.DllCharacteristics = newflags; + } +} + static void fix_timestamp(PE_COFF_LOADER_IMAGE_CONTEXT *ctx) { @@ -417,6 +447,8 @@ handle_one(char *f) load_pe(f, map, sz, &ctx); + set_dll_characteristics(&ctx); + fix_timestamp(&ctx); fix_checksum(&ctx, map, sz); @@ -449,6 +481,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, " -N Disable the NX compatibility flag\n"); + fprintf(out, " -n Enable the NX compatibility flag\n"); fprintf(out, " -h Print this help text and exit\n"); exit(status); @@ -464,6 +498,12 @@ int main(int argc, char **argv) {.name = "usage", .val = '?', }, + {.name = "disable-nx-compat", + .val = 'N', + }, + {.name = "enable-nx-compat", + .val = 'n', + }, {.name = "quiet", .val = 'q', }, @@ -474,12 +514,18 @@ int main(int argc, char **argv) }; int longindex = -1; - while ((i = getopt_long(argc, argv, "hqv", options, &longindex)) != -1) { + while ((i = getopt_long(argc, argv, "hNnqv", options, &longindex)) != -1) { switch (i) { case 'h': case '?': usage(longindex == -1 ? 1 : 0); break; + case 'N': + set_nx_compat = false; + break; + case 'n': + set_nx_compat = true; + break; case 'q': verbosity = MAX(verbosity - 1, MIN_VERBOSITY); break; -- cgit v1.2.3 From 7c7642530fab73facaf3eac233cfbce29e10b0ef Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Thu, 17 Nov 2022 12:31:31 -0500 Subject: Enable the NX compatibility flag by default. Currently by default, when we build shim we do not set the PE NX-compatibility DLL Characteristic flag. This signifies to the firmware that shim (including the components it loads) is not prepared for several related firmware changes: - non-executable stack - non-executable pages from AllocatePages()/AllocatePool()/etc. - non-writable 0 page (not strictly related but some firmware will be transitioning at the same time) - the need to use the UEFI 2.10 Memory Attribute Protocol to set page permissions. This patch changes that default to be enabled by default. Distributors of shim will need to ensure that either their builds disable this bit (using "post-process-pe -N"), or that the bootloaders and kernels you support loading are all compliant with this change. A new make variable, POST_PROCESS_PE_FLAGS, has been added to simplify doing so. Signed-off-by: Peter Jones --- BUILDING | 3 +++ Make.defaults | 2 ++ Makefile | 2 +- post-process-pe.c | 2 +- 4 files changed, 7 insertions(+), 2 deletions(-) (limited to 'post-process-pe.c') diff --git a/BUILDING b/BUILDING index 3b2e85d3..17cd98d3 100644 --- a/BUILDING +++ b/BUILDING @@ -78,6 +78,9 @@ Variables you could set to customize the build: - OSLABEL This is the label that will be put in BOOT$(EFI_ARCH).CSV for your OS. By default this is the same value as EFIDIR . +- POST_PROCESS_PE_FLAGS + This allows you to add flags to the invocation of "post-process-pe", for + example to disable the NX compatibility flag. Vendor SBAT data: It will sometimes be requested by reviewers that a build includes extra diff --git a/Make.defaults b/Make.defaults index c46164a3..9af89f4e 100644 --- a/Make.defaults +++ b/Make.defaults @@ -139,6 +139,8 @@ CFLAGS = $(FEATUREFLAGS) \ $(INCLUDES) \ $(DEFINES) +POST_PROCESS_PE_FLAGS = + ifneq ($(origin OVERRIDE_SECURITY_POLICY), undefined) DEFINES += -DOVERRIDE_SECURITY_POLICY endif diff --git a/Makefile b/Makefile index a9202f46..f0f53f8f 100644 --- a/Makefile +++ b/Makefile @@ -255,7 +255,7 @@ endif -j .rela* -j .dyn -j .reloc -j .eh_frame \ -j .vendor_cert -j .sbat -j .sbatlevel \ $(FORMAT) $< $@ - ./post-process-pe -vv $@ + ./post-process-pe -vv $(POST_PROCESS_PE_FLAGS) $@ ifneq ($(origin ENABLE_SHIM_HASH),undefined) %.hash : %.efi diff --git a/post-process-pe.c b/post-process-pe.c index de8f4a38..f39fdddf 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -42,7 +42,7 @@ static int verbosity; 0; \ }) -static bool set_nx_compat = false; +static bool set_nx_compat = true; typedef uint8_t UINT8; typedef uint16_t UINT16; -- cgit v1.2.3 From be8ff7c2680fed067cdd76df0afc43138c24cc0d Mon Sep 17 00:00:00 2001 From: Peter Jones Date: Fri, 1 Dec 2023 18:19:38 -0500 Subject: post-process-pe: Don't set the NX_COMPAT flag by default after all. We thought we would fully support NX compatibility in the full stack for this release, but all of the necessary components aren't *quite* ready for this release. This patch switches back the default that was changed in a53b9f7ceec1d, but it leaves the build infrastructure in place. Signed-off-by: Peter Jones --- post-process-pe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'post-process-pe.c') diff --git a/post-process-pe.c b/post-process-pe.c index f39fdddf..de8f4a38 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -42,7 +42,7 @@ static int verbosity; 0; \ }) -static bool set_nx_compat = true; +static bool set_nx_compat = false; typedef uint8_t UINT8; typedef uint16_t UINT16; -- cgit v1.2.3 From 7cde2cc52f19f733de7855419d1c43a13a8d6c5f Mon Sep 17 00:00:00 2001 From: Dennis Tseng Date: Fri, 28 Apr 2023 10:47:33 +0800 Subject: post-process-pe: add tests to validate NX compliance This changes post-process-pe to give warnings, and optionally errors, if a shim binary is built with Section Alignment or characteristics are not compatible with NX, or if the EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT flag is not set and require_nx_compat is true. Co-authored-by: Peter Jones Co-authored-by: Kamil Aronowski Signed-off-by: Dennis Tseng --- include/peimage.h | 1 + post-process-pe.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) (limited to 'post-process-pe.c') diff --git a/include/peimage.h b/include/peimage.h index 6eef1051..4182a17b 100644 --- a/include/peimage.h +++ b/include/peimage.h @@ -824,6 +824,7 @@ typedef struct { EFI_IMAGE_DATA_DIRECTORY *RelocDir; EFI_IMAGE_DATA_DIRECTORY *SecDir; UINT64 NumberOfRvaAndSizes; + UINT16 DllCharacteristics; EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr; } PE_COFF_LOADER_IMAGE_CONTEXT; diff --git a/post-process-pe.c b/post-process-pe.c index de8f4a38..008da93b 100644 --- a/post-process-pe.c +++ b/post-process-pe.c @@ -43,6 +43,7 @@ static int verbosity; }) static bool set_nx_compat = false; +static bool require_nx_compat = false; typedef uint8_t UINT8; typedef uint16_t UINT16; @@ -162,6 +163,7 @@ load_pe(const char *const file, void *const data, const size_t datasize, ctx->ImageSize = PEHdr->Pe32Plus.OptionalHeader.SizeOfImage; ctx->SectionAlignment = PEHdr->Pe32Plus.OptionalHeader.SectionAlignment; + ctx->DllCharacteristics = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics; FileAlignment = PEHdr->Pe32Plus.OptionalHeader.FileAlignment; OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER64); } else { @@ -172,6 +174,7 @@ load_pe(const char *const file, void *const data, const size_t datasize, ctx->ImageSize = (UINT64)PEHdr->Pe32.OptionalHeader.SizeOfImage; ctx->SectionAlignment = PEHdr->Pe32.OptionalHeader.SectionAlignment; + ctx->DllCharacteristics = PEHdr->Pe32.OptionalHeader.DllCharacteristics; FileAlignment = PEHdr->Pe32.OptionalHeader.FileAlignment; OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER32); } @@ -358,6 +361,50 @@ set_dll_characteristics(PE_COFF_LOADER_IMAGE_CONTEXT *ctx) } else { ctx->PEHdr->Pe32.OptionalHeader.DllCharacteristics = newflags; } + ctx->DllCharacteristics = newflags; +} + +static int +validate_nx_compat(PE_COFF_LOADER_IMAGE_CONTEXT *ctx) +{ + EFI_IMAGE_SECTION_HEADER *Section; + int i; + bool nx_compat_flag; + const int level = require_nx_compat ? ERROR : WARNING; + int ret = 0; + + nx_compat_flag = EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT + & ctx->DllCharacteristics; + debug(NOISE, "NX-Compat-Flag: %s\n", nx_compat_flag ? "set" : "unset"); + if (!nx_compat_flag) { + debug(level, "NX Compatibility flag is not set\n"); + if (require_nx_compat) + ret = -1; + } + + debug(NOISE, "Section alignment is 0x%x, page size is 0x%x\n", + ctx->SectionAlignment, PAGE_SIZE); + if (ctx->SectionAlignment != PAGE_SIZE) { + debug(level, "Section alignment is not page aligned\n"); + if (require_nx_compat) + ret = -1; + } + + 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, + (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); + if (require_nx_compat) + ret = -1; + } + } + + return ret; } static void @@ -449,6 +496,10 @@ handle_one(char *f) set_dll_characteristics(&ctx); + rc = validate_nx_compat(&ctx); + if (rc < 0) + err(2, "NX compatibility check failed\n"); + fix_timestamp(&ctx); fix_checksum(&ctx, map, sz); @@ -483,6 +534,7 @@ static void __attribute__((__noreturn__)) usage(int status) fprintf(out, " -v Be more verbose\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"); fprintf(out, " -h Print this help text and exit\n"); exit(status); @@ -510,11 +562,14 @@ int main(int argc, char **argv) {.name = "verbose", .val = 'v', }, + {.name = "error-nx-compat", + .val = 'x', + }, {.name = ""} }; int longindex = -1; - while ((i = getopt_long(argc, argv, "hNnqv", options, &longindex)) != -1) { + while ((i = getopt_long(argc, argv, "hNnqvx", options, &longindex)) != -1) { switch (i) { case 'h': case '?': @@ -532,6 +587,9 @@ int main(int argc, char **argv) case 'v': verbosity = MIN(verbosity + 1, MAX_VERBOSITY); break; + case 'x': + require_nx_compat = true; + break; } } -- cgit v1.2.3