[PATCH 0/2] kexec-tools: Truncate PE file if it is signed

[PATCH 0/2] kexec-tools: Truncate PE file if it is signed

[Pingfan Liu]

Note: This series is compatible with the ongoing kernel patches '[PATCH
0/2] Kexec: Sign Image before packing into EFI STUB' [1]. It ensures
functionality when kexec_file_load enforces signature verification on
arm64, LoongArch, and RISC-V.


*** Issue ***
In the linux kernel drivers/firmware/efi/libstub/Makefile.zboot, the
original Image is padded with zero, using the following instruction:
        truncate -s $$(hexdump -s16 -n4 -e '"%u"' $<) $@

Hence pe-zboot.c decomopresses and gets Image plus trailing zeroes.

These trailing zeroes don't affect loading the original PE file. But
they do raise an issue during the signature verfication. The root cause is
that the kernel function:
        static int pefile_digest_pe_contents(const void *pebuf, unsigned int pelen,
                                             struct pefile_context *ctx,
                                             struct shash_desc *desc)
treats [pebuf, pebuf+pelen] as valid payload, which includes the
trailing zeroes. But that is not the truth.

*** Solution ***
In pratice, the table of attribute certificates come at the end of a
PE file. This patch utilizes that fact and truncates at the boundary of the
certificate table to get the original Image.

[1]: https://lore.kernel.org/linux-efi/20241206021000.8953-1-piliu@redhat.com/T/#t

Cc: Simon Horman <horms@kernel.org>
To: kexec@lists.infradead.org

Pingfan Liu (2):
  PE: Extract get_pehdr_offset() for reuse
  pe-zboot: Truncate the trailing zero if Image is signed

 include/pe.h           | 47 ++++++++++++++++++++++++++++++++++++++++++
 kexec/kexec-pe-zboot.c | 25 +++++++++++++++++++---
 kexec/kexec-uki.c      | 17 ---------------
 3 files changed, 69 insertions(+), 20 deletions(-)

-- 
2.41.0

[PATCH 1/2] PE: Extract get_pehdr_offset() for reuse

[Pingfan Liu]

The get_pehdr_offset() function helps skip the DOS stub in a PE file to
locate the start of the PE header. Since it can be used in multiple C
files, it is being moved to a header file.

Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Simon Horman <horms@kernel.org>
To: kexec@lists.infradead.org
---
 include/pe.h      | 17 +++++++++++++++++
 kexec/kexec-uki.c | 17 -----------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/pe.h b/include/pe.h
index 2617074..1e1c59b 100644
--- a/include/pe.h
+++ b/include/pe.h
@@ -101,4 +101,21 @@ struct section_header {
 	uint32_t flags;
 };
 
+/*
+ * Return -1 if not PE, else offset of the PE header
+ */
+static int get_pehdr_offset(const char *buf)
+{
+	int pe_hdr_offset;
+
+	pe_hdr_offset = *((int *)(buf + 0x3c));
+	buf += pe_hdr_offset;
+	if (!!memcmp(buf, "PE\0\0", 4)) {
+		printf("Not a PE file\n");
+		return -1;
+	}
+
+	return pe_hdr_offset;
+}
+
 #endif
diff --git a/kexec/kexec-uki.c b/kexec/kexec-uki.c
index 235f5a1..9888d7e 100644
--- a/kexec/kexec-uki.c
+++ b/kexec/kexec-uki.c
@@ -20,23 +20,6 @@
 static int embeded_linux_format_index = -1;
 static int kernel_fd = -1;
 
-/*
- * Return -1 if not PE, else offset of the PE header
- */
-static int get_pehdr_offset(const char *buf)
-{
-	int pe_hdr_offset;
-
-	pe_hdr_offset = *((int *)(buf + 0x3c));
-	buf += pe_hdr_offset;
-	if (!!memcmp(buf, "PE\0\0", 4)) {
-		printf("Not a PE file\n");
-		return -1;
-	}
-
-	return pe_hdr_offset;
-}
-
 static int create_tmpfd(const char *template, char *buf, int buf_sz, int *tmpfd)
 {
 	char *fname;
-- 
2.41.0

[PATCH 2/2] pe-zboot: Truncate the trailing zero if Image is signed

[Pingfan Liu]

*** Issue ***
In the linux kernel drivers/firmware/efi/libstub/Makefile.zboot, the
original Image is padded with zero, using the following instruction:
	truncate -s $$(hexdump -s16 -n4 -e '"%u"' $<) $@

Hence pe-zboot.c decomopresses and gets Image plus trailing zeroes.

These trailing zeroes don't affect loading the original PE file. But
they do raise an issue during the signature verfication. The root cause is
that the kernel function:
	static int pefile_digest_pe_contents(const void *pebuf, unsigned int pelen,
					     struct pefile_context *ctx,
					     struct shash_desc *desc)
treats [pebuf, pebuf+pelen] as valid payload, which includes the
trailing zeroes. But that is not the truth.

*** Solution ***
In pratice, the table of attribute certificates come at the end of a
PE file. This patch utilizes that fact and truncates at the boundary of the
certificate table to get the original Image.

Signed-off-by: Pingfan Liu <piliu@redhat.com>
Cc: Simon Horman <horms@kernel.org>
To: kexec@lists.infradead.org
---
 include/pe.h           | 30 ++++++++++++++++++++++++++++++
 kexec/kexec-pe-zboot.c | 25 ++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/include/pe.h b/include/pe.h
index 1e1c59b..a1aa91e 100644
--- a/include/pe.h
+++ b/include/pe.h
@@ -88,6 +88,30 @@ struct pe32plus_opt_hdr {
 	uint32_t data_dirs;	/* number of data dir entries */
 };
 
+struct data_dirent {
+	uint32_t virtual_address;	/* relative to load address */
+	uint32_t size;
+};
+
+struct data_directory {
+	struct data_dirent exports;		/* .edata */
+	struct data_dirent imports;		/* .idata */
+	struct data_dirent resources;		/* .rsrc */
+	struct data_dirent exceptions;		/* .pdata */
+	struct data_dirent certs;		/* certs */
+	struct data_dirent base_relocations;	/* .reloc */
+	struct data_dirent debug;		/* .debug */
+	struct data_dirent arch;		/* reservered */
+	struct data_dirent global_ptr;		/* global pointer reg. Size=0 */
+	struct data_dirent tls;			/* .tls */
+	struct data_dirent load_config;		/* load configuration structure */
+	struct data_dirent bound_imports;	/* no idea */
+	struct data_dirent import_addrs;	/* import address table */
+	struct data_dirent delay_imports;	/* delay-load import table */
+	struct data_dirent clr_runtime_hdr;	/* .cor (object only) */
+	struct data_dirent reserved;
+};
+
 struct section_header {
 	char name[8];			/* name or "/12\0" string tbl offset */
 	uint32_t virtual_size;		/* size of loaded section in ram */
@@ -101,6 +125,12 @@ struct section_header {
 	uint32_t flags;
 };
 
+struct win_certificate {
+	uint32_t length;
+	uint16_t revision;
+	uint16_t cert_type;
+};
+
 /*
  * Return -1 if not PE, else offset of the PE header
  */
diff --git a/kexec/kexec-pe-zboot.c b/kexec/kexec-pe-zboot.c
index 3abd17d..5c65062 100644
--- a/kexec/kexec-pe-zboot.c
+++ b/kexec/kexec-pe-zboot.c
@@ -28,6 +28,7 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include "kexec.h"
+#include <pe.h>
 #include <kexec-pe-zboot.h>
 
 #define FILENAME_IMAGE		"/tmp/ImageXXXXXX"
@@ -44,8 +45,11 @@ int pez_prepare(const char *crude_buf, off_t buf_sz, int *kernel_fd,
 	int fd = 0;
 	char *fname = NULL;
 	char *kernel_uncompressed_buf = NULL;
-	off_t decompressed_size = 0;
+	char *parse;
+	off_t original_file_sz, decompressed_size = 0;
 	const struct linux_pe_zboot_header *z;
+	struct pe32plus_opt_hdr *opt_hdr;
+	struct data_directory *dir;
 
 	z = (const struct linux_pe_zboot_header *)(crude_buf);
 
@@ -91,12 +95,27 @@ int pez_prepare(const char *crude_buf, off_t buf_sz, int *kernel_fd,
 	kernel_uncompressed_buf = slurp_decompress_file(fname,
 							&decompressed_size);
 
+	original_file_sz = decompressed_size;
 	dbgprintf("%s: decompressed size %ld\n", __func__, decompressed_size);
 
+	/* Makefile.zboot pads Image with zero, but the trailing zero is not part of PE file */
+	parse = kernel_uncompressed_buf + get_pehdr_offset(kernel_uncompressed_buf);
+	parse += sizeof(struct pe_hdr);
+	opt_hdr = (struct pe32plus_opt_hdr*)parse;
+	parse += sizeof(struct pe32plus_opt_hdr);
+	dir = (struct data_directory *)parse;
+	if (opt_hdr->data_dirs > ((char *)&dir->certs - (char *)dir)/sizeof(struct data_dirent)) {
+		/* If signed, the Attribute Certificate Table is always at the end of the PE file */
+		if (dir->certs.virtual_address != 0 && dir->certs.size != 0) {
+			original_file_sz = dir->certs.virtual_address + dir->certs.size;
+			ftruncate(fd, 0);
+		}
+	}
+
 	lseek(fd, 0, SEEK_SET);
 
 	if (write(fd,  kernel_uncompressed_buf,
-		  decompressed_size) != decompressed_size) {
+		  original_file_sz) != original_file_sz) {
 		dbgprintf("%s: Can't write the decompressed file %s\n",
 				__func__, fname);
 		ret = -1;
@@ -111,7 +130,7 @@ int pez_prepare(const char *crude_buf, off_t buf_sz, int *kernel_fd,
 		goto fail_bad_header;
 	}
 
-	*kernel_size = decompressed_size;
+	*kernel_size = original_file_sz;
 	dbgprintf("%s: done\n", __func__);
 
 	ret = 0;
-- 
2.41.0

Re: [PATCH 2/2] pe-zboot: Truncate the trailing zero if Image is signed

[Simon Horman]

On Fri, Dec 06, 2024 at 10:44:43AM +0800, Pingfan Liu wrote:
> *** Issue ***
> In the linux kernel drivers/firmware/efi/libstub/Makefile.zboot, the
> original Image is padded with zero, using the following instruction:
> 	truncate -s $$(hexdump -s16 -n4 -e '"%u"' $<) $@
> 
> Hence pe-zboot.c decomopresses and gets Image plus trailing zeroes.
> 
> These trailing zeroes don't affect loading the original PE file. But
> they do raise an issue during the signature verfication. The root cause is
> that the kernel function:
> 	static int pefile_digest_pe_contents(const void *pebuf, unsigned int pelen,
> 					     struct pefile_context *ctx,
> 					     struct shash_desc *desc)
> treats [pebuf, pebuf+pelen] as valid payload, which includes the
> trailing zeroes. But that is not the truth.
> 
> *** Solution ***
> In pratice, the table of attribute certificates come at the end of a
> PE file. This patch utilizes that fact and truncates at the boundary of the
> certificate table to get the original Image.
> 
> Signed-off-by: Pingfan Liu <piliu@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> To: kexec@lists.infradead.org

Thanks, applied after addressing some minor spelling issues
in the patch description.

Re: [PATCH 1/2] PE: Extract get_pehdr_offset() for reuse

[Simon Horman]

On Fri, Dec 06, 2024 at 10:44:42AM +0800, Pingfan Liu wrote:
> The get_pehdr_offset() function helps skip the DOS stub in a PE file to
> locate the start of the PE header. Since it can be used in multiple C
> files, it is being moved to a header file.
> 
> Signed-off-by: Pingfan Liu <piliu@redhat.com>

Thanks, applied.