[RFC PATCH 0/4] efi: x86: Use strict W^X mappings in PE/COFF header

[RFC PATCH 0/4] efi: x86: Use strict W^X mappings in PE/COFF header

[Ard Biesheuvel]

This is a follow-up to work proposed by Evgeny to tighten memory
permissions used by the EFI stub and subsequently by the decompressor on
x86.

Instead of going out of our way to make more space in the first 500
bytes of the image, and relying on non-1:1 mapped sections (which is
risky in the context of bespoke PE loaders), these patches reorganize
the header so the PE header comes after the x86 setup header, and can be
extended at will.

I pushed a branch at [1] that combines this with v4 of Evgeny's series
(after some minor surgery, e.g., to reorder the text and rodata sections
so they are contiguous)

We might split off the rodata section as well, and give it read/non-exec
permissions, but I'd like to discuss the approach first, and perhaps get
some testing data points.

Cc: Evgeniy Baskov <baskov@ispras.ru>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: Peter Jones <pjones@redhat.com>
Cc: "Limonciello, Mario" <mario.limonciello@amd.com>

[0] https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4

Ard Biesheuvel (4):
  efi: x86: Use private copy of struct setup_header
  efi: x86: Move PE header after setup header
  efi: x86: Drop alignment section header flags
  efi: x86: Split PE/COFF .text section into .text and .data

 arch/x86/boot/Makefile                  |  2 +-
 arch/x86/boot/header.S                  | 52 +++++++++-----------
 arch/x86/boot/setup.ld                  |  1 +
 arch/x86/boot/tools/build.c             | 38 +++++++++-----
 drivers/firmware/efi/libstub/x86-stub.c | 43 +++-------------
 5 files changed, 59 insertions(+), 77 deletions(-)

-- 
2.39.2

[RFC PATCH 1/4] efi: x86: Use private copy of struct setup_header

[Ard Biesheuvel]

The native EFI entrypoint does not take a struct boot_params from the
loader, but instead, it constructs one from scratch, using the setup
header data from the start of the image.

This setup header is placed in a way that permits legacy loaders to
manipulate the contents (i.e., to pass the kernel command line or the
address and size of an initial ramdisk), but EFI boot does not use it in
that way - it only copies the contents that were placed there at build
time, but EFI loaders will not (and should not) manipulate the setup
header to configure the boot. (Commit 63bf28ceb3ebbe76 "efi: x86: Wipe
setup_data on pure EFI boot" deals with some of the fallout of using
setup_data in a way that breaks EFI boot.) So having a pristine, private
copy of the setup header rather than copying the one legacy boot loaders
use would be an advantage for EFI boot.

As it turns out, there is another reason why copying this header is
slightly problematic: the fixed offset of 0x1f1 bytes into the image
makes it difficult to describe all the sections of the image, as we are
running out of space. We could mitigate this by placing some parts of
the PE/COFF header after this setup header (which will happen in a
subsequent patch), but this makes the setup_header fundamentally part of
the PE header as well, which means that it is no longer part of the
'in-memory' representation of the PE image, but only part of the
'on-disk' representation. This means copying the setup header from the
running image may not work, as the PE loader may not have put it in
memory to begin with.

So let's work around this, and simplify things at the same time, by just
copying the setup header into a EFI stub private allocation at build
time, and use that instead of copying it from the loaded image after it
has been started.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/Makefile                  |  2 +-
 arch/x86/boot/tools/build.c             |  8 ++++
 drivers/firmware/efi/libstub/x86-stub.c | 43 +++-----------------
 3 files changed, 15 insertions(+), 38 deletions(-)

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 9e38ffaadb5d972e..8203f1a23f7a1734 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
 
 SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
 
-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
 
 quiet_cmd_zoffset = ZOFFSET $@
       cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index a25c0c5efda95989..e6fd09789482ed04 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -115,6 +115,7 @@ static unsigned long efi32_stub_entry;
 static unsigned long efi64_stub_entry;
 static unsigned long efi_pe_entry;
 static unsigned long efi32_pe_entry;
+static unsigned long efi_boot_params;
 static unsigned long kernel_info;
 static unsigned long startup_64;
 static unsigned long _ehead;
@@ -453,6 +454,7 @@ static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, efi64_stub_entry);
 		PARSE_ZOFS(p, efi_pe_entry);
 		PARSE_ZOFS(p, efi32_pe_entry);
+		PARSE_ZOFS(p, efi_boot_params);
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
 		PARSE_ZOFS(p, _ehead);
@@ -589,6 +591,12 @@ int main(int argc, char **argv)
 	memcpy(output + setup_size, kernel, kern_file_size);
 	memset(output + setup_size + kern_file_size, 0, kern_size - kern_file_size);
 
+#ifdef CONFIG_EFI_STUB
+	/* Copy the setup header */
+	memcpy(output + setup_size + efi_boot_params + 0x1f1, &buf[0x1f1],
+	       0x290 - 0x1f1 /* == max possible sizeof(struct setup_header) */);
+#endif
+
 	/* Calculate and write kernel checksum. */
 	crc = partial_crc32(output, total_size - 4, crc);
 	put_unaligned_le32(crc, &output[total_size - 4]);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 1d1ab1911fd37767..5dbc9c7a4aa3aee7 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -347,6 +347,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 			       efi_system_table_t *sys_table_arg,
 			       struct boot_params *boot_params);
 
+struct boot_params efi_boot_params __section(".data") __aligned(SZ_4K);
+
 /*
  * Because the x86 boot code expects to be passed a boot_params we
  * need to create one ourselves (usually the bootloader would create
@@ -355,7 +357,6 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 				   efi_system_table_t *sys_table_arg)
 {
-	struct boot_params *boot_params;
 	struct setup_header *hdr;
 	void *image_base;
 	efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
@@ -378,55 +379,23 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	image_base = efi_table_attr(image, image_base);
 	image_offset = (void *)startup_32 - image_base;
 
-	status = efi_allocate_pages(sizeof(struct boot_params),
-				    (unsigned long *)&boot_params, ULONG_MAX);
-	if (status != EFI_SUCCESS) {
-		efi_err("Failed to allocate lowmem for boot params\n");
-		efi_exit(handle, status);
-	}
-
-	memset(boot_params, 0x0, sizeof(struct boot_params));
-
-	hdr = &boot_params->hdr;
-
-	/* Copy the setup header from the second sector to boot_params */
-	memcpy(&hdr->jump, image_base + 512,
-	       sizeof(struct setup_header) - offsetof(struct setup_header, jump));
-
-	/*
-	 * Fill out some of the header fields ourselves because the
-	 * EFI firmware loader doesn't load the first sector.
-	 */
-	hdr->root_flags	= 1;
-	hdr->vid_mode	= 0xffff;
-	hdr->boot_flag	= 0xAA55;
-
-	hdr->type_of_loader = 0x21;
+	hdr = &efi_boot_params.hdr;
 
 	/* Convert unicode cmdline to ascii */
 	cmdline_ptr = efi_convert_cmdline(image, &options_size);
 	if (!cmdline_ptr)
 		goto fail;
 
-	efi_set_u64_split((unsigned long)cmdline_ptr,
-			  &hdr->cmd_line_ptr, &boot_params->ext_cmd_line_ptr);
+	efi_set_u64_split((unsigned long)cmdline_ptr, &hdr->cmd_line_ptr,
+			  &efi_boot_params.ext_cmd_line_ptr);
 
 	hdr->ramdisk_image = 0;
 	hdr->ramdisk_size = 0;
 
-	/*
-	 * Disregard any setup data that was provided by the bootloader:
-	 * setup_data could be pointing anywhere, and we have no way of
-	 * authenticating or validating the payload.
-	 */
-	hdr->setup_data = 0;
-
-	efi_stub_entry(handle, sys_table_arg, boot_params);
+	efi_stub_entry(handle, sys_table_arg, &efi_boot_params);
 	/* not reached */
 
 fail:
-	efi_free(sizeof(struct boot_params), (unsigned long)boot_params);
-
 	efi_exit(handle, status);
 }
 
-- 
2.39.2

[RFC PATCH 2/4] efi: x86: Move PE header after setup header

[Ard Biesheuvel]

We are currently limited in the number of PE/COFF sections we can
describe in the PE header, due to lack of space. This is caused by the
presence of the setup header at offset 0x1f1, leaving only the space
before it for PE metadata.

However, now that we no longer copy the setup_header from this part of
the image for use by the EFI stub, we no longer have to describe it as
part of the loadable image. This means we can put the PE header *after*
the setup header, and use as much space as we like. It also means we
don't have to describe this part of the image in PE/COFF, and simply
treat it as part of the header. This means we can drop the ".setup"
section as well.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/header.S      | 26 +++-----------------
 arch/x86/boot/setup.ld      |  1 +
 arch/x86/boot/tools/build.c | 11 +++------
 3 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 9338c68e7413d6e6..aba499404d8b870e 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -85,7 +85,7 @@ bs_die:
 	# Offset to the PE header.
 	#
 	.long	LINUX_PE_MAGIC
-	.long	pe_header
+	.long	pe_header - bootsect_start
 #endif /* CONFIG_EFI_STUB */
 
 	.section ".bsdata", "a"
@@ -96,6 +96,8 @@ bugger_off_msg:
 	.byte	0
 
 #ifdef CONFIG_EFI_STUB
+	.section ".peheader", "a"
+	.align 8
 pe_header:
 	.long	PE_MAGIC
 
@@ -161,7 +163,7 @@ extra_header_fields:
 	#
 	.long	0				# SizeOfImage
 
-	.long	0x200				# SizeOfHeaders
+	.long	0x800				# SizeOfHeaders
 	.long	0				# CheckSum
 	.word	IMAGE_SUBSYSTEM_EFI_APPLICATION	# Subsystem (EFI application)
 #ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
@@ -192,26 +194,6 @@ extra_header_fields:
 
 	# Section table
 section_table:
-	#
-	# The offset & size fields are filled in by build.c.
-	#
-	.ascii	".setup"
-	.byte	0
-	.byte	0
-	.long	0
-	.long	0x0				# startup_{32,64}
-	.long	0				# Size of initialized data
-						# on disk
-	.long	0x0				# startup_{32,64}
-	.long	0				# PointerToRelocations
-	.long	0				# PointerToLineNumbers
-	.word	0				# NumberOfRelocations
-	.word	0				# NumberOfLineNumbers
-	.long	IMAGE_SCN_CNT_CODE		| \
-		IMAGE_SCN_MEM_READ		| \
-		IMAGE_SCN_MEM_EXECUTE		| \
-		IMAGE_SCN_ALIGN_16BYTES		# Characteristics
-
 	#
 	# The EFI application loader requires a relocation section
 	# because EFI applications must be relocatable. The .reloc
diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
index 49546c247ae25e97..5981287bbcb7f439 100644
--- a/arch/x86/boot/setup.ld
+++ b/arch/x86/boot/setup.ld
@@ -16,6 +16,7 @@ SECTIONS
 	. = 495;
 	.header		: { *(.header) }
 	.entrytext	: { *(.entrytext) }
+	.peheader	: { *(.peheader) }
 	.inittext	: { *(.inittext) }
 	.initdata	: { *(.initdata) }
 	__end_init = .;
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index e6fd09789482ed04..883e6359221cd588 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -296,16 +296,13 @@ static void update_pecoff_section_header(char *section_name, uint32_t offset, ui
 	update_pecoff_section_header_fields(section_name, offset, size, size, offset);
 }
 
-static void update_pecoff_setup_and_reloc(unsigned int size)
+static void update_pecoff_reloc(unsigned int size)
 {
-	uint32_t setup_offset = SECTOR_SIZE;
 	uint32_t reloc_offset = size - PECOFF_RELOC_RESERVE - PECOFF_COMPAT_RESERVE;
 #ifdef CONFIG_EFI_MIXED
 	uint32_t compat_offset = reloc_offset + PECOFF_RELOC_RESERVE;
 #endif
-	uint32_t setup_size = reloc_offset - setup_offset;
 
-	update_pecoff_section_header(".setup", setup_offset, setup_size);
 	update_pecoff_section_header(".reloc", reloc_offset, PECOFF_RELOC_RESERVE);
 
 	/*
@@ -353,7 +350,7 @@ static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
 	 * Size of code: Subtract the size of the first sector (512 bytes)
 	 * which includes the header.
 	 */
-	put_unaligned_le32(file_sz - SECTOR_SIZE + bss_sz, &hdr->text_size);
+	put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
 
 	/* Size of image */
 	put_unaligned_le32(init_sz, &hdr->image_size);
@@ -407,7 +404,7 @@ static void efi_stub_entry_update(void)
 
 #else
 
-static inline void update_pecoff_setup_and_reloc(unsigned int size) {}
+static inline void update_pecoff_reloc(unsigned int size) {}
 static inline void update_pecoff_text(unsigned int text_start,
 				      unsigned int file_sz,
 				      unsigned int init_sz) {}
@@ -542,7 +539,7 @@ int main(int argc, char **argv)
 #ifdef CONFIG_EFI_STUB
 	/* PE specification require 512-byte minimum section file alignment */
 	kern_size = round_up(kern_file_size + 4, SECTOR_SIZE);
-	update_pecoff_setup_and_reloc(setup_size);
+	update_pecoff_reloc(setup_size);
 #else
 	/* Number of 16-byte paragraphs, including space for a 4-byte CRC */
 	kern_size = round_up(kern_file_size + 4, PARAGRAPH_SIZE);
-- 
2.39.2

[RFC PATCH 3/4] efi: x86: Drop alignment section header flags

[Ard Biesheuvel]

The section header flags for alignment are documented as only being
applicable to PE object files, not PE executables, so let's drop them.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/header.S | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index aba499404d8b870e..4f1e1791cda4d316 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -212,8 +212,7 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
 		IMAGE_SCN_MEM_READ		| \
-		IMAGE_SCN_MEM_DISCARDABLE	| \
-		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
+		IMAGE_SCN_MEM_DISCARDABLE	# Characteristics
 
 #ifdef CONFIG_EFI_MIXED
 	#
@@ -231,8 +230,7 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
 		IMAGE_SCN_MEM_READ		| \
-		IMAGE_SCN_MEM_DISCARDABLE	| \
-		IMAGE_SCN_ALIGN_1BYTES		# Characteristics
+		IMAGE_SCN_MEM_DISCARDABLE	# Characteristics
 #endif
 
 	#
@@ -253,8 +251,7 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	IMAGE_SCN_CNT_CODE		| \
 		IMAGE_SCN_MEM_READ		| \
-		IMAGE_SCN_MEM_EXECUTE		| \
-		IMAGE_SCN_ALIGN_16BYTES		# Characteristics
+		IMAGE_SCN_MEM_EXECUTE		# Characteristics
 
 	.set	section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */
-- 
2.39.2

[RFC PATCH 4/4] efi: x86: Split PE/COFF .text section into .text and .data

[Ard Biesheuvel]

Modern PE loader implementations used by EFI will honour the PE section
permission attributes, and so we can use them to avoid mappings that are
writable and executable at the same time.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/header.S      | 17 ++++++++++++++++
 arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -253,6 +253,23 @@ section_table:
 		IMAGE_SCN_MEM_READ		| \
 		IMAGE_SCN_MEM_EXECUTE		# Characteristics
 
+	.ascii	".data"
+	.byte	0
+	.byte	0
+	.byte	0
+	.long	0
+	.long	0x0				# startup_{32,64}
+	.long	0				# Size of initialized data
+						# on disk
+	.long	0x0				# startup_{32,64}
+	.long	0				# PointerToRelocations
+	.long	0				# PointerToLineNumbers
+	.word	0				# NumberOfRelocations
+	.word	0				# NumberOfLineNumbers
+	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
+		IMAGE_SCN_MEM_READ		| \
+		IMAGE_SCN_MEM_WRITE		# Characteristics
+
 	.set	section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */
 
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 883e6359221cd588..b449c82feaadf2b8 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -119,6 +119,7 @@ static unsigned long efi_boot_params;
 static unsigned long kernel_info;
 static unsigned long startup_64;
 static unsigned long _ehead;
+static unsigned long _data;
 static unsigned long _end;
 
 /*----------------------------------------------------------------------*/
@@ -347,10 +348,15 @@ static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
 	init_sz	+= CONFIG_PHYSICAL_ALIGN;
 
 	/*
-	 * Size of code: Subtract the size of the first sector (512 bytes)
-	 * which includes the header.
+	 * Size of code: the size of the combined .text/.rodata section, which
+	 * ends at the _data marker symbol.
 	 */
-	put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
+	put_unaligned_le32(_data, &hdr->text_size);
+
+	/*
+	 * Size of data: the size of the combined .data/.bss section.
+	 */
+	put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
 
 	/* Size of image */
 	put_unaligned_le32(init_sz, &hdr->image_size);
@@ -360,9 +366,13 @@ static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
 	 */
 	put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
 
-	update_pecoff_section_header_fields(".text", text_start, text_sz + bss_sz,
-					    text_sz, text_start);
+	update_pecoff_section_header_fields(".text", text_start, _data,
+					    _data, text_start);
 
+	update_pecoff_section_header_fields(".data", text_start + _data,
+					    text_sz - _data + bss_sz,
+					    text_sz - _data,
+					    text_start + _data);
 	return text_start + file_sz;
 }
 
@@ -455,6 +465,7 @@ static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
 		PARSE_ZOFS(p, _ehead);
+		PARSE_ZOFS(p, _data);
 		PARSE_ZOFS(p, _end);
 
 		p = strchr(p, '\n');
-- 
2.39.2

Re: [RFC PATCH 2/4] efi: x86: Move PE header after setup header

[Ard Biesheuvel]

On Wed, 8 Mar 2023 at 21:22, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> We are currently limited in the number of PE/COFF sections we can
> describe in the PE header, due to lack of space. This is caused by the
> presence of the setup header at offset 0x1f1, leaving only the space
> before it for PE metadata.
>
> However, now that we no longer copy the setup_header from this part of
> the image for use by the EFI stub, we no longer have to describe it as
> part of the loadable image. This means we can put the PE header *after*
> the setup header, and use as much space as we like. It also means we
> don't have to describe this part of the image in PE/COFF, and simply
> treat it as part of the header. This means we can drop the ".setup"
> section as well.
>

Better idea: let's just rip out the ancient real mode boot code. It's
20+ years old and only prints an error message in case the kernel is
booted in a way that has not been supported for all that time.

Comments anyone?


> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/header.S      | 26 +++-----------------
>  arch/x86/boot/setup.ld      |  1 +
>  arch/x86/boot/tools/build.c | 11 +++------
>  3 files changed, 9 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 9338c68e7413d6e6..aba499404d8b870e 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -85,7 +85,7 @@ bs_die:
>         # Offset to the PE header.
>         #
>         .long   LINUX_PE_MAGIC
> -       .long   pe_header
> +       .long   pe_header - bootsect_start
>  #endif /* CONFIG_EFI_STUB */
>
>         .section ".bsdata", "a"
> @@ -96,6 +96,8 @@ bugger_off_msg:
>         .byte   0
>
>  #ifdef CONFIG_EFI_STUB
> +       .section ".peheader", "a"
> +       .align 8
>  pe_header:
>         .long   PE_MAGIC
>
> @@ -161,7 +163,7 @@ extra_header_fields:
>         #
>         .long   0                               # SizeOfImage
>
> -       .long   0x200                           # SizeOfHeaders
> +       .long   0x800                           # SizeOfHeaders
>         .long   0                               # CheckSum
>         .word   IMAGE_SUBSYSTEM_EFI_APPLICATION # Subsystem (EFI application)
>  #ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
> @@ -192,26 +194,6 @@ extra_header_fields:
>
>         # Section table
>  section_table:
> -       #
> -       # The offset & size fields are filled in by build.c.
> -       #
> -       .ascii  ".setup"
> -       .byte   0
> -       .byte   0
> -       .long   0
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # Size of initialized data
> -                                               # on disk
> -       .long   0x0                             # startup_{32,64}
> -       .long   0                               # PointerToRelocations
> -       .long   0                               # PointerToLineNumbers
> -       .word   0                               # NumberOfRelocations
> -       .word   0                               # NumberOfLineNumbers
> -       .long   IMAGE_SCN_CNT_CODE              | \
> -               IMAGE_SCN_MEM_READ              | \
> -               IMAGE_SCN_MEM_EXECUTE           | \
> -               IMAGE_SCN_ALIGN_16BYTES         # Characteristics
> -
>         #
>         # The EFI application loader requires a relocation section
>         # because EFI applications must be relocatable. The .reloc
> diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
> index 49546c247ae25e97..5981287bbcb7f439 100644
> --- a/arch/x86/boot/setup.ld
> +++ b/arch/x86/boot/setup.ld
> @@ -16,6 +16,7 @@ SECTIONS
>         . = 495;
>         .header         : { *(.header) }
>         .entrytext      : { *(.entrytext) }
> +       .peheader       : { *(.peheader) }
>         .inittext       : { *(.inittext) }
>         .initdata       : { *(.initdata) }
>         __end_init = .;
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index e6fd09789482ed04..883e6359221cd588 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -296,16 +296,13 @@ static void update_pecoff_section_header(char *section_name, uint32_t offset, ui
>         update_pecoff_section_header_fields(section_name, offset, size, size, offset);
>  }
>
> -static void update_pecoff_setup_and_reloc(unsigned int size)
> +static void update_pecoff_reloc(unsigned int size)
>  {
> -       uint32_t setup_offset = SECTOR_SIZE;
>         uint32_t reloc_offset = size - PECOFF_RELOC_RESERVE - PECOFF_COMPAT_RESERVE;
>  #ifdef CONFIG_EFI_MIXED
>         uint32_t compat_offset = reloc_offset + PECOFF_RELOC_RESERVE;
>  #endif
> -       uint32_t setup_size = reloc_offset - setup_offset;
>
> -       update_pecoff_section_header(".setup", setup_offset, setup_size);
>         update_pecoff_section_header(".reloc", reloc_offset, PECOFF_RELOC_RESERVE);
>
>         /*
> @@ -353,7 +350,7 @@ static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int
>          * Size of code: Subtract the size of the first sector (512 bytes)
>          * which includes the header.
>          */
> -       put_unaligned_le32(file_sz - SECTOR_SIZE + bss_sz, &hdr->text_size);
> +       put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
>
>         /* Size of image */
>         put_unaligned_le32(init_sz, &hdr->image_size);
> @@ -407,7 +404,7 @@ static void efi_stub_entry_update(void)
>
>  #else
>
> -static inline void update_pecoff_setup_and_reloc(unsigned int size) {}
> +static inline void update_pecoff_reloc(unsigned int size) {}
>  static inline void update_pecoff_text(unsigned int text_start,
>                                       unsigned int file_sz,
>                                       unsigned int init_sz) {}
> @@ -542,7 +539,7 @@ int main(int argc, char **argv)
>  #ifdef CONFIG_EFI_STUB
>         /* PE specification require 512-byte minimum section file alignment */
>         kern_size = round_up(kern_file_size + 4, SECTOR_SIZE);
> -       update_pecoff_setup_and_reloc(setup_size);
> +       update_pecoff_reloc(setup_size);
>  #else
>         /* Number of 16-byte paragraphs, including space for a 4-byte CRC */
>         kern_size = round_up(kern_file_size + 4, PARAGRAPH_SIZE);
> --
> 2.39.2
>

Re: [RFC PATCH 0/4] efi: x86: Use strict W^X mappings in PE/COFF header

[Evgeniy Baskov]

On 2023-03-08 23:22, Ard Biesheuvel wrote:
> This is a follow-up to work proposed by Evgeny to tighten memory
> permissions used by the EFI stub and subsequently by the decompressor 
> on
> x86.
> 
> Instead of going out of our way to make more space in the first 500
> bytes of the image, and relying on non-1:1 mapped sections (which is
> risky in the context of bespoke PE loaders), these patches reorganize
> the header so the PE header comes after the x86 setup header, and can 
> be
> extended at will.
> 
> I pushed a branch at [1] that combines this with v4 of Evgeny's series
> (after some minor surgery, e.g., to reorder the text and rodata 
> sections
> so they are contiguous)
> 
> We might split off the rodata section as well, and give it 
> read/non-exec
> permissions, but I'd like to discuss the approach first, and perhaps 
> get
> some testing data points.
> 
> Cc: Evgeniy Baskov <baskov@ispras.ru>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
> Cc: Peter Jones <pjones@redhat.com>
> Cc: "Limonciello, Mario" <mario.limonciello@amd.com>
> 
> [0] 
> https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4
> 
> Ard Biesheuvel (4):
>   efi: x86: Use private copy of struct setup_header
>   efi: x86: Move PE header after setup header
>   efi: x86: Drop alignment section header flags
>   efi: x86: Split PE/COFF .text section into .text and .data
> 
>  arch/x86/boot/Makefile                  |  2 +-
>  arch/x86/boot/header.S                  | 52 +++++++++-----------
>  arch/x86/boot/setup.ld                  |  1 +
>  arch/x86/boot/tools/build.c             | 38 +++++++++-----
>  drivers/firmware/efi/libstub/x86-stub.c | 43 +++-------------
>  5 files changed, 59 insertions(+), 77 deletions(-)

I've quickly looked through these patches but I'll do more testing 
tomorrow.

This approach seems to be better than mine if it will work. I've tried
the similar thing but I did not think of creating the local copy of the
bootparams and the attempt to map them did not work since the PE loader
I am trying to get kernel booting with does not accept sections before
the PE header. But since the bootparams is inside the padding and is
not used, it should be fine.

But this will still need more changes to work properly with stricter PE
loaders like the one that I've mentioned in my patch series [1].

The image should also have 4K aligned section virtual addresses and 
sizes
(even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory
attributes (or refuse to load the kernel). Another desired thing is 
having
adjacent section with no padding in between them, since [1] does have a
mode that requires sections them to be adjacent. 
(SizeOfHeaders/header_size
should also be set to the size of setup since it is also checked to be
adjacent to the first section.)

I did not do the one-to-one mapping of file and virtual addresses since 
it
would require almost 4K paddings for the auxiliary sections.

[1] https://github.com/acidanthera/audk/tree/secure_pe

Thanks,
Evgeniy Baskov

Re: [RFC PATCH 4/4] efi: x86: Split PE/COFF .text section into .text and .data

[Evgeniy Baskov]

On 2023-03-08 23:22, Ard Biesheuvel wrote:
> Modern PE loader implementations used by EFI will honour the PE section
> permission attributes, and so we can use them to avoid mappings that 
> are
> writable and executable at the same time.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/header.S      | 17 ++++++++++++++++
>  arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
> --- a/arch/x86/boot/header.S
> +++ b/arch/x86/boot/header.S
> @@ -253,6 +253,23 @@ section_table:
>  		IMAGE_SCN_MEM_READ		| \
>  		IMAGE_SCN_MEM_EXECUTE		# Characteristics
> 
> +	.ascii	".data"
> +	.byte	0
> +	.byte	0
> +	.byte	0
> +	.long	0
> +	.long	0x0				# startup_{32,64}
> +	.long	0				# Size of initialized data
> +						# on disk
> +	.long	0x0				# startup_{32,64}
> +	.long	0				# PointerToRelocations
> +	.long	0				# PointerToLineNumbers
> +	.word	0				# NumberOfRelocations
> +	.word	0				# NumberOfLineNumbers
> +	.long	IMAGE_SCN_CNT_INITIALIZED_DATA	| \
> +		IMAGE_SCN_MEM_READ		| \
> +		IMAGE_SCN_MEM_WRITE		# Characteristics
> +
>  	.set	section_count, (. - section_table) / 40
>  #endif /* CONFIG_EFI_STUB */
> 
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 883e6359221cd588..b449c82feaadf2b8 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -119,6 +119,7 @@ static unsigned long efi_boot_params;
>  static unsigned long kernel_info;
>  static unsigned long startup_64;
>  static unsigned long _ehead;
> +static unsigned long _data;
>  static unsigned long _end;
> 
>  
> /*----------------------------------------------------------------------*/
> @@ -347,10 +348,15 @@ static unsigned int
> update_pecoff_sections(unsigned int text_start, unsigned int
>  	init_sz	+= CONFIG_PHYSICAL_ALIGN;
> 
>  	/*
> -	 * Size of code: Subtract the size of the first sector (512 bytes)
> -	 * which includes the header.
> +	 * Size of code: the size of the combined .text/.rodata section, 
> which
> +	 * ends at the _data marker symbol.
>  	 */
> -	put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
> +	put_unaligned_le32(_data, &hdr->text_size);
> +
> +	/*
> +	 * Size of data: the size of the combined .data/.bss section.
> +	 */
> +	put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
> 
>  	/* Size of image */
>  	put_unaligned_le32(init_sz, &hdr->image_size);
> @@ -360,9 +366,13 @@ static unsigned int
> update_pecoff_sections(unsigned int text_start, unsigned int
>  	 */
>  	put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
> 
> -	update_pecoff_section_header_fields(".text", text_start, text_sz + 
> bss_sz,
> -					    text_sz, text_start);
> +	update_pecoff_section_header_fields(".text", text_start, _data,
> +					    _data, text_start);
> 
> +	update_pecoff_section_header_fields(".data", text_start + _data,
> +					    text_sz - _data + bss_sz,
> +					    text_sz - _data,
> +					    text_start + _data);
>  	return text_start + file_sz;
>  }
> 
> @@ -455,6 +465,7 @@ static void parse_zoffset(char *fname)
>  		PARSE_ZOFS(p, kernel_info);
>  		PARSE_ZOFS(p, startup_64);
>  		PARSE_ZOFS(p, _ehead);
> +		PARSE_ZOFS(p, _data);

This also requires _data to be fetched to zoffset.h:

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 8203f1a23f7a..0e5a18c3c165 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE

  SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))

-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define 
ZO_\2 0x\1/p'
+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z] 
\(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|_data\|z_.*\)$$/\#define 
ZO_\2 0x\1/p'

  quiet_cmd_zoffset = ZOFFSET $@
        cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@

>  		PARSE_ZOFS(p, _end);
> 
>  		p = strchr(p, '\n');

Re: [RFC PATCH 4/4] efi: x86: Split PE/COFF .text section into .text and .data

[Ard Biesheuvel]

On Thu, 9 Mar 2023 at 19:02, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-08 23:22, Ard Biesheuvel wrote:
> > Modern PE loader implementations used by EFI will honour the PE section
> > permission attributes, and so we can use them to avoid mappings that
> > are
> > writable and executable at the same time.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/header.S      | 17 ++++++++++++++++
> >  arch/x86/boot/tools/build.c | 21 +++++++++++++++-----
> >  2 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> > index 4f1e1791cda4d316..a8ff8bbb17bca7d7 100644
> > --- a/arch/x86/boot/header.S
> > +++ b/arch/x86/boot/header.S
> > @@ -253,6 +253,23 @@ section_table:
> >               IMAGE_SCN_MEM_READ              | \
> >               IMAGE_SCN_MEM_EXECUTE           # Characteristics
> >
> > +     .ascii  ".data"
> > +     .byte   0
> > +     .byte   0
> > +     .byte   0
> > +     .long   0
> > +     .long   0x0                             # startup_{32,64}
> > +     .long   0                               # Size of initialized data
> > +                                             # on disk
> > +     .long   0x0                             # startup_{32,64}
> > +     .long   0                               # PointerToRelocations
> > +     .long   0                               # PointerToLineNumbers
> > +     .word   0                               # NumberOfRelocations
> > +     .word   0                               # NumberOfLineNumbers
> > +     .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
> > +             IMAGE_SCN_MEM_READ              | \
> > +             IMAGE_SCN_MEM_WRITE             # Characteristics
> > +
> >       .set    section_count, (. - section_table) / 40
> >  #endif /* CONFIG_EFI_STUB */
> >
> > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> > index 883e6359221cd588..b449c82feaadf2b8 100644
> > --- a/arch/x86/boot/tools/build.c
> > +++ b/arch/x86/boot/tools/build.c
> > @@ -119,6 +119,7 @@ static unsigned long efi_boot_params;
> >  static unsigned long kernel_info;
> >  static unsigned long startup_64;
> >  static unsigned long _ehead;
> > +static unsigned long _data;
> >  static unsigned long _end;
> >
> >
> > /*----------------------------------------------------------------------*/
> > @@ -347,10 +348,15 @@ static unsigned int
> > update_pecoff_sections(unsigned int text_start, unsigned int
> >       init_sz += CONFIG_PHYSICAL_ALIGN;
> >
> >       /*
> > -      * Size of code: Subtract the size of the first sector (512 bytes)
> > -      * which includes the header.
> > +      * Size of code: the size of the combined .text/.rodata section,
> > which
> > +      * ends at the _data marker symbol.
> >        */
> > -     put_unaligned_le32(text_sz + bss_sz, &hdr->text_size);
> > +     put_unaligned_le32(_data, &hdr->text_size);
> > +
> > +     /*
> > +      * Size of data: the size of the combined .data/.bss section.
> > +      */
> > +     put_unaligned_le32(text_sz - _data + bss_sz, &hdr->data_size);
> >
> >       /* Size of image */
> >       put_unaligned_le32(init_sz, &hdr->image_size);
> > @@ -360,9 +366,13 @@ static unsigned int
> > update_pecoff_sections(unsigned int text_start, unsigned int
> >        */
> >       put_unaligned_le32(text_start + efi_pe_entry, &hdr->entry_point);
> >
> > -     update_pecoff_section_header_fields(".text", text_start, text_sz +
> > bss_sz,
> > -                                         text_sz, text_start);
> > +     update_pecoff_section_header_fields(".text", text_start, _data,
> > +                                         _data, text_start);
> >
> > +     update_pecoff_section_header_fields(".data", text_start + _data,
> > +                                         text_sz - _data + bss_sz,
> > +                                         text_sz - _data,
> > +                                         text_start + _data);
> >       return text_start + file_sz;
> >  }
> >
> > @@ -455,6 +465,7 @@ static void parse_zoffset(char *fname)
> >               PARSE_ZOFS(p, kernel_info);
> >               PARSE_ZOFS(p, startup_64);
> >               PARSE_ZOFS(p, _ehead);
> > +             PARSE_ZOFS(p, _data);
>
> This also requires _data to be fetched to zoffset.h:
>

Indeed - I'd fixed that locally but failed to include it in the patch,
apologies.

> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index 8203f1a23f7a..0e5a18c3c165 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -91,7 +91,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
>
>   SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
>
> -sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z]
> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define
> ZO_\2 0x\1/p'
> +sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [a-zA-Z]
> \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|efi32_pe_entry\|efi_boot_params\|input_data\|kernel_info\|_end\|_ehead\|_text\|_data\|z_.*\)$$/\#define
> ZO_\2 0x\1/p'
>
>   quiet_cmd_zoffset = ZOFFSET $@
>         cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
>
> >               PARSE_ZOFS(p, _end);
> >
> >               p = strchr(p, '\n');

Re: [RFC PATCH 0/4] efi: x86: Use strict W^X mappings in PE/COFF header

[Ard Biesheuvel]

On Thu, 9 Mar 2023 at 18:59, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-08 23:22, Ard Biesheuvel wrote:
> > This is a follow-up to work proposed by Evgeny to tighten memory
> > permissions used by the EFI stub and subsequently by the decompressor
> > on
> > x86.
> >
> > Instead of going out of our way to make more space in the first 500
> > bytes of the image, and relying on non-1:1 mapped sections (which is
> > risky in the context of bespoke PE loaders), these patches reorganize
> > the header so the PE header comes after the x86 setup header, and can
> > be
> > extended at will.
> >
> > I pushed a branch at [1] that combines this with v4 of Evgeny's series
> > (after some minor surgery, e.g., to reorder the text and rodata
> > sections
> > so they are contiguous)
> >
> > We might split off the rodata section as well, and give it
> > read/non-exec
> > permissions, but I'd like to discuss the approach first, and perhaps
> > get
> > some testing data points.
> >
> > Cc: Evgeniy Baskov <baskov@ispras.ru>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
> > Cc: Peter Jones <pjones@redhat.com>
> > Cc: "Limonciello, Mario" <mario.limonciello@amd.com>
> >
> > [0]
> > https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4
> >
> > Ard Biesheuvel (4):
> >   efi: x86: Use private copy of struct setup_header
> >   efi: x86: Move PE header after setup header
> >   efi: x86: Drop alignment section header flags
> >   efi: x86: Split PE/COFF .text section into .text and .data
> >
> >  arch/x86/boot/Makefile                  |  2 +-
> >  arch/x86/boot/header.S                  | 52 +++++++++-----------
> >  arch/x86/boot/setup.ld                  |  1 +
> >  arch/x86/boot/tools/build.c             | 38 +++++++++-----
> >  drivers/firmware/efi/libstub/x86-stub.c | 43 +++-------------
> >  5 files changed, 59 insertions(+), 77 deletions(-)
>
> I've quickly looked through these patches but I'll do more testing
> tomorrow.
>
> This approach seems to be better than mine if it will work. I've tried
> the similar thing but I did not think of creating the local copy of the
> bootparams and the attempt to map them did not work since the PE loader
> I am trying to get kernel booting with does not accept sections before
> the PE header. But since the bootparams is inside the padding and is
> not used, it should be fine.
>
> But this will still need more changes to work properly with stricter PE
> loaders like the one that I've mentioned in my patch series [1].
>
> The image should also have 4K aligned section virtual addresses and
> sizes
> (even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory
> attributes (or refuse to load the kernel).

EDK2 works fine as is, i.e. with only .text and .data aligned to 4k
virtually, and the data size of .data aligned to 512 bytes.

ProtectUefiImageCommon - 0x3C8600C0
  - 0x0000000038777000 - 0x0000000002BC6000
SetUefiImageMemoryAttributes - 0x0000000038777000 - 0x0000000000004000
(0x0000000000004008)
SetUefiImageMemoryAttributes - 0x000000003877B000 - 0x0000000000BEE000
(0x0000000000020008)
SetUefiImageMemoryAttributes - 0x0000000039369000 - 0x0000000001FD4000
(0x0000000000004008)

> Another desired thing is
> having
> adjacent section with no padding in between them, since [1] does have a
> mode that requires sections them to be adjacent.

Does that have any basis in the PE/COFF spec?

> (SizeOfHeaders/header_size
> should also be set to the size of setup since it is also checked to be
> adjacent to the first section.)
>

Does that have any basis in the PE/COFF spec?

> I did not do the one-to-one mapping of file and virtual addresses since
> it
> would require almost 4K paddings for the auxiliary sections.
>
> [1] https://github.com/acidanthera/audk/tree/secure_pe
>

I've backpedaled a little bit from this approach (see my other comment).

If we just rip out the real mode stub, we can keep the PE header
before the setup header, and simply describe whatever comes as .text.

Re: [RFC PATCH 0/4] efi: x86: Use strict W^X mappings in PE/COFF header

[Evgeniy Baskov]

On 2023-03-09 21:09, Ard Biesheuvel wrote:
> On Thu, 9 Mar 2023 at 18:59, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> On 2023-03-08 23:22, Ard Biesheuvel wrote:
>> > This is a follow-up to work proposed by Evgeny to tighten memory
>> > permissions used by the EFI stub and subsequently by the decompressor
>> > on
>> > x86.
>> >
>> > Instead of going out of our way to make more space in the first 500
>> > bytes of the image, and relying on non-1:1 mapped sections (which is
>> > risky in the context of bespoke PE loaders), these patches reorganize
>> > the header so the PE header comes after the x86 setup header, and can
>> > be
>> > extended at will.
>> >
>> > I pushed a branch at [1] that combines this with v4 of Evgeny's series
>> > (after some minor surgery, e.g., to reorder the text and rodata
>> > sections
>> > so they are contiguous)
>> >
>> > We might split off the rodata section as well, and give it
>> > read/non-exec
>> > permissions, but I'd like to discuss the approach first, and perhaps
>> > get
>> > some testing data points.
>> >
>> > Cc: Evgeniy Baskov <baskov@ispras.ru>
>> > Cc: Borislav Petkov <bp@alien8.de>
>> > Cc: Alexey Khoroshilov <khoroshilov@ispras.ru>
>> > Cc: Peter Jones <pjones@redhat.com>
>> > Cc: "Limonciello, Mario" <mario.limonciello@amd.com>
>> >
>> > [0]
>> > https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/
>> > [1]
>> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4
>> >
>> > Ard Biesheuvel (4):
>> >   efi: x86: Use private copy of struct setup_header
>> >   efi: x86: Move PE header after setup header
>> >   efi: x86: Drop alignment section header flags
>> >   efi: x86: Split PE/COFF .text section into .text and .data
>> >
>> >  arch/x86/boot/Makefile                  |  2 +-
>> >  arch/x86/boot/header.S                  | 52 +++++++++-----------
>> >  arch/x86/boot/setup.ld                  |  1 +
>> >  arch/x86/boot/tools/build.c             | 38 +++++++++-----
>> >  drivers/firmware/efi/libstub/x86-stub.c | 43 +++-------------
>> >  5 files changed, 59 insertions(+), 77 deletions(-)
>> 
>> I've quickly looked through these patches but I'll do more testing
>> tomorrow.
>> 
>> This approach seems to be better than mine if it will work. I've tried
>> the similar thing but I did not think of creating the local copy of 
>> the
>> bootparams and the attempt to map them did not work since the PE 
>> loader
>> I am trying to get kernel booting with does not accept sections before
>> the PE header. But since the bootparams is inside the padding and is
>> not used, it should be fine.
>> 
>> But this will still need more changes to work properly with stricter 
>> PE
>> loaders like the one that I've mentioned in my patch series [1].
>> 
>> The image should also have 4K aligned section virtual addresses and
>> sizes
>> (even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory
>> attributes (or refuse to load the kernel).
> 
> EDK2 works fine as is, i.e. with only .text and .data aligned to 4k
> virtually, and the data size of .data aligned to 512 bytes.
> 
> ProtectUefiImageCommon - 0x3C8600C0
>   - 0x0000000038777000 - 0x0000000002BC6000
> SetUefiImageMemoryAttributes - 0x0000000038777000 - 0x0000000000004000
> (0x0000000000004008)
> SetUefiImageMemoryAttributes - 0x000000003877B000 - 0x0000000000BEE000
> (0x0000000000020008)
> SetUefiImageMemoryAttributes - 0x0000000039369000 - 0x0000000001FD4000
> (0x0000000000004008)
> 

Nice to know that. I think .reloc and .compat can be kept small, since
protection for compressed kernel image is getting applied manually 
anyways
(patch "efi/x86: Explicitly set sections memory attributes").
But anyways we can align text/data on 4K by rounding setup size
(or the headers size if setup gets ripped out):

diff --cc arch/x86/boot/tools/build.c
index b449c82feaad,b449c82feaad..535646f283e3
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@@ -502,9 -502,9 +505,11 @@@ static unsigned int read_setup(char *pa
   	file_size += reserve_pecoff_compat_section(file_size);
   	file_size += reserve_pecoff_reloc_section(file_size);

--	/* Pad unused space with zeros */
--
++#ifdef CONFIG_EFI_STUB
++	setup_size = round_up(file_size, 0x1000);
++#else
   	setup_size = round_up(file_size, SECTOR_SIZE);
++#endif

   	if (setup_size < SETUP_SECT_MIN * SECTOR_SIZE)
   		setup_size = SETUP_SECT_MIN * SECTOR_SIZE;

>> Another desired thing is
>> having
>> adjacent section with no padding in between them, since [1] does have 
>> a
>> mode that requires sections them to be adjacent.
> 
> Does that have any basis in the PE/COFF spec?

No, it is not, I think this mode is rather for the internal firmware 
images.
So this would just be nice to have and nothing strongly required.

> 
>> (SizeOfHeaders/header_size
>> should also be set to the size of setup since it is also checked to be
>> adjacent to the first section.)
>> 
> 
> Does that have any basis in the PE/COFF spec?

This is neither.

> 
>> I did not do the one-to-one mapping of file and virtual addresses 
>> since
>> it
>> would require almost 4K paddings for the auxiliary sections.
>> 
>> [1] https://github.com/acidanthera/audk/tree/secure_pe
>> 
> 
> I've backpedaled a little bit from this approach (see my other 
> comment).
> 
> If we just rip out the real mode stub, we can keep the PE header
> before the setup header, and simply describe whatever comes as .text.

That sounds promising. I think the safest way is to make this a compile
time option though, at least as the initial change, so it will not
break any obscure boot loaders. But since modern kernel configurations
are likely won't even fit into the real mode address space, this option
can probably be made mutually exclusive with EFISTUB or
CONFIG_EFI_DXE_MEM_ATTRIBUTES.

Thanks,
Evgeniy Baskov