LinuxLists.cc - [PATCH v2 0/5] Couple of bugfixes to sev-es series

[permalink] [raw]

Subject: [PATCH v2 2/5] x86/boot: Split out command-line related declarations

Move prototypes for command-line related functions into a new header
file to split it out from misc.h.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/acpi.c | 1 +
arch/x86/boot/compressed/cmdline.c | 1 +
arch/x86/boot/compressed/cmdline.h | 13 +++++++++++++
arch/x86/boot/compressed/early_serial_console.c | 1 +
arch/x86/boot/compressed/kaslr.c | 7 +------
arch/x86/boot/compressed/misc.h | 4 ----
arch/x86/boot/compressed/pgtable_64.c | 2 +-
7 files changed, 18 insertions(+), 11 deletions(-)
create mode 100644 arch/x86/boot/compressed/cmdline.h

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 8bcbcee54aa1..9097108c37e1 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -3,6 +3,7 @@
#include "misc.h"
#include "error.h"
#include "../string.h"
+#include "cmdline.h"

#include <linux/numa.h>
#include <linux/efi.h>
diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..20f2e6d8b891 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include "misc.h"
+#include "cmdline.h"

static unsigned long fs;
static inline void set_fs(unsigned long seg)
diff --git a/arch/x86/boot/compressed/cmdline.h b/arch/x86/boot/compressed/cmdline.h
new file mode 100644
index 000000000000..72800770bd60
--- /dev/null
+++ b/arch/x86/boot/compressed/cmdline.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef BOOT_COMPRESSED_CMDLINE_H
+#define BOOT_COMPRESSED_CMDLINE_H
+
+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+unsigned long get_cmd_line_ptr(void);
+int cmdline_find_option(const char *option, char *buffer, int bufsize);
+int cmdline_find_option_bool(const char *option);
+
+#endif /* BOOT_COMPRESSED_CMDLINE_H */
diff --git a/arch/x86/boot/compressed/early_serial_console.c b/arch/x86/boot/compressed/early_serial_console.c
index 261e81fb9582..64a1f557e122 100644
--- a/arch/x86/boot/compressed/early_serial_console.c
+++ b/arch/x86/boot/compressed/early_serial_console.c
@@ -1,4 +1,5 @@
#include "misc.h"
+#include "cmdline.h"

int early_serial_base;

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b59547ce5b19..489fabc051d7 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -22,6 +22,7 @@
#include "misc.h"
#include "error.h"
#include "../string.h"
+#include "cmdline.h"

#include <generated/compile.h>
#include <linux/module.h>
@@ -36,12 +37,6 @@
#define STATIC
#include <linux/decompress/mm.h>

-#define _SETUP
-#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
-#undef _SETUP
-
-extern unsigned long get_cmd_line_ptr(void);
-
/* Simplified build-specific string for starting entropy. */
static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 6d31f1b4c4d1..e3e2f312c025 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -69,10 +69,6 @@ static inline void debug_puthex(unsigned long value)

#endif

-/* cmdline.c */
-int cmdline_find_option(const char *option, char *buffer, int bufsize);
-int cmdline_find_option_bool(const char *option);
-
struct mem_vector {
u64 start;
u64 size;
diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 0fb948c0c8b4..46d761f7faa6 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -4,6 +4,7 @@
#include <asm/efi.h>
#include "pgtable.h"
#include "../string.h"
+#include "cmdline.h"

#define BIOS_START_MIN 0x20000U /* 128K, less than this is insane */
#define BIOS_START_MAX 0x9f000U /* 640K, absolute maximum */
@@ -26,7 +27,6 @@ static char trampoline_save[TRAMPOLINE_32BIT_SIZE];
unsigned long *trampoline_32bit __section(.data);

extern struct boot_params *boot_params;
-int cmdline_find_option_bool(const char *option);

static unsigned long find_trampoline_placement(void)
{
--
2.26.2

2020-10-08 19:23:00

[permalink] [raw]

Subject: [PATCH v2 3/5] x86/boot/64: Show original faulting address in case of error

This makes the #PF handler print the original CR2 value in case of
error, instead of after aligning to PMD_SIZE.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/ident_map_64.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 063a60edcf99..fd957b2625e9 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -327,9 +327,6 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)

ghcb_fault = sev_es_check_ghcb_fault(address);

- address &= PMD_MASK;
- end = address + PMD_SIZE;
-
/*
* Check for unexpected error codes. Unexpected are:
* - Faults on present pages
@@ -345,5 +342,8 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
* Error code is sane - now identity map the 2M region around
* the faulting address.
*/
+ address &= PMD_MASK;
+ end = address + PMD_SIZE;
+
add_identity_map(address, end);
}
--
2.26.2

2020-10-08 19:23:14

[permalink] [raw]

Subject: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

Commits

ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
8570978ea030 ("x86/boot/compressed/64: Don't pre-map memory in KASLR code")

set up a new page table in the decompressor stub, but without explicit
mappings for boot_params and the kernel command line, relying on the #PF
handler instead.

This is fragile, as boot_params and the command line mappings are
required for the main kernel. If EARLY_PRINTK and RANDOMIZE_BASE are
disabled, a QEMU/OVMF boot never accesses the command line in the
decompressor stub, and so it never gets mapped. The main kernel accesses
it from the identity mapping if AMD_MEM_ENCRYPT is enabled, and will
crash.

Fix this by adding back the explicit mapping of boot_params and the
command line.

Note: the changes also removed the explicit mapping of the main kernel,
with the result that .bss and .brk may not be in the identity mapping,
but those don't get accessed by the main kernel before it switches to
its own page tables.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/ident_map_64.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index fd957b2625e9..a3613857c532 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -21,6 +21,7 @@

#include "error.h"
#include "misc.h"
+#include "cmdline.h"

/* These actually do the work of building the kernel identity maps. */
#include <linux/pgtable.h>
@@ -109,6 +110,8 @@ static void add_identity_map(unsigned long start, unsigned long end)
/* Locates and clears a region for a new top level page table. */
void initialize_identity_maps(void)
{
+ unsigned long cmdline;
+
/* Exclude the encryption mask from __PHYSICAL_MASK */
physical_mask &= ~sme_me_mask;

@@ -149,10 +152,19 @@ void initialize_identity_maps(void)
}

/*
- * New page-table is set up - map the kernel image and load it
- * into cr3.
+ * New page-table is set up - map the kernel image, boot_params and the
+ * command line.
+ * The uncompressed kernel requires boot_params and the command line to
+ * be mapped in the identity mapping.
+ * Map them explicitly here in case the compressed kernel does not
+ * touch them, or does not touch all the pages covering them.
*/
add_identity_map((unsigned long)_head, (unsigned long)_end);
+ add_identity_map((unsigned long)boot_params, (unsigned long)(boot_params + 1));
+ cmdline = get_cmd_line_ptr();
+ add_identity_map(cmdline, cmdline + COMMAND_LINE_SIZE);
+
+ /* Load the new page-table. */
write_cr3(top_level_pgt);
}

--
2.26.2

2020-10-08 19:23:21

[permalink] [raw]

Subject: [PATCH v2 1/5] x86/boot: Initialize boot_params in startup code

Save the boot_params pointer passed in by the bootloader in
startup_32/64. This avoids having to initialize it in two different
places in C code, and having to preserve SI through the early assembly
code.

Move boot_params from .bss to .data, since .bss will be cleared after
the kernel is relocated.

Signed-off-by: Arvind Sankar <[email protected]>
Reviewed-by: Joerg Roedel <[email protected]>
---
arch/x86/boot/compressed/head_32.S | 12 +++++----
arch/x86/boot/compressed/head_64.S | 35 +++++++++------------------
arch/x86/boot/compressed/misc.c | 10 +-------
arch/x86/boot/compressed/pgtable_64.c | 5 +---
4 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 659fad53ca82..9b5c88d8b290 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -113,6 +113,9 @@ SYM_FUNC_START(startup_32)
addl BP_init_size(%esi), %ebx
subl $_end@GOTOFF, %ebx

+ /* Initialize boot_params */
+ movl %esi, boot_params@GOTOFF(%edx)
+
/* Set up the stack */
leal boot_stack_end@GOTOFF(%ebx), %esp

@@ -124,7 +127,6 @@ SYM_FUNC_START(startup_32)
* Copy the compressed kernel to the end of our buffer
* where decompression in place becomes safe.
*/
- pushl %esi
leal (_bss@GOTOFF-4)(%edx), %esi
leal (_bss@GOTOFF-4)(%ebx), %edi
movl $(_bss - startup_32), %ecx
@@ -132,7 +134,6 @@ SYM_FUNC_START(startup_32)
std
rep movsl
cld
- popl %esi

/*
* The GDT may get overwritten either during the copy we just did or
@@ -187,14 +188,12 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
pushl %eax /* input_data */
leal boot_heap@GOTOFF(%ebx), %eax
pushl %eax /* heap area */
- pushl %esi /* real mode pointer */
call extract_kernel /* returns kernel location in %eax */
- addl $24, %esp

/*
* Jump to the extracted kernel.
*/
- xorl %ebx, %ebx
+ movl boot_params@GOTOFF(%ebx), %esi
jmp *%eax
SYM_FUNC_END(.Lrelocated)

@@ -209,6 +208,9 @@ SYM_DATA_START_LOCAL(gdt)
.quad 0x00cf92000000ffff /* __KERNEL_DS */
SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)

+/* boot_params must be in .data because .bss is cleared after the kernel is relocated */
+SYM_DATA(boot_params, .long 0)
+
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 1c80f1738fd9..521b41ca14fe 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -375,6 +375,9 @@ SYM_CODE_START(startup_64)
subl $ rva(_end), %ebx
addq %rbp, %rbx

+ /* Initialize boot_params */
+ movq %rsi, boot_params(%rip)
+
/* Set up the stack */
leaq rva(boot_stack_end)(%rbx), %rsp

@@ -429,14 +432,8 @@ SYM_CODE_START(startup_64)
* - Address of the trampoline is returned in RAX.
* - Non zero RDX means trampoline needs to enable 5-level
* paging.
- *
- * RSI holds real mode data and needs to be preserved across
- * this function call.
*/
- pushq %rsi
- movq %rsi, %rdi /* real mode address */
call paging_prepare
- popq %rsi

/* Save the trampoline address in RCX */
movq %rax, %rcx
@@ -461,14 +458,9 @@ trampoline_return:
*
* RDI is address of the page table to use instead of page table
* in trampoline memory (if required).
- *
- * RSI holds real mode data and needs to be preserved across
- * this function call.
*/
- pushq %rsi
leaq rva(top_pgtable)(%rbx), %rdi
call cleanup_trampoline
- popq %rsi

/* Zero EFLAGS */
pushq $0
@@ -478,7 +470,6 @@ trampoline_return:
* Copy the compressed kernel to the end of our buffer
* where decompression in place becomes safe.
*/
- pushq %rsi
leaq (_bss-8)(%rip), %rsi
leaq rva(_bss-8)(%rbx), %rdi
movl $(_bss - startup_32), %ecx
@@ -486,7 +477,6 @@ trampoline_return:
std
rep movsq
cld
- popq %rsi

/*
* The GDT may get overwritten either during the copy we just did or
@@ -541,28 +531,24 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
* handler. Then load stage2 IDT and switch to the kernel's own
* page-table.
*/
- pushq %rsi
call set_sev_encryption_mask
call load_stage2_idt
call initialize_identity_maps
- popq %rsi

/*
* Do the extraction, and jump to the new kernel..
*/
- pushq %rsi /* Save the real mode argument */
- movq %rsi, %rdi /* real mode address */
- leaq boot_heap(%rip), %rsi /* malloc area for uncompression */
- leaq input_data(%rip), %rdx /* input_data */
- movl input_len(%rip), %ecx /* input_len */
- movq %rbp, %r8 /* output target address */
- movl output_len(%rip), %r9d /* decompressed length, end of relocs */
+ leaq boot_heap(%rip), %rdi /* malloc area for uncompression */
+ leaq input_data(%rip), %rsi /* input_data */
+ movl input_len(%rip), %edx /* input_len */
+ movq %rbp, %rcx /* output target address */
+ movl output_len(%rip), %r8d /* decompressed length, end of relocs */
call extract_kernel /* returns kernel location in %rax */
- popq %rsi

/*
* Jump to the decompressed kernel.
*/
+ movq boot_params(%rip), %rsi
jmp *%rax
SYM_FUNC_END(.Lrelocated)

@@ -691,6 +677,9 @@ SYM_DATA_START(boot_idt)
.endr
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

+/* boot_params must be in .data because .bss is cleared after the kernel is relocated */
+SYM_DATA(boot_params, .quad 0)
+
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 267e7f93050e..279631650bd8 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -39,11 +39,6 @@
/* Functions used by the included decompressor code below. */
void *memmove(void *dest, const void *src, size_t n);

-/*
- * This is set up by the setup-routine at boot-time
- */
-struct boot_params *boot_params;
-
memptr free_mem_ptr;
memptr free_mem_end_ptr;

@@ -338,7 +333,7 @@ static void parse_elf(void *output)
* |-------uncompressed kernel image---------|
*
*/
-asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
+asmlinkage __visible void *extract_kernel(memptr heap,
unsigned char *input_data,
unsigned long input_len,
unsigned char *output,
@@ -348,9 +343,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
unsigned long needed_size;

- /* Retain x86 boot parameters pointer passed from startup_32/64. */
- boot_params = rmode;
-
/* Clear flags intended for solely in-kernel use. */
boot_params->hdr.loadflags &= ~KASLR_FLAG;

diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 7d0394f4ebf9..0fb948c0c8b4 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -98,13 +98,10 @@ static unsigned long find_trampoline_placement(void)
return bios_start - TRAMPOLINE_32BIT_SIZE;
}

-struct paging_config paging_prepare(void *rmode)
+struct paging_config paging_prepare(void)
{
struct paging_config paging_config = {};

- /* Initialize boot_params. Required for cmdline_find_option_bool(). */
- boot_params = rmode;
-
/*
* Check if LA57 is desired and supported.
*
--
2.26.2

2020-10-08 19:24:30

[permalink] [raw]

Subject: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On 64-bit, the startup_64_setup_env() function added in
866b556efa12 ("x86/head/64: Install startup GDT")
has stack protection enabled because of set_bringup_idt_handler().

At this point, %gs is not yet initialized, and this doesn't cause a
crash only because the #PF handler from the decompressor stub is still
installed and handles the page fault.

Disable stack protection for the whole file, and do it on 32-bit as
well to avoid surprises.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/kernel/Makefile | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 04ceea8f4a89..68608bd892c0 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -47,6 +47,8 @@ endif
# non-deterministic coverage.
KCOV_INSTRUMENT := n

+CFLAGS_head$(BITS).o += -fno-stack-protector
+
CFLAGS_irq.o := -I $(srctree)/$(src)/../include/asm/trace

obj-y := process_$(BITS).o signal.o
--
2.26.2

2020-10-09 14:44:45

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] x86/boot/64: Show original faulting address in case of error

On Thu, Oct 08, 2020 at 03:16:21PM -0400, Arvind Sankar wrote:
> This makes the #PF handler print the original CR2 value in case of
> error, instead of after aligning to PMD_SIZE.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> arch/x86/boot/compressed/ident_map_64.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> index 063a60edcf99..fd957b2625e9 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -327,9 +327,6 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
>
> ghcb_fault = sev_es_check_ghcb_fault(address);
>
> - address &= PMD_MASK;
> - end = address + PMD_SIZE;
> -
> /*
> * Check for unexpected error codes. Unexpected are:
> * - Faults on present pages
> @@ -345,5 +342,8 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
> * Error code is sane - now identity map the 2M region around
> * the faulting address.
> */
> + address &= PMD_MASK;
> + end = address + PMD_SIZE;
> +
> add_identity_map(address, end);
> }

Reviewed-by: Joerg Roedel <[email protected]>

2020-10-09 14:50:59

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Thu, Oct 08, 2020 at 03:16:22PM -0400, Arvind Sankar wrote:
> Commits
>
> ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
> 8570978ea030 ("x86/boot/compressed/64: Don't pre-map memory in KASLR code")
>
> set up a new page table in the decompressor stub, but without explicit
> mappings for boot_params and the kernel command line, relying on the #PF
> handler instead.
>
> This is fragile, as boot_params and the command line mappings are
> required for the main kernel. If EARLY_PRINTK and RANDOMIZE_BASE are
> disabled, a QEMU/OVMF boot never accesses the command line in the
> decompressor stub, and so it never gets mapped. The main kernel accesses
> it from the identity mapping if AMD_MEM_ENCRYPT is enabled, and will
> crash.
>
> Fix this by adding back the explicit mapping of boot_params and the
> command line.
>
> Note: the changes also removed the explicit mapping of the main kernel,
> with the result that .bss and .brk may not be in the identity mapping,
> but those don't get accessed by the main kernel before it switches to
> its own page tables.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> arch/x86/boot/compressed/ident_map_64.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> index fd957b2625e9..a3613857c532 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -21,6 +21,7 @@
>
> #include "error.h"
> #include "misc.h"
> +#include "cmdline.h"
>
> /* These actually do the work of building the kernel identity maps. */
> #include <linux/pgtable.h>
> @@ -109,6 +110,8 @@ static void add_identity_map(unsigned long start, unsigned long end)
> /* Locates and clears a region for a new top level page table. */
> void initialize_identity_maps(void)
> {
> + unsigned long cmdline;
> +
> /* Exclude the encryption mask from __PHYSICAL_MASK */
> physical_mask &= ~sme_me_mask;
>
> @@ -149,10 +152,19 @@ void initialize_identity_maps(void)
> }
>
> /*
> - * New page-table is set up - map the kernel image and load it
> - * into cr3.
> + * New page-table is set up - map the kernel image, boot_params and the
> + * command line.
> + * The uncompressed kernel requires boot_params and the command line to
> + * be mapped in the identity mapping.
> + * Map them explicitly here in case the compressed kernel does not
> + * touch them, or does not touch all the pages covering them.
> */
> add_identity_map((unsigned long)_head, (unsigned long)_end);
> + add_identity_map((unsigned long)boot_params, (unsigned long)(boot_params + 1));
> + cmdline = get_cmd_line_ptr();
> + add_identity_map(cmdline, cmdline + COMMAND_LINE_SIZE);
> +
> + /* Load the new page-table. */
> write_cr3(top_level_pgt);
> }

Reviewed-by: Joerg Roedel <[email protected]>

2020-10-09 14:55:13

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On Thu, Oct 08, 2020 at 03:16:23PM -0400, Arvind Sankar wrote:
> On 64-bit, the startup_64_setup_env() function added in
> 866b556efa12 ("x86/head/64: Install startup GDT")
> has stack protection enabled because of set_bringup_idt_handler().
>
> At this point, %gs is not yet initialized, and this doesn't cause a
> crash only because the #PF handler from the decompressor stub is still
> installed and handles the page fault.
>
> Disable stack protection for the whole file, and do it on 32-bit as
> well to avoid surprises.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> arch/x86/kernel/Makefile | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 04ceea8f4a89..68608bd892c0 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -47,6 +47,8 @@ endif
> # non-deterministic coverage.
> KCOV_INSTRUMENT := n
>
> +CFLAGS_head$(BITS).o += -fno-stack-protector
> +
> CFLAGS_irq.o := -I $(srctree)/$(src)/../include/asm/trace
>
> obj-y := process_$(BITS).o signal.o
> --
> 2.26.2

Reviewed-by: Joerg Roedel <[email protected]>

2020-10-10 23:17:53

[permalink] [raw]

Subject: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

Commit
ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
started using a new set of pagetables even without KASLR.

After that commit, initialize_identity_maps() is called before the
5-level paging variables are setup in choose_random_location(), which
will not work if 5-level paging is actually enabled.

Fix this by moving the initialization of __pgtable_l5_enabled,
pgdir_shift and ptrs_per_p4d into cleanup_trampoline(), which is called
immediately after the finalization of whether the kernel is executing
with 4- or 5-level paging. This will be earlier than anything that might
require those variables, and keeps the 4- vs 5-level paging code all in
one place.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/ident_map_64.c | 6 ------
arch/x86/boot/compressed/kaslr.c | 8 --------
arch/x86/boot/compressed/pgtable_64.c | 16 ++++++++++++++++
3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index a3613857c532..505d6299b76e 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -34,12 +34,6 @@
#define __PAGE_OFFSET __PAGE_OFFSET_BASE
#include "../../mm/ident_map.c"

-#ifdef CONFIG_X86_5LEVEL
-unsigned int __pgtable_l5_enabled;
-unsigned int pgdir_shift = 39;
-unsigned int ptrs_per_p4d = 1;
-#endif
-
/* Used by PAGE_KERN* macros: */
pteval_t __default_kernel_pte_mask __read_mostly = ~0;

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 489fabc051d7..9eabd8bc7673 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -835,14 +835,6 @@ void choose_random_location(unsigned long input,
return;
}

-#ifdef CONFIG_X86_5LEVEL
- if (__read_cr4() & X86_CR4_LA57) {
- __pgtable_l5_enabled = 1;
- pgdir_shift = 48;
- ptrs_per_p4d = 512;
- }
-#endif
-
boot_params->hdr.loadflags |= KASLR_FLAG;

if (IS_ENABLED(CONFIG_X86_32))
diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 46d761f7faa6..0976c2d2ab2f 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -9,6 +9,13 @@
#define BIOS_START_MIN 0x20000U /* 128K, less than this is insane */
#define BIOS_START_MAX 0x9f000U /* 640K, absolute maximum */

+#ifdef CONFIG_X86_5LEVEL
+/* __pgtable_l5_enabled needs to be in .data to avoid being cleared along with .bss */
+unsigned int __section(.data) __pgtable_l5_enabled;
+unsigned int __section(.data) pgdir_shift = 39;
+unsigned int __section(.data) ptrs_per_p4d = 1;
+#endif
+
struct paging_config {
unsigned long trampoline_start;
unsigned long l5_required;
@@ -195,4 +202,13 @@ void cleanup_trampoline(void *pgtable)

/* Restore trampoline memory */
memcpy(trampoline_32bit, trampoline_save, TRAMPOLINE_32BIT_SIZE);
+
+ /* Initialize variables for 5-level paging */
+#ifdef CONFIG_X86_5LEVEL
+ if (__read_cr4() & X86_CR4_LA57) {
+ __pgtable_l5_enabled = 1;
+ pgdir_shift = 48;
+ ptrs_per_p4d = 512;
+ }
+#endif
}
--
2.26.2

2020-10-13 12:00:11

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Tue, Oct 13, 2020 at 11:20:47AM +0300, Kirill A. Shutemov wrote:
> With TCG or KVM? I use -machine "type=q35,accel=tcg".

Thx, that triggered it - it was KVM before.

Btw, are 5level boxes shipping already or not yet? Because I've not seen
one yet. If you have access to the hw, I'd appreciate it if you could
test Arvind's patch ontop of

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=x86/seves

which does that early pagetable rework.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-13 12:00:52

by Kirill A. Shutemov

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Tue, Oct 13, 2020 at 10:11:17AM +0200, Borislav Petkov wrote:
> On Mon, Oct 12, 2020 at 11:35:01AM -0400, Arvind Sankar wrote:
> > > qemu supports it. -cpu "qemu64,+la57"
> >
> > Thanks! On QEMU, it does crash without this patch.
>
> Works fine here. I gave this to qemu:
>
> -cpu qemu64,+la57,vendor=GenuineIntel
>
> it said:
>
> qemu-system-x86_64: warning: host doesn't support requested feature: CPUID.07H:ECX.la57 [bit 16]
>
> because host is AMD, probably, and I have
>
> CONFIG_X86_5LEVEL=y
>
> enabled for the guest kernel and tip:x86/seves booted fine.
>
> What am I missing?

With TCG or KVM? I use -machine "type=q35,accel=tcg".

--
Kirill A. Shutemov

2020-10-13 12:02:15

by Kirill A. Shutemov

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Tue, Oct 13, 2020 at 10:33:58AM +0200, Borislav Petkov wrote:
> On Tue, Oct 13, 2020 at 11:20:47AM +0300, Kirill A. Shutemov wrote:
> > With TCG or KVM? I use -machine "type=q35,accel=tcg".
>
> Thx, that triggered it - it was KVM before.
>
> Btw, are 5level boxes shipping already or not yet?

Not yet.

> Because I've not seen one yet. If you have access to the hw, I'd
> appreciate it if you could test Arvind's patch ontop of
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=x86/seves
>
> which does that early pagetable rework.

Okay, will do.

--
Kirill A. Shutemov

2020-10-13 12:02:56

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

Hi Arvind,

On Sat, Oct 10, 2020 at 03:11:10PM -0400, Arvind Sankar wrote:
> Commit
> ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
> started using a new set of pagetables even without KASLR.
>
> After that commit, initialize_identity_maps() is called before the
> 5-level paging variables are setup in choose_random_location(), which
> will not work if 5-level paging is actually enabled.
>
> Fix this by moving the initialization of __pgtable_l5_enabled,
> pgdir_shift and ptrs_per_p4d into cleanup_trampoline(), which is called
> immediately after the finalization of whether the kernel is executing
> with 4- or 5-level paging. This will be earlier than anything that might
> require those variables, and keeps the 4- vs 5-level paging code all in
> one place.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> arch/x86/boot/compressed/ident_map_64.c | 6 ------
> arch/x86/boot/compressed/kaslr.c | 8 --------
> arch/x86/boot/compressed/pgtable_64.c | 16 ++++++++++++++++
> 3 files changed, 16 insertions(+), 14 deletions(-)

Thanks for fixing this! It is not only a fix but also a nice cleanup of
the 5level-paging initialization code.

Reviewed-by: Joerg Roedel <[email protected]>
Tested-by: Joerg Roedel <[email protected]>

2020-10-13 12:03:04

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Tue, Oct 13, 2020 at 12:12:28PM +0300, Kirill A. Shutemov wrote:
> Not yet.

Oh good, that makes this fix a lot less critical then.

> Okay, will do.

Thx!

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-15 13:54:59

by Kirill A. Shutemov

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Tue, Oct 13, 2020 at 10:33:58AM +0200, Borislav Petkov wrote:
> On Tue, Oct 13, 2020 at 11:20:47AM +0300, Kirill A. Shutemov wrote:
> > With TCG or KVM? I use -machine "type=q35,accel=tcg".
>
> Thx, that triggered it - it was KVM before.
>
> Btw, are 5level boxes shipping already or not yet? Because I've not seen
> one yet. If you have access to the hw, I'd appreciate it if you could
> test Arvind's patch ontop of
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=x86/seves
>
> which does that early pagetable rework.

Yes, the patch helps to fix 5-level paging boot:

Tested-by: Kirill A. Shutemov <[email protected]>

--
Kirill A. Shutemov

2020-10-16 10:24:52

[permalink] [raw]

Subject: Re: [PATCH] x86/boot/64: Initialize 5-level paging variables earlier

On Thu, Oct 15, 2020 at 04:52:09PM +0300, Kirill A. Shutemov wrote:
> Yes, the patch helps to fix 5-level paging boot:
>
> Tested-by: Kirill A. Shutemov <[email protected]>

Thx!

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 13:09:52

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On Thu, Oct 08, 2020 at 03:16:23PM -0400, Arvind Sankar wrote:
> On 64-bit, the startup_64_setup_env() function added in
> 866b556efa12 ("x86/head/64: Install startup GDT")
> has stack protection enabled because of set_bringup_idt_handler().

Where? I don't see it.

I have

CONFIG_STACKPROTECTOR=y
# CONFIG_STACKPROTECTOR_STRONG is not set

and a __stack_chk_fail call is nowhere to be found in the resulting
head64.s file.

startup_64_setup_env:
# arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
leaq startup_gdt(%rdi), %rax #, tmp99
# arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
subq $_text, %rax #, tmp101
movq %rax, startup_gdt_descr+2(%rip) # tmp101, startup_gdt_descr.address
# ./arch/x86/include/asm/desc.h:209: asm volatile("lgdt %0"::"m" (*dtr));
#APP
# 209 "./arch/x86/include/asm/desc.h" 1
lgdt startup_gdt_descr(%rip) # startup_gdt_descr
# 0 "" 2
# arch/x86/kernel/head64.c:600: asm volatile("movl %%eax, %%ds\n"
#NO_APP
movl $24, %eax #, tmp102
#APP
# 600 "arch/x86/kernel/head64.c" 1
movl %eax, %ds
movl %eax, %ss
movl %eax, %es

# 0 "" 2
# arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
#NO_APP
leaq bringup_idt_table(%rdi), %r9 #, tmp105
leaq bringup_idt_descr(%rdi), %r8 #, tmp103
leaq vc_no_ghcb(%rdi), %rsi #, tmp107
# arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
subq $_text, %r9 #, _11
subq $_text, %r8 #, _8
subq $_text, %rsi #, tmp109
# arch/x86/kernel/head64.c:572: set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
movq %r9, %rdi # _11,
call set_bringup_idt_handler.constprop.0 #
# arch/x86/kernel/head64.c:575: desc->address = (unsigned long)idt;
movq %r9, 2(%r8) # _11, MEM[(struct desc_ptr *)_8].address
# ./arch/x86/include/asm/desc.h:214: asm volatile("lidt %0"::"m" (*dtr));
#APP
# 214 "./arch/x86/include/asm/desc.h" 1
lidt (%r8) # MEM[(const struct desc_ptr *)_8]
# 0 "" 2
# arch/x86/kernel/head64.c:605: }
#NO_APP
ret

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 13:15:32

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On Fri, Oct 16, 2020 at 01:17:03PM +0200, Borislav Petkov wrote:
> On Thu, Oct 08, 2020 at 03:16:23PM -0400, Arvind Sankar wrote:
> > On 64-bit, the startup_64_setup_env() function added in
> > 866b556efa12 ("x86/head/64: Install startup GDT")
> > has stack protection enabled because of set_bringup_idt_handler().
>
> Where? I don't see it.
>
> I have
>
> CONFIG_STACKPROTECTOR=y
> # CONFIG_STACKPROTECTOR_STRONG is not set

You need STACKPROTECTOR_STRONG -- I was testing with defconfig and the
option is enabled by default. You also need AMD_MEM_ENCRYPT enabled,
which it looks like you do.

>
> and a __stack_chk_fail call is nowhere to be found in the resulting
> head64.s file.
>
> startup_64_setup_env:
> # arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
> leaq startup_gdt(%rdi), %rax #, tmp99
> # arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
> subq $_text, %rax #, tmp101
> movq %rax, startup_gdt_descr+2(%rip) # tmp101, startup_gdt_descr.address
> # ./arch/x86/include/asm/desc.h:209: asm volatile("lgdt %0"::"m" (*dtr));
> #APP
> # 209 "./arch/x86/include/asm/desc.h" 1
> lgdt startup_gdt_descr(%rip) # startup_gdt_descr
> # 0 "" 2
> # arch/x86/kernel/head64.c:600: asm volatile("movl %%eax, %%ds\n"
> #NO_APP
> movl $24, %eax #, tmp102
> #APP
> # 600 "arch/x86/kernel/head64.c" 1
> movl %eax, %ds
> movl %eax, %ss
> movl %eax, %es
>
> # 0 "" 2
> # arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
> #NO_APP
> leaq bringup_idt_table(%rdi), %r9 #, tmp105
> leaq bringup_idt_descr(%rdi), %r8 #, tmp103
> leaq vc_no_ghcb(%rdi), %rsi #, tmp107
> # arch/x86/kernel/head64.c:91: return ptr - (void *)_text + (void *)physaddr;
> subq $_text, %r9 #, _11
> subq $_text, %r8 #, _8
> subq $_text, %rsi #, tmp109
> # arch/x86/kernel/head64.c:572: set_bringup_idt_handler(idt, X86_TRAP_VC, handler);
> movq %r9, %rdi # _11,
> call set_bringup_idt_handler.constprop.0 #
> # arch/x86/kernel/head64.c:575: desc->address = (unsigned long)idt;
> movq %r9, 2(%r8) # _11, MEM[(struct desc_ptr *)_8].address
> # ./arch/x86/include/asm/desc.h:214: asm volatile("lidt %0"::"m" (*dtr));
> #APP
> # 214 "./arch/x86/include/asm/desc.h" 1
> lidt (%r8) # MEM[(const struct desc_ptr *)_8]
> # 0 "" 2
> # arch/x86/kernel/head64.c:605: }
> #NO_APP
> ret
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 13:22:59

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On Fri, Oct 16, 2020 at 08:43:01AM -0400, Arvind Sankar wrote:
> You need STACKPROTECTOR_STRONG -- I was testing with defconfig and the
> option is enabled by default.

And you need to write those things in the commit messages.

Please, for the future, always make sure that all required ingredients
for triggering a bug are documented in the commit message, before
sending a fix. Jörg and I were both scratching heads on how you're
reproducing this.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 15:47:18

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] x86/head/64: Disable stack protection for head$(BITS).o

On Fri, Oct 16, 2020 at 03:15:45PM +0200, Borislav Petkov wrote:
> On Fri, Oct 16, 2020 at 08:43:01AM -0400, Arvind Sankar wrote:
> > You need STACKPROTECTOR_STRONG -- I was testing with defconfig and the
> > option is enabled by default.
>
> And you need to write those things in the commit messages.
>
> Please, for the future, always make sure that all required ingredients
> for triggering a bug are documented in the commit message, before
> sending a fix. Jörg and I were both scratching heads on how you're
> reproducing this.
>
> Thx.
>

Sorry about that. This config option I didn't notice before since I
hadn't explicitly enabled the option, but I should have been more clear
about how I reproduced. I actually came across this from the other end,
was looking at the disassembly, saw the stack check call, and then spent
a while debugging why it did _not_ just always crash.

Thanks.

2020-10-16 17:05:58

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 06:27:59PM +0200, Borislav Petkov wrote:
> On Thu, Oct 08, 2020 at 03:16:22PM -0400, Arvind Sankar wrote:
> > Signed-off-by: Arvind Sankar <[email protected]>
> > ---
> > arch/x86/boot/compressed/ident_map_64.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
>
> Ok, just pushed your two fixes here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc0%2b1-seves
>
> Please rebase this one ontop and put the two cleanups last so that the
> fixes can go into urgent while the two cleanups can follow later through
> normal tip flow. And yes, pls do this in the future too: fixes should
> be minimal so that they can be expedited first and normal cleanups and
> features can be done later.
>

Just for clarity, by cleanups you mean patches 2 and 3? i.e. you want to
see 1, 4, 2, 3?

Thanks.

2020-10-16 17:16:52

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 12:47:55PM -0400, Arvind Sankar wrote:
> Just for clarity, by cleanups you mean patches 2 and 3? i.e. you want to
> see 1, 4, 2, 3?

It is important for:

[PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

to come first so that it goes in now.

The rest:

[PATCH v2 1/5] x86/boot: Initialize boot_params in startup code
[PATCH v2 2/5] x86/boot: Split out command-line related declarations
[PATCH v2 3/5] x86/boot/64: Show original faulting address in case of error

can come in any order and when ready.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 17:23:06

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 07:07:42PM +0200, Borislav Petkov wrote:
> On Fri, Oct 16, 2020 at 12:47:55PM -0400, Arvind Sankar wrote:
> > Just for clarity, by cleanups you mean patches 2 and 3? i.e. you want to
> > see 1, 4, 2, 3?
>
> It is important for:
>
> [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line
>
> to come first so that it goes in now.

This patch depends on 1 to initialize boot_params before
initialize_identity_maps() is called. You want me to rework it to avoid
that?

>
> The rest:
>
> [PATCH v2 1/5] x86/boot: Initialize boot_params in startup code
> [PATCH v2 2/5] x86/boot: Split out command-line related declarations
> [PATCH v2 3/5] x86/boot/64: Show original faulting address in case of error
>
> can come in any order and when ready.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 19:57:04

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Thu, Oct 08, 2020 at 03:16:22PM -0400, Arvind Sankar wrote:
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> arch/x86/boot/compressed/ident_map_64.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)

Ok, just pushed your two fixes here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc0%2b1-seves

Please rebase this one ontop and put the two cleanups last so that the
fixes can go into urgent while the two cleanups can follow later through
normal tip flow. And yes, pls do this in the future too: fixes should
be minimal so that they can be expedited first and normal cleanups and
features can be done later.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 19:58:22

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 01:20:58PM -0400, Arvind Sankar wrote:
> This patch depends on 1 to initialize boot_params before
> initialize_identity_maps() is called. You want me to rework it to avoid
> that?

Yes please. If it doesn't become too ugly, that is. If it does, then
we'll have to think of something else...

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-16 20:11:35

[permalink] [raw]

Subject: [PATCH v3 1/4] x86/boot/64: Explicitly map boot_params and command line

Commits

ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
8570978ea030 ("x86/boot/compressed/64: Don't pre-map memory in KASLR code")

set up a new page table in the decompressor stub, but without explicit
mappings for boot_params and the kernel command line, relying on the #PF
handler instead.

This is fragile, as boot_params and the command line mappings are
required for the main kernel. If EARLY_PRINTK and RANDOMIZE_BASE are
disabled, a QEMU/OVMF boot never accesses the command line in the
decompressor stub, and so it never gets mapped. The main kernel accesses
it from the identity mapping if AMD_MEM_ENCRYPT is enabled, and will
crash.

Fix this by adding back the explicit mapping of boot_params and the
command line.

Note: the changes also removed the explicit mapping of the main kernel,
with the result that .bss and .brk may not be in the identity mapping,
but those don't get accessed by the main kernel before it switches to
its own page tables.

Signed-off-by: Arvind Sankar <[email protected]>
Reviewed-by: Joerg Roedel <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 3 +++
arch/x86/boot/compressed/ident_map_64.c | 24 +++++++++++++++++++++---
2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 1c80f1738fd9..3976b4e92e1b 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -544,6 +544,9 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
pushq %rsi
call set_sev_encryption_mask
call load_stage2_idt
+ /* Pass boot_params to initialize_identity_maps */
+ popq %rdi
+ pushq %rdi
call initialize_identity_maps
popq %rsi

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index c6f7aef7e85a..bf61581277c2 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -33,6 +33,12 @@
#define __PAGE_OFFSET __PAGE_OFFSET_BASE
#include "../../mm/ident_map.c"

+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+extern unsigned long get_cmd_line_ptr(void);
+
/* Used by PAGE_KERN* macros: */
pteval_t __default_kernel_pte_mask __read_mostly = ~0;

@@ -101,8 +107,10 @@ static void add_identity_map(unsigned long start, unsigned long end)
}

/* Locates and clears a region for a new top level page table. */
-void initialize_identity_maps(void)
+void initialize_identity_maps(void *rmode)
{
+ unsigned long cmdline;
+
/* Exclude the encryption mask from __PHYSICAL_MASK */
physical_mask &= ~sme_me_mask;

@@ -143,10 +151,20 @@ void initialize_identity_maps(void)
}

/*
- * New page-table is set up - map the kernel image and load it
- * into cr3.
+ * New page-table is set up - map the kernel image, boot_params and the
+ * command line.
+ * The uncompressed kernel requires boot_params and the command line to
+ * be mapped in the identity mapping.
+ * Map them explicitly here in case the compressed kernel does not
+ * touch them, or does not touch all the pages covering them.
*/
add_identity_map((unsigned long)_head, (unsigned long)_end);
+ boot_params = rmode;
+ add_identity_map((unsigned long)boot_params, (unsigned long)(boot_params + 1));
+ cmdline = get_cmd_line_ptr();
+ add_identity_map(cmdline, cmdline + COMMAND_LINE_SIZE);
+
+ /* Load the new page-table. */
write_cr3(top_level_pgt);
}

--
2.26.2

2020-10-16 22:17:34

[permalink] [raw]

Subject: [PATCH v3 2/4] x86/boot: Initialize boot_params in startup code

Save the boot_params pointer passed in by the bootloader in
startup_32/64. This avoids having to initialize it in multiple places in
C code, and having to preserve SI through the early assembly code.

Move boot_params from .bss to .data, since .bss will be cleared after
the kernel is relocated.

Signed-off-by: Arvind Sankar <[email protected]>
Reviewed-by: Joerg Roedel <[email protected]>
---
arch/x86/boot/compressed/head_32.S | 12 ++++----
arch/x86/boot/compressed/head_64.S | 38 ++++++++-----------------
arch/x86/boot/compressed/ident_map_64.c | 3 +-
arch/x86/boot/compressed/misc.c | 10 +------
arch/x86/boot/compressed/pgtable_64.c | 5 +---
5 files changed, 22 insertions(+), 46 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 659fad53ca82..9b5c88d8b290 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -113,6 +113,9 @@ SYM_FUNC_START(startup_32)
addl BP_init_size(%esi), %ebx
subl $_end@GOTOFF, %ebx

+ /* Initialize boot_params */
+ movl %esi, boot_params@GOTOFF(%edx)
+
/* Set up the stack */
leal boot_stack_end@GOTOFF(%ebx), %esp

@@ -124,7 +127,6 @@ SYM_FUNC_START(startup_32)
* Copy the compressed kernel to the end of our buffer
* where decompression in place becomes safe.
*/
- pushl %esi
leal (_bss@GOTOFF-4)(%edx), %esi
leal (_bss@GOTOFF-4)(%ebx), %edi
movl $(_bss - startup_32), %ecx
@@ -132,7 +134,6 @@ SYM_FUNC_START(startup_32)
std
rep movsl
cld
- popl %esi

/*
* The GDT may get overwritten either during the copy we just did or
@@ -187,14 +188,12 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
pushl %eax /* input_data */
leal boot_heap@GOTOFF(%ebx), %eax
pushl %eax /* heap area */
- pushl %esi /* real mode pointer */
call extract_kernel /* returns kernel location in %eax */
- addl $24, %esp

/*
* Jump to the extracted kernel.
*/
- xorl %ebx, %ebx
+ movl boot_params@GOTOFF(%ebx), %esi
jmp *%eax
SYM_FUNC_END(.Lrelocated)

@@ -209,6 +208,9 @@ SYM_DATA_START_LOCAL(gdt)
.quad 0x00cf92000000ffff /* __KERNEL_DS */
SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)

+/* boot_params must be in .data because .bss is cleared after the kernel is relocated */
+SYM_DATA(boot_params, .long 0)
+
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 3976b4e92e1b..521b41ca14fe 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -375,6 +375,9 @@ SYM_CODE_START(startup_64)
subl $ rva(_end), %ebx
addq %rbp, %rbx

+ /* Initialize boot_params */
+ movq %rsi, boot_params(%rip)
+
/* Set up the stack */
leaq rva(boot_stack_end)(%rbx), %rsp

@@ -429,14 +432,8 @@ SYM_CODE_START(startup_64)
* - Address of the trampoline is returned in RAX.
* - Non zero RDX means trampoline needs to enable 5-level
* paging.
- *
- * RSI holds real mode data and needs to be preserved across
- * this function call.
*/
- pushq %rsi
- movq %rsi, %rdi /* real mode address */
call paging_prepare
- popq %rsi

/* Save the trampoline address in RCX */
movq %rax, %rcx
@@ -461,14 +458,9 @@ trampoline_return:
*
* RDI is address of the page table to use instead of page table
* in trampoline memory (if required).
- *
- * RSI holds real mode data and needs to be preserved across
- * this function call.
*/
- pushq %rsi
leaq rva(top_pgtable)(%rbx), %rdi
call cleanup_trampoline
- popq %rsi

/* Zero EFLAGS */
pushq $0
@@ -478,7 +470,6 @@ trampoline_return:
* Copy the compressed kernel to the end of our buffer
* where decompression in place becomes safe.
*/
- pushq %rsi
leaq (_bss-8)(%rip), %rsi
leaq rva(_bss-8)(%rbx), %rdi
movl $(_bss - startup_32), %ecx
@@ -486,7 +477,6 @@ trampoline_return:
std
rep movsq
cld
- popq %rsi

/*
* The GDT may get overwritten either during the copy we just did or
@@ -541,31 +531,24 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
* handler. Then load stage2 IDT and switch to the kernel's own
* page-table.
*/
- pushq %rsi
call set_sev_encryption_mask
call load_stage2_idt
- /* Pass boot_params to initialize_identity_maps */
- popq %rdi
- pushq %rdi
call initialize_identity_maps
- popq %rsi

/*
* Do the extraction, and jump to the new kernel..
*/
- pushq %rsi /* Save the real mode argument */
- movq %rsi, %rdi /* real mode address */
- leaq boot_heap(%rip), %rsi /* malloc area for uncompression */
- leaq input_data(%rip), %rdx /* input_data */
- movl input_len(%rip), %ecx /* input_len */
- movq %rbp, %r8 /* output target address */
- movl output_len(%rip), %r9d /* decompressed length, end of relocs */
+ leaq boot_heap(%rip), %rdi /* malloc area for uncompression */
+ leaq input_data(%rip), %rsi /* input_data */
+ movl input_len(%rip), %edx /* input_len */
+ movq %rbp, %rcx /* output target address */
+ movl output_len(%rip), %r8d /* decompressed length, end of relocs */
call extract_kernel /* returns kernel location in %rax */
- popq %rsi

/*
* Jump to the decompressed kernel.
*/
+ movq boot_params(%rip), %rsi
jmp *%rax
SYM_FUNC_END(.Lrelocated)

@@ -694,6 +677,9 @@ SYM_DATA_START(boot_idt)
.endr
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

+/* boot_params must be in .data because .bss is cleared after the kernel is relocated */
+SYM_DATA(boot_params, .quad 0)
+
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index bf61581277c2..b679908c120e 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -107,7 +107,7 @@ static void add_identity_map(unsigned long start, unsigned long end)
}

/* Locates and clears a region for a new top level page table. */
-void initialize_identity_maps(void *rmode)
+void initialize_identity_maps(void)
{
unsigned long cmdline;

@@ -159,7 +159,6 @@ void initialize_identity_maps(void *rmode)
* touch them, or does not touch all the pages covering them.
*/
add_identity_map((unsigned long)_head, (unsigned long)_end);
- boot_params = rmode;
add_identity_map((unsigned long)boot_params, (unsigned long)(boot_params + 1));
cmdline = get_cmd_line_ptr();
add_identity_map(cmdline, cmdline + COMMAND_LINE_SIZE);
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 267e7f93050e..279631650bd8 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -39,11 +39,6 @@
/* Functions used by the included decompressor code below. */
void *memmove(void *dest, const void *src, size_t n);

-/*
- * This is set up by the setup-routine at boot-time
- */
-struct boot_params *boot_params;
-
memptr free_mem_ptr;
memptr free_mem_end_ptr;

@@ -338,7 +333,7 @@ static void parse_elf(void *output)
* |-------uncompressed kernel image---------|
*
*/
-asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
+asmlinkage __visible void *extract_kernel(memptr heap,
unsigned char *input_data,
unsigned long input_len,
unsigned char *output,
@@ -348,9 +343,6 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
unsigned long needed_size;

- /* Retain x86 boot parameters pointer passed from startup_32/64. */
- boot_params = rmode;
-
/* Clear flags intended for solely in-kernel use. */
boot_params->hdr.loadflags &= ~KASLR_FLAG;

diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 5def1674d6f1..25add5510edc 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -105,13 +105,10 @@ static unsigned long find_trampoline_placement(void)
return bios_start - TRAMPOLINE_32BIT_SIZE;
}

-struct paging_config paging_prepare(void *rmode)
+struct paging_config paging_prepare(void)
{
struct paging_config paging_config = {};

- /* Initialize boot_params. Required for cmdline_find_option_bool(). */
- boot_params = rmode;
-
/*
* Check if LA57 is desired and supported.
*
--
2.26.2

2020-10-16 22:17:34

[permalink] [raw]

Subject: [PATCH v3 4/4] x86/boot/64: Show original faulting address in case of error

This makes the #PF handler print the original CR2 value in case of
error, instead of after aligning to PMD_SIZE.

Signed-off-by: Arvind Sankar <[email protected]>
Reviewed-by: Joerg Roedel <[email protected]>
---
arch/x86/boot/compressed/ident_map_64.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 06ebe5e3e489..505d6299b76e 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -333,9 +333,6 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)

ghcb_fault = sev_es_check_ghcb_fault(address);

- address &= PMD_MASK;
- end = address + PMD_SIZE;
-
/*
* Check for unexpected error codes. Unexpected are:
* - Faults on present pages
@@ -351,5 +348,8 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
* Error code is sane - now identity map the 2M region around
* the faulting address.
*/
+ address &= PMD_MASK;
+ end = address + PMD_SIZE;
+
add_identity_map(address, end);
}
--
2.26.2

2020-10-16 22:17:34

[permalink] [raw]

Subject: [PATCH v3 3/4] x86/boot: Split out command-line related declarations

Move prototypes for command-line related functions into a new header
file to split it out from misc.h.

Signed-off-by: Arvind Sankar <[email protected]>
---
arch/x86/boot/compressed/acpi.c | 1 +
arch/x86/boot/compressed/cmdline.c | 1 +
arch/x86/boot/compressed/cmdline.h | 13 +++++++++++++
arch/x86/boot/compressed/early_serial_console.c | 1 +
arch/x86/boot/compressed/ident_map_64.c | 7 +------
arch/x86/boot/compressed/kaslr.c | 7 +------
arch/x86/boot/compressed/misc.h | 4 ----
arch/x86/boot/compressed/pgtable_64.c | 2 +-
8 files changed, 19 insertions(+), 17 deletions(-)
create mode 100644 arch/x86/boot/compressed/cmdline.h

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 8bcbcee54aa1..9097108c37e1 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -3,6 +3,7 @@
#include "misc.h"
#include "error.h"
#include "../string.h"
+#include "cmdline.h"

#include <linux/numa.h>
#include <linux/efi.h>
diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..20f2e6d8b891 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include "misc.h"
+#include "cmdline.h"

static unsigned long fs;
static inline void set_fs(unsigned long seg)
diff --git a/arch/x86/boot/compressed/cmdline.h b/arch/x86/boot/compressed/cmdline.h
new file mode 100644
index 000000000000..72800770bd60
--- /dev/null
+++ b/arch/x86/boot/compressed/cmdline.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef BOOT_COMPRESSED_CMDLINE_H
+#define BOOT_COMPRESSED_CMDLINE_H
+
+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+unsigned long get_cmd_line_ptr(void);
+int cmdline_find_option(const char *option, char *buffer, int bufsize);
+int cmdline_find_option_bool(const char *option);
+
+#endif /* BOOT_COMPRESSED_CMDLINE_H */
diff --git a/arch/x86/boot/compressed/early_serial_console.c b/arch/x86/boot/compressed/early_serial_console.c
index 261e81fb9582..64a1f557e122 100644
--- a/arch/x86/boot/compressed/early_serial_console.c
+++ b/arch/x86/boot/compressed/early_serial_console.c
@@ -1,4 +1,5 @@
#include "misc.h"
+#include "cmdline.h"

int early_serial_base;

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index b679908c120e..06ebe5e3e489 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -21,6 +21,7 @@

#include "error.h"
#include "misc.h"
+#include "cmdline.h"

/* These actually do the work of building the kernel identity maps. */
#include <linux/pgtable.h>
@@ -33,12 +34,6 @@
#define __PAGE_OFFSET __PAGE_OFFSET_BASE
#include "../../mm/ident_map.c"

-#define _SETUP
-#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
-#undef _SETUP
-
-extern unsigned long get_cmd_line_ptr(void);
-
/* Used by PAGE_KERN* macros: */
pteval_t __default_kernel_pte_mask __read_mostly = ~0;

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b92fffbe761f..9eabd8bc7673 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -22,6 +22,7 @@
#include "misc.h"
#include "error.h"
#include "../string.h"
+#include "cmdline.h"

#include <generated/compile.h>
#include <linux/module.h>
@@ -36,12 +37,6 @@
#define STATIC
#include <linux/decompress/mm.h>

-#define _SETUP
-#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
-#undef _SETUP
-
-extern unsigned long get_cmd_line_ptr(void);
-
/* Simplified build-specific string for starting entropy. */
static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 6d31f1b4c4d1..e3e2f312c025 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -69,10 +69,6 @@ static inline void debug_puthex(unsigned long value)

#endif

-/* cmdline.c */
-int cmdline_find_option(const char *option, char *buffer, int bufsize);
-int cmdline_find_option_bool(const char *option);
-
struct mem_vector {
u64 start;
u64 size;
diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 25add5510edc..0976c2d2ab2f 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -4,6 +4,7 @@
#include <asm/efi.h>
#include "pgtable.h"
#include "../string.h"
+#include "cmdline.h"

#define BIOS_START_MIN 0x20000U /* 128K, less than this is insane */
#define BIOS_START_MAX 0x9f000U /* 640K, absolute maximum */
@@ -33,7 +34,6 @@ static char trampoline_save[TRAMPOLINE_32BIT_SIZE];
unsigned long *trampoline_32bit __section(.data);

extern struct boot_params *boot_params;
-int cmdline_find_option_bool(const char *option);

static unsigned long find_trampoline_placement(void)
{
--
2.26.2

2020-10-16 22:33:21

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 07:32:32PM +0200, Borislav Petkov wrote:
> On Fri, Oct 16, 2020 at 01:20:58PM -0400, Arvind Sankar wrote:
> > This patch depends on 1 to initialize boot_params before
> > initialize_identity_maps() is called. You want me to rework it to avoid
> > that?
>
> Yes please. If it doesn't become too ugly, that is. If it does, then
> we'll have to think of something else...
>
> Thx.
>

I just sent out v3, with the patches reordered. Maybe just pick up the
fix for now, and the other three patches I can resend sometime after the
merge window, along with other cleanups I've been working on.

Thanks.

2020-10-16 22:33:40

[permalink] [raw]

Subject: Re: [PATCH v2 4/5] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 05:18:41PM -0400, Arvind Sankar wrote:
> I just sent out v3, with the patches reordered. Maybe just pick up the
> fix for now, and the other three patches I can resend sometime after the
> merge window, along with other cleanups I've been working on.

Sure, however you prefer.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-19 14:54:28

[permalink] [raw]

Subject: Re: [PATCH v3 1/4] x86/boot/64: Explicitly map boot_params and command line

On Fri, Oct 16, 2020 at 04:04:01PM -0400, Arvind Sankar wrote:
> Commits
>
> ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
> 8570978ea030 ("x86/boot/compressed/64: Don't pre-map memory in KASLR code")
>
> set up a new page table in the decompressor stub, but without explicit
> mappings for boot_params and the kernel command line, relying on the #PF
> handler instead.
>
> This is fragile, as boot_params and the command line mappings are
> required for the main kernel. If EARLY_PRINTK and RANDOMIZE_BASE are
> disabled, a QEMU/OVMF boot never accesses the command line in the
> decompressor stub, and so it never gets mapped. The main kernel accesses
> it from the identity mapping if AMD_MEM_ENCRYPT is enabled, and will
> crash.
>
> Fix this by adding back the explicit mapping of boot_params and the
> command line.
>
> Note: the changes also removed the explicit mapping of the main kernel,
> with the result that .bss and .brk may not be in the identity mapping,
> but those don't get accessed by the main kernel before it switches to
> its own page tables.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> Reviewed-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/boot/compressed/head_64.S | 3 +++
> arch/x86/boot/compressed/ident_map_64.c | 24 +++++++++++++++++++++---
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 1c80f1738fd9..3976b4e92e1b 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -544,6 +544,9 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> pushq %rsi
> call set_sev_encryption_mask
> call load_stage2_idt
> + /* Pass boot_params to initialize_identity_maps */
> + popq %rdi
> + pushq %rdi

Any reason why you're not doing

movq (%rsp), %rdi

here instead?

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-10-19 17:15:02

[permalink] [raw]

Subject: Re: [PATCH v3 1/4] x86/boot/64: Explicitly map boot_params and command line

On Mon, Oct 19, 2020 at 04:51:15PM +0200, Borislav Petkov wrote:
> On Fri, Oct 16, 2020 at 04:04:01PM -0400, Arvind Sankar wrote:
> > Commits
> >
> > ca0e22d4f011 ("x86/boot/compressed/64: Always switch to own page table")
> > 8570978ea030 ("x86/boot/compressed/64: Don't pre-map memory in KASLR code")
> >
> > set up a new page table in the decompressor stub, but without explicit
> > mappings for boot_params and the kernel command line, relying on the #PF
> > handler instead.
> >
> > This is fragile, as boot_params and the command line mappings are
> > required for the main kernel. If EARLY_PRINTK and RANDOMIZE_BASE are
> > disabled, a QEMU/OVMF boot never accesses the command line in the
> > decompressor stub, and so it never gets mapped. The main kernel accesses
> > it from the identity mapping if AMD_MEM_ENCRYPT is enabled, and will
> > crash.
> >
> > Fix this by adding back the explicit mapping of boot_params and the
> > command line.
> >
> > Note: the changes also removed the explicit mapping of the main kernel,
> > with the result that .bss and .brk may not be in the identity mapping,
> > but those don't get accessed by the main kernel before it switches to
> > its own page tables.
> >
> > Signed-off-by: Arvind Sankar <[email protected]>
> > Reviewed-by: Joerg Roedel <[email protected]>
> > ---
> > arch/x86/boot/compressed/head_64.S | 3 +++
> > arch/x86/boot/compressed/ident_map_64.c | 24 +++++++++++++++++++++---
> > 2 files changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> > index 1c80f1738fd9..3976b4e92e1b 100644
> > --- a/arch/x86/boot/compressed/head_64.S
> > +++ b/arch/x86/boot/compressed/head_64.S
> > @@ -544,6 +544,9 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
> > pushq %rsi
> > call set_sev_encryption_mask
> > call load_stage2_idt
> > + /* Pass boot_params to initialize_identity_maps */
> > + popq %rdi
> > + pushq %rdi
>
> Any reason why you're not doing
>
> movq (%rsp), %rdi
>
> here instead?
>

No real reason. This will disappear anyway in the cleanup patch.

2020-10-19 17:33:00