linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot
@ 2020-02-17 14:48 Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 1/5] efi/x86: Drop redundant .bss section Ard Biesheuvel
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

This series is another part of my effort to reduce the level of knowledge
on the part of the bootloader or firmware of internal per-architecture
details regarding where/how the kernel is loaded and where its initrd and
other context data are passed.

Changes since v1:
- add mixed mode wrappers for the EFI loaded_image protocol (#2)
- expose Exit() boot service and invoke it on errors rather than doing and
  ordinary function return or entering a deadloop (#3)
- move code into .text and r/o data into .rodata where appropriate, and
  disentangle the mixed mode boot path and the ordinary one (#4)
- disable initrd load from file in mixed mode
- reorder patches #4 and #5

---- v1 blurb (cont'd) ---

The x86 architecture has a so-called 'EFI handover protocol', which defines
how the bootparams struct should be populated, and how it should be
interpreted to figure out where to load the kernel, and at which offset in
the binary the entrypoint is located. This scheme allows the initrd to be
loaded beforehand, and allows 32-bit firmware to invoke a 64-bit kernel
via a special entrypoint that manages the state transitions between the
two execution modes.

Due to this, x86 loaders currently do not rely on LoadImage and StartImage,
and therefore, are forced to re-implement things like image authentication
for secure boot and taking the measurements for measured boot in their open
coded clones of these routines.

My previous series on this topic [0][1] implements a generic way to load the
initrd from any source supported by the loader without relying on something
like a device tree or bootparams structure, and so native boot should not
need the EFI handover protocol anymore after those change are merged.

What remains is mixed mode boot, which also needs the EFI handover protocol
regardless of whether an initrd is loaded or not. So let's get rid of that
requirement, and take advantage of the fact that EDK2 based firmware does
support LoadImage() for X64 binaries on IA32 firmware, which means we can
rely on the secure boot and measured boot checks being performed by the
firmware. The only thing we need to put on top is a way to discover the
non-native entrypoint into the binary in a way that does not rely on x86
specific headers and data structures.

So let's introduce a new .compat header in the PE/COFF metadata of the
bzImage, and populate it with a <machine type, entrypoint> tuple, allowing
a generic EFI loader to decide whether the entrypoint supports its native
machine type, and invoke it as an ordinary EFI application entrypoint.
Since we will not be passing a bootparams structure, we need to discover
the base of the image (which contains the setup header) via the loaded
image protocol before we can enter the kernel in 32-bit mode at startup_32()

A loader implementation for OVMF can be found at [2]. Note that this loader
code is fully generic, and permits mixed mode images to be launched from
the UEFI shell or other generic components based on LoadImage/Startimage.
It could be used without modifications if other architectures ever emerge
that support kernels that can be invoked from a non-native (but cross-type
supported) loader.

[0] https://lore.kernel.org/linux-arm-kernel/20200206140352.6300-1-ardb@kernel.org/
[1] https://lore.kernel.org/linux-efi/20200216141104.21477-1-ardb@kernel.org/
[2] https://github.com/ardbiesheuvel/edk2/commits/linux-efi-generic

Cc: lersek@redhat.com
Cc: leif@nuviainc.com
Cc: pjones@redhat.com
Cc: mjg59@google.com
Cc: agraf@csgraf.de
Cc: daniel.kiper@oracle.com
Cc: hdegoede@redhat.com
Cc: nivedita@alum.mit.edu
Cc: mingo@kernel.org

Ard Biesheuvel (5):
  efi/x86: Drop redundant .bss section
  efi/libstub/x86: Make loaded_image protocol handling mixed mode safe
  efi/libstub/x86: Use Exit() boot service to exit the stub on errors
  efi/x86: Implement mixed mode boot without the handover protocol
  efi/x86: Add true mixed mode entry point into .compat section

 arch/x86/boot/Makefile                        |  2 +-
 arch/x86/boot/compressed/head_64.S            | 60 +++++++++++++++-
 arch/x86/boot/header.S                        | 23 +++----
 arch/x86/boot/tools/build.c                   | 69 ++++++++++++-------
 arch/x86/include/asm/efi.h                    |  8 +++
 .../firmware/efi/libstub/efi-stub-helper.c    |  4 +-
 drivers/firmware/efi/libstub/efistub.h        | 50 ++++++++++----
 drivers/firmware/efi/libstub/x86-stub.c       | 53 ++++++++------
 8 files changed, 192 insertions(+), 77 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/5] efi/x86: Drop redundant .bss section
  2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
@ 2020-02-17 14:48 ` Ard Biesheuvel
  2020-02-21 16:40   ` Arvind Sankar
  2020-02-17 14:48 ` [PATCH v2 2/5] efi/libstub/x86: Make loaded_image protocol handling mixed mode safe Ard Biesheuvel
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

In commit

  c7fb93ec51d462ec ("x86/efi: Include a .bss section within the PE/COFF headers")

we added a separate .bss section to the PE/COFF header of the compressed
kernel describing the static memory footprint of the decompressor, to
ensure that it has enough headroom to decompress itself.

We can achieve the exact same result by increasing the virtual size of
the .text section, without changing the raw size, which, as per the
PE/COFF specification, requires the loader to zero initialize the delta.

Doing so frees up a slot in the section table, which we will use later
to describe the mixed mode entrypoint.

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

diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 97d9b6d6c1af..d59f6604bb42 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -106,7 +106,7 @@ coff_header:
 #else
 	.word	0x8664				# x86-64
 #endif
-	.word	4				# nr_sections
+	.word	3				# nr_sections
 	.long	0 				# TimeDateStamp
 	.long	0				# PointerToSymbolTable
 	.long	1				# NumberOfSymbols
@@ -248,25 +248,6 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	0x60500020			# Characteristics (section flags)
 
-	#
-	# The offset & size fields are filled in by build.c.
-	#
-	.ascii	".bss"
-	.byte	0
-	.byte	0
-	.byte	0
-	.byte	0
-	.long	0
-	.long	0x0
-	.long	0				# Size of initialized data
-						# on disk
-	.long	0x0
-	.long	0				# PointerToRelocations
-	.long	0				# PointerToLineNumbers
-	.word	0				# NumberOfRelocations
-	.word	0				# NumberOfLineNumbers
-	.long	0xc8000080			# Characteristics (section flags)
-
 #endif /* CONFIG_EFI_STUB */
 
 	# Kernel attributes; used by setup.  This is part 1 of the
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 55e669d29e54..0c8c5a52f1f0 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -203,10 +203,12 @@ static void update_pecoff_setup_and_reloc(unsigned int size)
 	put_unaligned_le32(10, &buf[reloc_offset + 4]);
 }
 
-static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
+static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
+			       unsigned int init_sz)
 {
 	unsigned int pe_header;
 	unsigned int text_sz = file_sz - text_start;
+	unsigned int bss_sz = init_sz - file_sz;
 
 	pe_header = get_unaligned_le32(&buf[0x3c]);
 
@@ -216,28 +218,19 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
 	 */
 	put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
 
-	/*
-	 * Address of entry point for PE/COFF executable
-	 */
-	put_unaligned_le32(text_start + efi_pe_entry, &buf[pe_header + 0x28]);
-
-	update_pecoff_section_header(".text", text_start, text_sz);
-}
-
-static void update_pecoff_bss(unsigned int file_sz, unsigned int init_sz)
-{
-	unsigned int pe_header;
-	unsigned int bss_sz = init_sz - file_sz;
-
-	pe_header = get_unaligned_le32(&buf[0x3c]);
-
 	/* Size of uninitialized data */
 	put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);
 
 	/* Size of image */
 	put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);
 
-	update_pecoff_section_header_fields(".bss", file_sz, bss_sz, 0, 0);
+	/*
+	 * Address of entry point for PE/COFF executable
+	 */
+	put_unaligned_le32(text_start + efi_pe_entry, &buf[pe_header + 0x28]);
+
+	update_pecoff_section_header_fields(".text", text_start, text_sz + bss_sz,
+					    text_sz, text_start);
 }
 
 static int reserve_pecoff_reloc_section(int c)
@@ -278,9 +271,8 @@ static void efi_stub_entry_update(void)
 
 static inline void update_pecoff_setup_and_reloc(unsigned int size) {}
 static inline void update_pecoff_text(unsigned int text_start,
-				      unsigned int file_sz) {}
-static inline void update_pecoff_bss(unsigned int file_sz,
-				     unsigned int init_sz) {}
+				      unsigned int file_sz,
+				      unsigned int init_sz) {}
 static inline void efi_stub_defaults(void) {}
 static inline void efi_stub_entry_update(void) {}
 
@@ -406,9 +398,8 @@ int main(int argc, char ** argv)
 	buf[0x1f1] = setup_sectors-1;
 	put_unaligned_le32(sys_size, &buf[0x1f4]);
 
-	update_pecoff_text(setup_sectors * 512, i + (sys_size * 16));
 	init_sz = get_unaligned_le32(&buf[0x260]);
-	update_pecoff_bss(i + (sys_size * 16), init_sz);
+	update_pecoff_text(setup_sectors * 512, i + (sys_size * 16), init_sz);
 
 	efi_stub_entry_update();
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/5] efi/libstub/x86: Make loaded_image protocol handling mixed mode safe
  2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 1/5] efi/x86: Drop redundant .bss section Ard Biesheuvel
@ 2020-02-17 14:48 ` Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 3/5] efi/libstub/x86: Use Exit() boot service to exit the stub on errors Ard Biesheuvel
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

Add the definitions and use the special wrapper so that the loaded_image
UEFI protocol can be safely used from mixed mode.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c |  4 +-
 drivers/firmware/efi/libstub/efistub.h         | 45 ++++++++++++++------
 drivers/firmware/efi/libstub/x86-stub.c        |  4 +-
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index d98f9a1bf82c..618c189dd55c 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -171,8 +171,8 @@ char *efi_convert_cmdline(efi_loaded_image_t *image,
 	const u16 *s2;
 	u8 *s1 = NULL;
 	unsigned long cmdline_addr = 0;
-	int load_options_chars = image->load_options_size / 2; /* UTF-16 */
-	const u16 *options = image->load_options;
+	int load_options_chars = efi_table_attr(image, load_options_size) / 2;
+	const u16 *options = efi_table_attr(image, load_options);
 	int options_bytes = 0;  /* UTF-8 bytes */
 	int options_chars = 0;  /* UTF-16 chars */
 	efi_status_t status;
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 2e5e79edb4d7..6960e730f990 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -308,20 +308,37 @@ union efi_graphics_output_protocol {
 	} mixed_mode;
 };
 
-typedef struct {
-	u32			revision;
-	efi_handle_t		parent_handle;
-	efi_system_table_t	*system_table;
-	efi_handle_t		device_handle;
-	void			*file_path;
-	void			*reserved;
-	u32			load_options_size;
-	void			*load_options;
-	void			*image_base;
-	__aligned_u64		image_size;
-	unsigned int		image_code_type;
-	unsigned int		image_data_type;
-	efi_status_t		(__efiapi *unload)(efi_handle_t image_handle);
+typedef union {
+	struct {
+		u32			revision;
+		efi_handle_t		parent_handle;
+		efi_system_table_t	*system_table;
+		efi_handle_t		device_handle;
+		void			*file_path;
+		void			*reserved;
+		u32			load_options_size;
+		void			*load_options;
+		void			*image_base;
+		__aligned_u64		image_size;
+		unsigned int		image_code_type;
+		unsigned int		image_data_type;
+		efi_status_t		(__efiapi *unload)(efi_handle_t image_handle);
+	};
+	struct {
+		u32		revision;
+		u32		parent_handle;
+		u32		system_table;
+		u32		device_handle;
+		u32		file_path;
+		u32		reserved;
+		u32		load_options_size;
+		u32		load_options;
+		u32		image_base;
+		__aligned_u64	image_size;
+		u32		image_code_type;
+		u32		image_data_type;
+		u32		unload;
+	} mixed_mode;
 } efi_loaded_image_t;
 
 typedef struct {
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 7d4866471f86..ce0c3caa3087 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -377,7 +377,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 		return status;
 	}
 
-	hdr = &((struct boot_params *)image->image_base)->hdr;
+	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
 	above4g = hdr->xloadflags & XLF_CAN_BE_LOADED_ABOVE_4G;
 
 	status = efi_allocate_pages(0x4000, (unsigned long *)&boot_params,
@@ -392,7 +392,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	hdr = &boot_params->hdr;
 
 	/* Copy the second sector to boot_params */
-	memcpy(&hdr->jump, image->image_base + 512, 512);
+	memcpy(&hdr->jump, efi_table_attr(image, image_base) + 512, 512);
 
 	/*
 	 * Fill out some of the header fields ourselves because the
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/5] efi/libstub/x86: Use Exit() boot service to exit the stub on errors
  2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 1/5] efi/x86: Drop redundant .bss section Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 2/5] efi/libstub/x86: Make loaded_image protocol handling mixed mode safe Ard Biesheuvel
@ 2020-02-17 14:48 ` Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol Ard Biesheuvel
  2020-02-17 14:48 ` [PATCH v2 5/5] efi/x86: Add true mixed mode entry point into .compat section Ard Biesheuvel
  4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

Currently, we either return with an error [from efi_pe_entry()] or
enter a deadloop [in efi_main()] if any fatal errors occur during
execution of the EFI stub. Let's switch to calling the Exit() EFI boot
service instead in both cases, so that we
a) can get rid of the deadloop, and simply return to the boot manager
   if any errors occur during execution of the stub, including during
   the call to ExitBootServices(),
b) can also return cleanly from efi_pe_entry() or efi_main() in mixed
   mode, once we introduce support for LoadImage/StartImage based mixed
   mode in the next patch.

Note that on systems running downstream GRUBs [which do not use LoadImage
or StartImage to boot the kernel, and instead, pass their own image
handle as the loaded image handle], calling Exit() will exit from GRUB
rather than from the kernel, but this is a tolerable side effect.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/include/asm/efi.h              |  8 ++++++++
 drivers/firmware/efi/libstub/efistub.h  |  5 ++++-
 drivers/firmware/efi/libstub/x86-stub.c | 20 +++++++++++++-------
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index fcb21e3d13c5..05131b962374 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -268,6 +268,11 @@ static inline void *efi64_zero_upper(void *p)
 	return p;
 }
 
+static inline u32 efi64_convert_status(efi_status_t status)
+{
+	return (u32)(status | (u64)status >> 32);
+}
+
 #define __efi64_argmap_free_pages(addr, size)				\
 	((addr), 0, (size))
 
@@ -286,6 +291,9 @@ static inline void *efi64_zero_upper(void *p)
 #define __efi64_argmap_locate_device_path(protocol, path, handle)	\
 	((protocol), (path), efi64_zero_upper(handle))
 
+#define __efi64_argmap_exit(handle, status, size, data)			\
+	((handle), efi64_convert_status(status), (size), (data))
+
 /* PCI I/O */
 #define __efi64_argmap_get_location(protocol, seg, bus, dev, func)	\
 	((protocol), efi64_zero_upper(seg), efi64_zero_upper(bus),	\
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 6960e730f990..cc90a748bcf0 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -144,7 +144,10 @@ union efi_boot_services {
 								     void *);
 		void *load_image;
 		void *start_image;
-		void *exit;
+		efi_status_t __noreturn (__efiapi *exit)(efi_handle_t,
+							 efi_status_t,
+							 unsigned long,
+							 efi_char16_t *);
 		void *unload_image;
 		efi_status_t (__efiapi *exit_boot_services)(efi_handle_t,
 							    unsigned long);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index ce0c3caa3087..cec6baa14d5c 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -340,6 +340,13 @@ static void setup_graphics(struct boot_params *boot_params)
 	}
 }
 
+
+static void __noreturn efi_exit(efi_handle_t handle, efi_status_t status)
+{
+	efi_bs_call(exit, handle, status, 0, NULL);
+	unreachable();
+}
+
 void startup_32(struct boot_params *boot_params);
 
 void __noreturn efi_stub_entry(efi_handle_t handle,
@@ -369,12 +376,12 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
-		return EFI_INVALID_PARAMETER;
+		efi_exit(handle, EFI_INVALID_PARAMETER);
 
 	status = efi_bs_call(handle_protocol, handle, &proto, (void *)&image);
 	if (status != EFI_SUCCESS) {
 		efi_printk("Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
-		return status;
+		efi_exit(handle, status);
 	}
 
 	hdr = &((struct boot_params *)efi_table_attr(image, image_base))->hdr;
@@ -384,7 +391,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 				    above4g ? ULONG_MAX : UINT_MAX);
 	if (status != EFI_SUCCESS) {
 		efi_printk("Failed to allocate lowmem for boot params\n");
-		return status;
+		efi_exit(handle, status);
 	}
 
 	memset(boot_params, 0x0, 0x4000);
@@ -442,7 +449,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 fail:
 	efi_free(0x4000, (unsigned long)boot_params);
 
-	return status;
+	efi_exit(handle, status);
 }
 
 static void add_e820ext(struct boot_params *params,
@@ -709,7 +716,7 @@ struct boot_params *efi_main(efi_handle_t handle,
 
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
-		goto fail;
+		efi_exit(handle, EFI_INVALID_PARAMETER);
 
 	/*
 	 * If the kernel isn't already loaded at the preferred load
@@ -793,6 +800,5 @@ struct boot_params *efi_main(efi_handle_t handle,
 fail:
 	efi_printk("efi_main() failed!\n");
 
-	for (;;)
-		asm("hlt");
+	efi_exit(handle, status);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol
  2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2020-02-17 14:48 ` [PATCH v2 3/5] efi/libstub/x86: Use Exit() boot service to exit the stub on errors Ard Biesheuvel
@ 2020-02-17 14:48 ` Ard Biesheuvel
  2020-02-21 16:39   ` Arvind Sankar
  2020-02-17 14:48 ` [PATCH v2 5/5] efi/x86: Add true mixed mode entry point into .compat section Ard Biesheuvel
  4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

Add support for booting 64-bit x86 kernels from 32-bit firmware running
on 64-bit capable CPUs without requiring the bootloader to implement
the EFI handover protocol or allocate the setup block, etc etc, all of
which can be done by the stub itself, using code that already exists.

Instead, create an ordinary EFI application entrypoint but implemented
in 32-bit code [so that it can be invoked by 32-bit firmware], and stash
the address of this 32-bit entrypoint in the .compat section where the
bootloader can find it.

Note that we use the setup block embedded in the binary to go through
startup_32(), but it gets reallocated and copied in efi_pe_entry(),
using the same code that runs when the x86 kernel is booted in EFI
mode from native firmware. This requires the loaded image protocol to
be installed on the kernel image's EFI handle, and point to the kernel
image itself and not to its loader. This, in turn, requires the
bootloader to use the LoadImage() boot service to load the 64-bit
image from 32-bit firmware, which is in fact supported by firmware
based on EDK2. (Only StartImage() will fail, and instead, the newly
added entrypoint needs to be invoked)

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/head_64.S      | 60 +++++++++++++++++++-
 drivers/firmware/efi/libstub/x86-stub.c | 29 +++++-----
 2 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index a4f5561c1c0e..47a475146107 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -207,8 +207,11 @@ SYM_FUNC_START(startup_32)
 	cmp	$0, %edi
 	jz	1f
 	leal	efi64_stub_entry(%ebp), %eax
-	movl	%esi, %edx
 	movl	efi32_boot_args+4(%ebp), %esi
+	movl	efi32_boot_args+8(%ebp), %edx	// saved bootparams pointer
+	cmpl	$0, %edx
+	jnz	1f
+	leal	efi32_pe_enter64(%ebp), %eax
 1:
 #endif
 	pushl	%eax
@@ -233,6 +236,8 @@ SYM_FUNC_START(efi32_stub_entry)
 1:	pop	%ebp
 	subl	$1b, %ebp
 
+	movl	%esi, efi32_boot_args+8(%ebp)
+SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL)
 	movl	%ecx, efi32_boot_args(%ebp)
 	movl	%edx, efi32_boot_args+4(%ebp)
 	movb	$0, efi_is64(%ebp)
@@ -641,8 +646,59 @@ SYM_DATA_START_LOCAL(gdt)
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
 #ifdef CONFIG_EFI_MIXED
-SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0)
+SYM_DATA_LOCAL(efi32_boot_args, .long 0, 0, 0)
 SYM_DATA(efi_is64, .byte 1)
+
+#define ST32_boottime		60 // offsetof(efi_system_table_32_t, boottime)
+#define BS32_handle_protocol	88 // offsetof(efi_boot_services_32_t, handle_protocol)
+#define LI32_image_base		32 // offsetof(efi_loaded_image_32_t, image_base)
+
+	.text
+	.code32
+SYM_FUNC_START(efi32_pe_entry)
+	pushl	%ebp
+
+	call	1f
+1:	pop	%ebp
+	subl	$1b, %ebp
+
+	/* Get the loaded image protocol pointer from the image handle */
+	subl	$12, %esp			// space for the loaded image pointer
+	pushl	%esp				// pass its address
+	leal	4f(%ebp), %eax
+	pushl	%eax				// pass the GUID address
+	pushl	28(%esp)			// pass the image handle
+
+	movl	36(%esp), %eax			// sys_table
+	movl	ST32_boottime(%eax), %eax	// sys_table->boottime
+	call	*BS32_handle_protocol(%eax)	// sys_table->boottime->handle_protocol
+	cmp	$0, %eax
+	jnz	2f
+
+	movl	32(%esp), %ecx			// image_handle
+	movl	36(%esp), %edx			// sys_table
+	movl	12(%esp), %esi			// loaded_image
+	movl	LI32_image_base(%esi), %esi	// loaded_image->image_base
+	jmp	efi32_pe_stub_entry
+
+2:	addl	$24, %esp
+	popl	%ebp
+	ret
+SYM_FUNC_END(efi32_pe_entry)
+
+	.code64
+SYM_CODE_START_LOCAL(efi32_pe_enter64)
+	movl	%edi, %ecx		// MS calling convention
+	movl	%esi, %edx
+	call	efi_pe_entry
+	/* not reached */
+SYM_CODE_END(efi32_pe_enter64)
+
+	.section ".rodata"
+	/* EFI loaded image protocol GUID */
+4:	.long	0x5B1B31A1
+	.word	0x9562, 0x11d2
+	.byte	0x8E, 0x3F, 0x00, 0xA0, 0xC9, 0x69, 0x72, 0x3B
 #endif
 
 /*
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index cec6baa14d5c..9db98839d7b4 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -424,21 +424,24 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	hdr->ramdisk_image = 0;
 	hdr->ramdisk_size = 0;
 
-	status = efi_parse_options(cmdline_ptr);
-	if (status != EFI_SUCCESS)
-		goto fail2;
-
-	if (!noinitrd()) {
-		status = efi_load_initrd(image, &ramdisk_addr, &ramdisk_size,
-					 hdr->initrd_addr_max,
-					 above4g ? ULONG_MAX
-						 : hdr->initrd_addr_max);
+	if (efi_is_native()) {
+		status = efi_parse_options(cmdline_ptr);
 		if (status != EFI_SUCCESS)
 			goto fail2;
-		hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
-		hdr->ramdisk_size  = ramdisk_size & 0xffffffff;
-		boot_params->ext_ramdisk_image = (u64)ramdisk_addr >> 32;
-		boot_params->ext_ramdisk_size  = (u64)ramdisk_size >> 32;
+
+		if (!noinitrd()) {
+			status = efi_load_initrd(image, &ramdisk_addr,
+						 &ramdisk_size,
+						 hdr->initrd_addr_max,
+						 above4g ? ULONG_MAX
+							 : hdr->initrd_addr_max);
+			if (status != EFI_SUCCESS)
+				goto fail2;
+			hdr->ramdisk_image = ramdisk_addr & 0xffffffff;
+			hdr->ramdisk_size  = ramdisk_size & 0xffffffff;
+			boot_params->ext_ramdisk_image = (u64)ramdisk_addr >> 32;
+			boot_params->ext_ramdisk_size  = (u64)ramdisk_size >> 32;
+		}
 	}
 
 	efi_stub_entry(handle, sys_table, boot_params);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 5/5] efi/x86: Add true mixed mode entry point into .compat section
  2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2020-02-17 14:48 ` [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol Ard Biesheuvel
@ 2020-02-17 14:48 ` Ard Biesheuvel
  4 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-17 14:48 UTC (permalink / raw)
  To: linux-efi
  Cc: Ard Biesheuvel, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

Currently, mixed mode is closely tied to the EFI handover protocol
and relies on intimate knowledge of the bootparams structure, setup
header etc, all of which are rather byzantine and entirely specific
to x86.

Even though no other EFI supported architectures are currently known
that could support something like mixed mode, it still makes sense to
abstract a bit from this, and make it part of a generic Linux on EFI
boot protocol.

To that end, add a .compat section to the mixed mode binary, and populate
it with the PE machine type and entry point address, allowing firmware
implementations to match it to their native machine type, and invoke
non-native binaries using a secondary entry point.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/Makefile      |  2 +-
 arch/x86/boot/header.S      | 20 +++++++++++-
 arch/x86/boot/tools/build.c | 34 +++++++++++++++++++-
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 012b82fc8617..ef9e1f2c836c 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -88,7 +88,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\|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\|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/header.S b/arch/x86/boot/header.S
index d59f6604bb42..76a485013771 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -106,7 +106,7 @@ coff_header:
 #else
 	.word	0x8664				# x86-64
 #endif
-	.word	3				# nr_sections
+	.word	section_count			# nr_sections
 	.long	0 				# TimeDateStamp
 	.long	0				# PointerToSymbolTable
 	.long	1				# NumberOfSymbols
@@ -230,6 +230,23 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	0x42100040			# Characteristics (section flags)
 
+#ifdef CONFIG_EFI_MIXED
+	#
+	# The offset & size fields are filled in by build.c.
+	#
+	.asciz	".compat"
+	.long	0
+	.long	0x0
+	.long	0				# Size of initialized data
+						# on disk
+	.long	0x0
+	.long	0				# PointerToRelocations
+	.long	0				# PointerToLineNumbers
+	.word	0				# NumberOfRelocations
+	.word	0				# NumberOfLineNumbers
+	.long	0x60500020			# Characteristics (section flags)
+#endif
+
 	#
 	# The offset & size fields are filled in by build.c.
 	#
@@ -248,6 +265,7 @@ section_table:
 	.word	0				# NumberOfLineNumbers
 	.long	0x60500020			# Characteristics (section flags)
 
+	.set	section_count, (. - section_table) / 40
 #endif /* CONFIG_EFI_STUB */
 
 	# Kernel attributes; used by setup.  This is part 1 of the
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index 0c8c5a52f1f0..e83144c9db93 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -53,9 +53,16 @@ u8 buf[SETUP_SECT_MAX*512];
 
 #define PECOFF_RELOC_RESERVE 0x20
 
+#ifdef CONFIG_EFI_MIXED
+#define PECOFF_COMPAT_RESERVE 0x20
+#else
+#define PECOFF_COMPAT_RESERVE 0x0
+#endif
+
 unsigned long efi32_stub_entry;
 unsigned long efi64_stub_entry;
 unsigned long efi_pe_entry;
+unsigned long efi32_pe_entry;
 unsigned long kernel_info;
 unsigned long startup_64;
 
@@ -189,7 +196,10 @@ static void update_pecoff_section_header(char *section_name, u32 offset, u32 siz
 static void update_pecoff_setup_and_reloc(unsigned int size)
 {
 	u32 setup_offset = 0x200;
-	u32 reloc_offset = size - PECOFF_RELOC_RESERVE;
+	u32 reloc_offset = size - PECOFF_RELOC_RESERVE - PECOFF_COMPAT_RESERVE;
+#ifdef CONFIG_EFI_MIXED
+	u32 compat_offset = reloc_offset + PECOFF_RELOC_RESERVE;
+#endif
 	u32 setup_size = reloc_offset - setup_offset;
 
 	update_pecoff_section_header(".setup", setup_offset, setup_size);
@@ -201,6 +211,20 @@ static void update_pecoff_setup_and_reloc(unsigned int size)
 	 */
 	put_unaligned_le32(reloc_offset + 10, &buf[reloc_offset]);
 	put_unaligned_le32(10, &buf[reloc_offset + 4]);
+
+#ifdef CONFIG_EFI_MIXED
+	update_pecoff_section_header(".compat", compat_offset, PECOFF_COMPAT_RESERVE);
+
+	/*
+	 * Put the IA-32 machine type (0x14c) and the associated entry point
+	 * address in the .compat section, so loaders can figure out which other
+	 * execution modes this image supports.
+	 */
+	buf[compat_offset] = 0x1;
+	buf[compat_offset + 1] = 0x8;
+	put_unaligned_le16(0x14c, &buf[compat_offset + 2]);
+	put_unaligned_le32(efi32_pe_entry + size, &buf[compat_offset + 4]);
+#endif
 }
 
 static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
@@ -282,6 +306,12 @@ static inline int reserve_pecoff_reloc_section(int c)
 }
 #endif /* CONFIG_EFI_STUB */
 
+static int reserve_pecoff_compat_section(int c)
+{
+	/* Reserve 0x20 bytes for .compat section */
+	memset(buf+c, 0, PECOFF_COMPAT_RESERVE);
+	return PECOFF_COMPAT_RESERVE;
+}
 
 /*
  * Parse zoffset.h and find the entry points. We could just #include zoffset.h
@@ -314,6 +344,7 @@ static void parse_zoffset(char *fname)
 		PARSE_ZOFS(p, efi32_stub_entry);
 		PARSE_ZOFS(p, efi64_stub_entry);
 		PARSE_ZOFS(p, efi_pe_entry);
+		PARSE_ZOFS(p, efi32_pe_entry);
 		PARSE_ZOFS(p, kernel_info);
 		PARSE_ZOFS(p, startup_64);
 
@@ -357,6 +388,7 @@ int main(int argc, char ** argv)
 		die("Boot block hasn't got boot flag (0xAA55)");
 	fclose(file);
 
+	c += reserve_pecoff_compat_section(c);
 	c += reserve_pecoff_reloc_section(c);
 
 	/* Pad unused space with zeros */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol
  2020-02-17 14:48 ` [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol Ard Biesheuvel
@ 2020-02-21 16:39   ` Arvind Sankar
  2020-02-21 17:12     ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-02-21 16:39 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

On Mon, Feb 17, 2020 at 03:48:21PM +0100, Ard Biesheuvel wrote:
> Add support for booting 64-bit x86 kernels from 32-bit firmware running
> on 64-bit capable CPUs without requiring the bootloader to implement
> the EFI handover protocol or allocate the setup block, etc etc, all of
> which can be done by the stub itself, using code that already exists.
> 
> Instead, create an ordinary EFI application entrypoint but implemented
> in 32-bit code [so that it can be invoked by 32-bit firmware], and stash
> the address of this 32-bit entrypoint in the .compat section where the
> bootloader can find it.
> 
> Note that we use the setup block embedded in the binary to go through
> startup_32(), but it gets reallocated and copied in efi_pe_entry(),
> using the same code that runs when the x86 kernel is booted in EFI
> mode from native firmware. This requires the loaded image protocol to
> be installed on the kernel image's EFI handle, and point to the kernel
> image itself and not to its loader. This, in turn, requires the
> bootloader to use the LoadImage() boot service to load the 64-bit
> image from 32-bit firmware, which is in fact supported by firmware
> based on EDK2. (Only StartImage() will fail, and instead, the newly
> added entrypoint needs to be invoked)
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

I think there's one issue with this. startup_32 is 14KiB from the start
of the image because of .setup. This means the code in startup_32 that
rounds the load address up to kernel_alignment will likely calculate it
as 2MiB from the image address (if the image address was 2MiB-aligned),
and the page tables constructed by the 32-bit code will be beyond the
space allocated for the image.

I think the simplest fix would be to increase SizeOfImage by
kernel_alignment to allow enough slop space for the alignment. We should
also increase it by text_start, since we need init_size beginning from
startup_32, not from the image address.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/5] efi/x86: Drop redundant .bss section
  2020-02-17 14:48 ` [PATCH v2 1/5] efi/x86: Drop redundant .bss section Ard Biesheuvel
@ 2020-02-21 16:40   ` Arvind Sankar
  2020-02-21 16:45     ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-02-21 16:40 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-efi, lersek, leif, pjones, mjg59, agraf, daniel.kiper,
	hdegoede, nivedita, mingo

On Mon, Feb 17, 2020 at 03:48:18PM +0100, Ard Biesheuvel wrote:
> In commit
> 
>   c7fb93ec51d462ec ("x86/efi: Include a .bss section within the PE/COFF headers")
> 
> we added a separate .bss section to the PE/COFF header of the compressed
> kernel describing the static memory footprint of the decompressor, to
> ensure that it has enough headroom to decompress itself.
> 
> We can achieve the exact same result by increasing the virtual size of
> the .text section, without changing the raw size, which, as per the
> PE/COFF specification, requires the loader to zero initialize the delta.
> 
> Doing so frees up a slot in the section table, which we will use later
> to describe the mixed mode entrypoint.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/x86/boot/header.S      | 21 +-----------
>  arch/x86/boot/tools/build.c | 35 ++++++++------------
>  2 files changed, 14 insertions(+), 42 deletions(-)
> 
> diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> index 55e669d29e54..0c8c5a52f1f0 100644
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -203,10 +203,12 @@ static void update_pecoff_setup_and_reloc(unsigned int size)
>  	put_unaligned_le32(10, &buf[reloc_offset + 4]);
>  }
>  
> -static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
> +static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
> +			       unsigned int init_sz)
>  {
>  	unsigned int pe_header;
>  	unsigned int text_sz = file_sz - text_start;
> +	unsigned int bss_sz = init_sz - file_sz;
>  
>  	pe_header = get_unaligned_le32(&buf[0x3c]);
>  
> @@ -216,28 +218,19 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
>  	 */
>  	put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
>  
> -	/*
> -	 * Address of entry point for PE/COFF executable
> -	 */
> -	put_unaligned_le32(text_start + efi_pe_entry, &buf[pe_header + 0x28]);
> -
> -	update_pecoff_section_header(".text", text_start, text_sz);
> -}
> -
> -static void update_pecoff_bss(unsigned int file_sz, unsigned int init_sz)
> -{
> -	unsigned int pe_header;
> -	unsigned int bss_sz = init_sz - file_sz;
> -
> -	pe_header = get_unaligned_le32(&buf[0x3c]);
> -
>  	/* Size of uninitialized data */
>  	put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);

Should this still be populated given that there's no .bss section any
more?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/5] efi/x86: Drop redundant .bss section
  2020-02-21 16:40   ` Arvind Sankar
@ 2020-02-21 16:45     ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-21 16:45 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: linux-efi, Laszlo Ersek, Leif Lindholm, Peter Jones,
	Matthew Garrett, Alexander Graf, Daniel Kiper, Hans de Goede,
	Ingo Molnar

On Fri, 21 Feb 2020 at 17:40, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Feb 17, 2020 at 03:48:18PM +0100, Ard Biesheuvel wrote:
> > In commit
> >
> >   c7fb93ec51d462ec ("x86/efi: Include a .bss section within the PE/COFF headers")
> >
> > we added a separate .bss section to the PE/COFF header of the compressed
> > kernel describing the static memory footprint of the decompressor, to
> > ensure that it has enough headroom to decompress itself.
> >
> > We can achieve the exact same result by increasing the virtual size of
> > the .text section, without changing the raw size, which, as per the
> > PE/COFF specification, requires the loader to zero initialize the delta.
> >
> > Doing so frees up a slot in the section table, which we will use later
> > to describe the mixed mode entrypoint.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/header.S      | 21 +-----------
> >  arch/x86/boot/tools/build.c | 35 ++++++++------------
> >  2 files changed, 14 insertions(+), 42 deletions(-)
> >
> > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> > index 55e669d29e54..0c8c5a52f1f0 100644
> > --- a/arch/x86/boot/tools/build.c
> > +++ b/arch/x86/boot/tools/build.c
> > @@ -203,10 +203,12 @@ static void update_pecoff_setup_and_reloc(unsigned int size)
> >       put_unaligned_le32(10, &buf[reloc_offset + 4]);
> >  }
> >
> > -static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
> > +static void update_pecoff_text(unsigned int text_start, unsigned int file_sz,
> > +                            unsigned int init_sz)
> >  {
> >       unsigned int pe_header;
> >       unsigned int text_sz = file_sz - text_start;
> > +     unsigned int bss_sz = init_sz - file_sz;
> >
> >       pe_header = get_unaligned_le32(&buf[0x3c]);
> >
> > @@ -216,28 +218,19 @@ static void update_pecoff_text(unsigned int text_start, unsigned int file_sz)
> >        */
> >       put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
> >
> > -     /*
> > -      * Address of entry point for PE/COFF executable
> > -      */
> > -     put_unaligned_le32(text_start + efi_pe_entry, &buf[pe_header + 0x28]);
> > -
> > -     update_pecoff_section_header(".text", text_start, text_sz);
> > -}
> > -
> > -static void update_pecoff_bss(unsigned int file_sz, unsigned int init_sz)
> > -{
> > -     unsigned int pe_header;
> > -     unsigned int bss_sz = init_sz - file_sz;
> > -
> > -     pe_header = get_unaligned_le32(&buf[0x3c]);
> > -
> >       /* Size of uninitialized data */
> >       put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);
>
> Should this still be populated given that there's no .bss section any
> more?

Good point. The PE/COFF spec is explicit, for a change, and
specifically mentions that this should be the combined sizeof all BSS
sections. It doesn't really specify how one could have multiple BSS
sections, but the wording does support your view that this should be
zero, and the value of bss_sz added to the SizeOfText field.

I couldn't find any code in EDK2 that actually references this field
(apart from the ELF to PE/COFF converter that always sets it to 0x0),
and so I don't think it really matters.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol
  2020-02-21 16:39   ` Arvind Sankar
@ 2020-02-21 17:12     ` Ard Biesheuvel
  2020-02-21 17:59       ` Arvind Sankar
  0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-21 17:12 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: linux-efi, Laszlo Ersek, Leif Lindholm, Peter Jones,
	Matthew Garrett, Alexander Graf, Daniel Kiper, Hans de Goede,
	Ingo Molnar

On Fri, 21 Feb 2020 at 17:39, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Mon, Feb 17, 2020 at 03:48:21PM +0100, Ard Biesheuvel wrote:
> > Add support for booting 64-bit x86 kernels from 32-bit firmware running
> > on 64-bit capable CPUs without requiring the bootloader to implement
> > the EFI handover protocol or allocate the setup block, etc etc, all of
> > which can be done by the stub itself, using code that already exists.
> >
> > Instead, create an ordinary EFI application entrypoint but implemented
> > in 32-bit code [so that it can be invoked by 32-bit firmware], and stash
> > the address of this 32-bit entrypoint in the .compat section where the
> > bootloader can find it.
> >
> > Note that we use the setup block embedded in the binary to go through
> > startup_32(), but it gets reallocated and copied in efi_pe_entry(),
> > using the same code that runs when the x86 kernel is booted in EFI
> > mode from native firmware. This requires the loaded image protocol to
> > be installed on the kernel image's EFI handle, and point to the kernel
> > image itself and not to its loader. This, in turn, requires the
> > bootloader to use the LoadImage() boot service to load the 64-bit
> > image from 32-bit firmware, which is in fact supported by firmware
> > based on EDK2. (Only StartImage() will fail, and instead, the newly
> > added entrypoint needs to be invoked)
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> I think there's one issue with this. startup_32 is 14KiB from the start
> of the image because of .setup. This means the code in startup_32 that
> rounds the load address up to kernel_alignment will likely calculate it
> as 2MiB from the image address (if the image address was 2MiB-aligned),
> and the page tables constructed by the 32-bit code will be beyond the
> space allocated for the image.
>

Right. Image address could be any multiple of 4 KB so we'll have to
deal with that.

> I think the simplest fix would be to increase SizeOfImage by
> kernel_alignment to allow enough slop space for the alignment.

So we basically need at least 2 MB - 14 KB slack at the top, right?
That's easily done.

> We should
> also increase it by text_start, since we need init_size beginning from
> startup_32, not from the image address.

So something like the below?

--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -236,14 +236,23 @@

        pe_header = get_unaligned_le32(&buf[0x3c]);

+#ifdef CONFIG_EFI_MIXED
+       /*
+        * In order for startup_32 to safely execute in place, we need to give
+        * it a bit of headroom to create its page tables.
+        */
+       bss_sz += text_start + CONFIG_PHYSICAL_ALIGN;
+       init_sz += text_start + CONFIG_PHYSICAL_ALIGN;
+#endif
+
        /*
         * Size of code: Subtract the size of the first sector (512 bytes)
         * which includes the header.
         */
-       put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
+       put_unaligned_le32(file_sz + bss_sz- 512, &buf[pe_header + 0x1c]);

        /* Size of uninitialized data */
-       put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);
+       put_unaligned_le32(0, &buf[pe_header + 0x24]);

        /* Size of image */
        put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol
  2020-02-21 17:12     ` Ard Biesheuvel
@ 2020-02-21 17:59       ` Arvind Sankar
  2020-02-21 18:54         ` Ard Biesheuvel
  0 siblings, 1 reply; 12+ messages in thread
From: Arvind Sankar @ 2020-02-21 17:59 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Arvind Sankar, linux-efi, Laszlo Ersek, Leif Lindholm,
	Peter Jones, Matthew Garrett, Alexander Graf, Daniel Kiper,
	Hans de Goede, Ingo Molnar

On Fri, Feb 21, 2020 at 06:12:40PM +0100, Ard Biesheuvel wrote:
> On Fri, 21 Feb 2020 at 17:39, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> >
> > On Mon, Feb 17, 2020 at 03:48:21PM +0100, Ard Biesheuvel wrote:
> > > Add support for booting 64-bit x86 kernels from 32-bit firmware running
> > > on 64-bit capable CPUs without requiring the bootloader to implement
> > > the EFI handover protocol or allocate the setup block, etc etc, all of
> > > which can be done by the stub itself, using code that already exists.
> > >
> > > Instead, create an ordinary EFI application entrypoint but implemented
> > > in 32-bit code [so that it can be invoked by 32-bit firmware], and stash
> > > the address of this 32-bit entrypoint in the .compat section where the
> > > bootloader can find it.
> > >
> > > Note that we use the setup block embedded in the binary to go through
> > > startup_32(), but it gets reallocated and copied in efi_pe_entry(),
> > > using the same code that runs when the x86 kernel is booted in EFI
> > > mode from native firmware. This requires the loaded image protocol to
> > > be installed on the kernel image's EFI handle, and point to the kernel
> > > image itself and not to its loader. This, in turn, requires the
> > > bootloader to use the LoadImage() boot service to load the 64-bit
> > > image from 32-bit firmware, which is in fact supported by firmware
> > > based on EDK2. (Only StartImage() will fail, and instead, the newly
> > > added entrypoint needs to be invoked)
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > I think there's one issue with this. startup_32 is 14KiB from the start
> > of the image because of .setup. This means the code in startup_32 that
> > rounds the load address up to kernel_alignment will likely calculate it
> > as 2MiB from the image address (if the image address was 2MiB-aligned),
> > and the page tables constructed by the 32-bit code will be beyond the
> > space allocated for the image.
> >
> 
> Right. Image address could be any multiple of 4 KB so we'll have to
> deal with that.
> 
> > I think the simplest fix would be to increase SizeOfImage by
> > kernel_alignment to allow enough slop space for the alignment.
> 
> So we basically need at least 2 MB - 14 KB slack at the top, right?
> That's easily done.
> 
> > We should
> > also increase it by text_start, since we need init_size beginning from
> > startup_32, not from the image address.
> 
> So something like the below?

Yup.

You might as well do the text_start bit unconditionally I think? If by
some blind stroke of luck startup_32 ends up at pref_address and so we
don't call efi_relocate_kernel, we'll need the room.

> 
> --- a/arch/x86/boot/tools/build.c
> +++ b/arch/x86/boot/tools/build.c
> @@ -236,14 +236,23 @@
> 
>         pe_header = get_unaligned_le32(&buf[0x3c]);
> 
> +#ifdef CONFIG_EFI_MIXED
> +       /*
> +        * In order for startup_32 to safely execute in place, we need to give
> +        * it a bit of headroom to create its page tables.
> +        */
> +       bss_sz += text_start + CONFIG_PHYSICAL_ALIGN;
> +       init_sz += text_start + CONFIG_PHYSICAL_ALIGN;
> +#endif
> +
>         /*
>          * Size of code: Subtract the size of the first sector (512 bytes)
>          * which includes the header.
>          */
> -       put_unaligned_le32(file_sz - 512, &buf[pe_header + 0x1c]);
> +       put_unaligned_le32(file_sz + bss_sz- 512, &buf[pe_header + 0x1c]);
> 
>         /* Size of uninitialized data */
> -       put_unaligned_le32(bss_sz, &buf[pe_header + 0x24]);
> +       put_unaligned_le32(0, &buf[pe_header + 0x24]);
> 
>         /* Size of image */
>         put_unaligned_le32(init_sz, &buf[pe_header + 0x50]);

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol
  2020-02-21 17:59       ` Arvind Sankar
@ 2020-02-21 18:54         ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-02-21 18:54 UTC (permalink / raw)
  To: Arvind Sankar
  Cc: linux-efi, Laszlo Ersek, Leif Lindholm, Peter Jones,
	Matthew Garrett, Alexander Graf, Daniel Kiper, Hans de Goede,
	Ingo Molnar

On Fri, 21 Feb 2020 at 18:59, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Fri, Feb 21, 2020 at 06:12:40PM +0100, Ard Biesheuvel wrote:
> > On Fri, 21 Feb 2020 at 17:39, Arvind Sankar <nivedita@alum.mit.edu> wrote:
> > >
> > > On Mon, Feb 17, 2020 at 03:48:21PM +0100, Ard Biesheuvel wrote:
> > > > Add support for booting 64-bit x86 kernels from 32-bit firmware running
> > > > on 64-bit capable CPUs without requiring the bootloader to implement
> > > > the EFI handover protocol or allocate the setup block, etc etc, all of
> > > > which can be done by the stub itself, using code that already exists.
> > > >
> > > > Instead, create an ordinary EFI application entrypoint but implemented
> > > > in 32-bit code [so that it can be invoked by 32-bit firmware], and stash
> > > > the address of this 32-bit entrypoint in the .compat section where the
> > > > bootloader can find it.
> > > >
> > > > Note that we use the setup block embedded in the binary to go through
> > > > startup_32(), but it gets reallocated and copied in efi_pe_entry(),
> > > > using the same code that runs when the x86 kernel is booted in EFI
> > > > mode from native firmware. This requires the loaded image protocol to
> > > > be installed on the kernel image's EFI handle, and point to the kernel
> > > > image itself and not to its loader. This, in turn, requires the
> > > > bootloader to use the LoadImage() boot service to load the 64-bit
> > > > image from 32-bit firmware, which is in fact supported by firmware
> > > > based on EDK2. (Only StartImage() will fail, and instead, the newly
> > > > added entrypoint needs to be invoked)
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > I think there's one issue with this. startup_32 is 14KiB from the start
> > > of the image because of .setup. This means the code in startup_32 that
> > > rounds the load address up to kernel_alignment will likely calculate it
> > > as 2MiB from the image address (if the image address was 2MiB-aligned),
> > > and the page tables constructed by the 32-bit code will be beyond the
> > > space allocated for the image.
> > >
> >
> > Right. Image address could be any multiple of 4 KB so we'll have to
> > deal with that.
> >
> > > I think the simplest fix would be to increase SizeOfImage by
> > > kernel_alignment to allow enough slop space for the alignment.
> >
> > So we basically need at least 2 MB - 14 KB slack at the top, right?
> > That's easily done.
> >
> > > We should
> > > also increase it by text_start, since we need init_size beginning from
> > > startup_32, not from the image address.
> >
> > So something like the below?
>
> Yup.
>
> You might as well do the text_start bit unconditionally I think? If by
> some blind stroke of luck startup_32 ends up at pref_address and so we
> don't call efi_relocate_kernel, we'll need the room.
>

Yeah, so this could already be an issue today ...

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-02-21 18:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 14:48 [PATCH v2 0/5] efi/x86: add support for generic EFI mixed mode boot Ard Biesheuvel
2020-02-17 14:48 ` [PATCH v2 1/5] efi/x86: Drop redundant .bss section Ard Biesheuvel
2020-02-21 16:40   ` Arvind Sankar
2020-02-21 16:45     ` Ard Biesheuvel
2020-02-17 14:48 ` [PATCH v2 2/5] efi/libstub/x86: Make loaded_image protocol handling mixed mode safe Ard Biesheuvel
2020-02-17 14:48 ` [PATCH v2 3/5] efi/libstub/x86: Use Exit() boot service to exit the stub on errors Ard Biesheuvel
2020-02-17 14:48 ` [PATCH v2 4/5] efi/x86: Implement mixed mode boot without the handover protocol Ard Biesheuvel
2020-02-21 16:39   ` Arvind Sankar
2020-02-21 17:12     ` Ard Biesheuvel
2020-02-21 17:59       ` Arvind Sankar
2020-02-21 18:54         ` Ard Biesheuvel
2020-02-17 14:48 ` [PATCH v2 5/5] efi/x86: Add true mixed mode entry point into .compat section Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).