| 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