lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 20 Oct 2022 15:36:36 +0300 From: Evgeniy Baskov <baskov@...ras.ru> To: Ard Biesheuvel <ardb@...nel.org> Cc: Borislav Petkov <bp@...en8.de>, Andy Lutomirski <luto@...nel.org>, Dave Hansen <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Thomas Gleixner <tglx@...utronix.de>, Alexey Khoroshilov <khoroshilov@...ras.ru>, lvc-project@...uxtesting.org, x86@...nel.org, linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH 13/16] efi/x86: Support extracting kernel from libstub On 2022-10-19 10:35, Ard Biesheuvel wrote: > On Tue, 6 Sept 2022 at 12:42, Evgeniy Baskov <baskov@...ras.ru> 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@...ras.ru> >> >> 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.
Powered by blists - more mailing lists