Re: [PATCH 13/16] efi/x86: Support extracting kernel from libstub
From: Evgeniy Baskov
Date: Thu Oct 20 2022 - 08:37:02 EST
On 2022-10-19 10:35, Ard Biesheuvel wrote:
On Tue, 6 Sept 2022 at 12:42, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
Doing it that way allows setting up stricter memory attributes,
simplifies boot code path and removes potential relocation
of kernel image.
Wire up required interfaces and minimally initialize zero page
fields needed for it to function correctly.
Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
create mode 100644 arch/x86/include/asm/shared/extract.h
create mode 100644 drivers/firmware/efi/libstub/x86-extract-direct.c
---
arch/x86/boot/compressed/head_32.S | 6 +-
arch/x86/boot/compressed/head_64.S | 45 ++++
arch/x86/include/asm/shared/extract.h | 25 ++
drivers/firmware/efi/Kconfig | 14 ++
drivers/firmware/efi/libstub/Makefile | 1 +
drivers/firmware/efi/libstub/efistub.h | 5 +
.../firmware/efi/libstub/x86-extract-direct.c | 220
++++++++++++++++++
drivers/firmware/efi/libstub/x86-stub.c | 45 ++--
8 files changed, 343 insertions(+), 18 deletions(-)
create mode 100644 arch/x86/include/asm/shared/extract.h
create mode 100644 drivers/firmware/efi/libstub/x86-extract-direct.c
diff --git a/arch/x86/boot/compressed/head_32.S
b/arch/x86/boot/compressed/head_32.S
index b46a1c4109cf..d2866f06bc9f 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -155,7 +155,11 @@ SYM_FUNC_START(efi32_stub_entry)
add $0x4, %esp
movl 8(%esp), %esi /* save boot_params pointer */
call efi_main
- /* efi_main returns the possibly relocated address of
startup_32 */
+
+ /*
+ * efi_main returns the possibly
+ * relocated address of exteracted kernel entry point.
extracted
Thanks, will fix.
...
diff --git a/arch/x86/include/asm/shared/extract.h
b/arch/x86/include/asm/shared/extract.h
new file mode 100644
index 000000000000..163678145884
--- /dev/null
+++ b/arch/x86/include/asm/shared/extract.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef ASM_SHARED_EXTRACT_H
+#define ASM_SHARED_EXTRACT_H
+
+#define MAP_WRITE 0x02 /* Writable memory */
+#define MAP_EXEC 0x04 /* Executable memory */
+#define MAP_ALLOC 0x10 /* Range needs to be allocated */
+#define MAP_PROTECT 0x20 /* Set exact memory attributes for memory
range */
+
+struct efi_iofunc {
+ void (*putstr)(const char *msg);
+ void (*puthex)(unsigned long x);
+ unsigned long (*map_range)(unsigned long start,
+ unsigned long end,
+ unsigned int flags);
This looks a bit random - having a map_range() routine as a member of
the console I/O struct. Can we make this abstraction a bit more
natural?
Hmm, I can either change the name of this stucture
to something more generic (like efi_extract_callbacks) or
split map_range separately as a separate function argument.
(Renaming seems simpler, so I will do that for now.)
+};
+
+void *efi_extract_kernel(struct boot_params *rmode,
+ struct efi_iofunc *iofunc,
+ unsigned char *input_data,
+ unsigned long input_len,
+ unsigned char *output,
+ unsigned long output_len);
+
+#endif /* ASM_SHARED_EXTRACT_H */
diff --git a/drivers/firmware/efi/Kconfig
b/drivers/firmware/efi/Kconfig
index 6cb7384ad2ac..2418402a0bda 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -91,6 +91,20 @@ config EFI_DXE_MEM_ATTRIBUTES
Use DXE services to check and alter memory protection
attributes during boot via EFISTUB to ensure that memory
ranges used by the kernel are writable and executable.
+ This option also enables stricter memory attributes
+ on compressed kernel PE image.
+
+config EFI_STUB_EXTRACT_DIRECT
+ bool "Extract kernel directly from UEFI environment"
+ depends on EFI && EFI_STUB && X86_64
+ default y
What is the reason for making this configurable? Couldn't we just
enable it unconditionally?
When I first implemented it it was too hackish, but now it seems OK, so
I can make it unconditional, and it will make things simpler in several
places. Although making it work on x86_32 will require some additional
work.
Also kernel with EFI_STUB_EXTRACT_DIRECT disabled breaks boot process
with Mu
firmware when W^X enabled, as pointed out by Peter.
So, I guess, I will just remove the switch.
+#ifdef CONFIG_X86
+unsigned long extract_kernel_direct(struct boot_params *boot_params);
+void startup_32(struct boot_params *boot_params);
+#endif
+
Please put this somewhere else
Will adding little x86-specific header file for these be appropriate?
+
+#include "efistub.h"
+
+static void do_puthex(unsigned long value);
+static void do_putstr(const char *msg);
+
Can we get rid of these forward declarations?
Yes, I will move those functions here and remove declarations.
...
+ /* First page of trampoline is a top level page table */
+ efi_adjust_memory_range_protection(trampoline_start,
+ PAGE_SIZE,
+ EFI_MEMORY_XP);
+
+ /* Second page of trampoline is the code (with a padding) */
+ status = efi_get_memory_map(&map);
efi_get_memory_map() has been updated in the mean time, so this needs a
rewrite.
Yep, it needs a rebase now.
...
setup_memory_protection(unsigned long image_base, unsigned long
image_size)
{
/*
- * Allow execution of possible trampoline used
- * for switching between 4- and 5-level page tables
- * and relocated kernel image.
- */
+ * Allow execution of possible trampoline used
+ * for switching between 4- and 5-level page tables
+ * and relocated kernel image.
+ */
Drop this hunk please
That was unintentional, thanks.