[PATCH 0/5] Couple of bugfixes to sev-es series

[PATCH 0/5] Couple of bugfixes to sev-es series

[Arvind Sankar]

With the SEV-ES series, the kernel command line is no longer guaranteed
to be mapped on entry into the main kernel. This fixes that, and a
stackprotector issue that cropped up on head64.c.

The first three patches are preparatory cleanups. Patch 4 fixes the
mapping issue and patch 5 disables stack protector for head code.

Arvind Sankar (5):
  x86/boot: Initialize boot_params in startup code
  x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h
  x86/boot/64: Change add_identity_map() to take size for ease of use
  x86/boot/64: Explicitly map boot_params and command line
  x86/head/64: Disable stack protection for head$(BITS).o

 arch/x86/boot/compressed/cmdline.c      |  8 ------
 arch/x86/boot/compressed/head_32.S      | 11 ++++----
 arch/x86/boot/compressed/head_64.S      | 34 ++++++++-----------------
 arch/x86/boot/compressed/ident_map_64.c | 18 ++++++-------
 arch/x86/boot/compressed/kaslr.c        |  6 -----
 arch/x86/boot/compressed/misc.c         | 10 +-------
 arch/x86/boot/compressed/misc.h         | 13 ++++++++++
 arch/x86/boot/compressed/pgtable_64.c   |  5 +---
 arch/x86/kernel/Makefile                |  2 ++
 9 files changed, 43 insertions(+), 64 deletions(-)

-- 
2.26.2

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

[Arvind Sankar]

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.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/head_32.S    | 11 +++++----
 arch/x86/boot/compressed/head_64.S    | 34 +++++++++------------------
 arch/x86/boot/compressed/misc.c       | 10 +-------
 arch/x86/boot/compressed/pgtable_64.c |  5 +---
 4 files changed, 19 insertions(+), 41 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 659fad53ca82..c2b014ca92f7 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,8 @@ SYM_DATA_START_LOCAL(gdt)
 	.quad	0x00cf92000000ffff	/* __KERNEL_DS */
 SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
 
+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..78f873f76579 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,8 @@ SYM_DATA_START(boot_idt)
 	.endr
 SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)
 
+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

[PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Arvind Sankar]

Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h for easier
use from multiple files.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/cmdline.c |  8 --------
 arch/x86/boot/compressed/kaslr.c   |  6 ------
 arch/x86/boot/compressed/misc.h    | 13 +++++++++++++
 3 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
index f1add5d85da9..d0e1d386749d 100644
--- a/arch/x86/boot/compressed/cmdline.c
+++ b/arch/x86/boot/compressed/cmdline.c
@@ -12,14 +12,6 @@ static inline char rdfs8(addr_t addr)
 	return *((char *)(fs + addr));
 }
 #include "../cmdline.c"
-unsigned long get_cmd_line_ptr(void)
-{
-	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
-
-	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
-
-	return cmd_line_ptr;
-}
 int cmdline_find_option(const char *option, char *buffer, int bufsize)
 {
 	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b59547ce5b19..f3286a3bef36 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -36,12 +36,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..95aacc361f78 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -25,6 +25,10 @@
 #include <asm/bootparam.h>
 #include <asm/desc_defs.h>
 
+#define _SETUP
+#include <asm/setup.h>	/* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
 #define BOOT_CTYPE_H
 #include <linux/acpi.h>
 
@@ -70,6 +74,15 @@ static inline void debug_puthex(unsigned long value)
 #endif
 
 /* cmdline.c */
+static inline
+unsigned long get_cmd_line_ptr(void)
+{
+	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
+
+	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
+
+	return cmd_line_ptr;
+}
 int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 
-- 
2.26.2

[PATCH 3/5] x86/boot/64: Change add_identity_map() to take size for ease of use

[Arvind Sankar]

Change back the arguments of add_identity_map() to (start, size) instead
of (start, end). This reverts

  21cf2372618e ("x86/boot/compressed/64: Change add_identity_map() to take start and end")

since we will soon have more callers that know the size rather than the
end address.

This also 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 <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/ident_map_64.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 063a60edcf99..070cda70aef3 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -90,8 +90,9 @@ static struct x86_mapping_info mapping_info;
 /*
  * Adds the specified range to the identity mappings.
  */
-static void add_identity_map(unsigned long start, unsigned long end)
+static void add_identity_map(unsigned long start, unsigned long size)
 {
+	unsigned long end = start + size;
 	int ret;
 
 	/* Align boundary to 2M. */
@@ -152,7 +153,7 @@ void initialize_identity_maps(void)
 	 * New page-table is set up - map the kernel image and load it
 	 * into cr3.
 	 */
-	add_identity_map((unsigned long)_head, (unsigned long)_end);
+	add_identity_map((unsigned long)_head, (unsigned long)_end - (unsigned long)_head);
 	write_cr3(top_level_pgt);
 }
 
@@ -322,14 +323,10 @@ static void do_pf_error(const char *msg, unsigned long error_code,
 void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
 	unsigned long address = native_read_cr2();
-	unsigned long end;
 	bool ghcb_fault;
 
 	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,5 @@ 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.
 	 */
-	add_identity_map(address, end);
+	add_identity_map(address & PMD_MASK, PMD_SIZE);
 }
-- 
2.26.2

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

[Arvind Sankar]

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 <nivedita@alum.mit.edu>
---
 arch/x86/boot/compressed/ident_map_64.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 070cda70aef3..8edeff0d9324 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -150,10 +150,13 @@ 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, and load the new page-table into cr3.
 	 */
 	add_identity_map((unsigned long)_head, (unsigned long)_end - (unsigned long)_head);
+	add_identity_map((unsigned long)boot_params, sizeof(*boot_params));
+	add_identity_map(get_cmd_line_ptr(), COMMAND_LINE_SIZE);
+
 	write_cr3(top_level_pgt);
 }
 
-- 
2.26.2

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

[Arvind Sankar]

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 <nivedita@alum.mit.edu>
---
 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

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

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:51PM -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.

Hmm, that is weird. Can you please share your config with which this
happens? I have a commit in my local branch which disables page-faulting
in the pre-decompression code before jumping to the uncompressed kernel
image, and it did not break anything here.

Also, what compiler did you use to trigger this?

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

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:47PM -0400, Arvind Sankar wrote:
> 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.
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Nice cleanup!

>  /*
>   * Jump to the extracted kernel.
>   */
> -	xorl	%ebx, %ebx
> +	movl	boot_params@GOTOFF(%ebx), %esi
>  	jmp	*%eax
>  SYM_FUNC_END(.Lrelocated)
>  
> @@ -209,6 +208,8 @@ SYM_DATA_START_LOCAL(gdt)
>  	.quad	0x00cf92000000ffff	/* __KERNEL_DS */
>  SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
>  
> +SYM_DATA(boot_params, .long 0)
> +

You should add a comment here that boot_params needs to be in the .data
section because in .bss it would get zeroed out again later. Same
applies to the 64bit version of this.

With that changed:

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:48PM -0400, Arvind Sankar wrote:
> Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h for easier
> use from multiple files.
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/cmdline.c |  8 --------
>  arch/x86/boot/compressed/kaslr.c   |  6 ------
>  arch/x86/boot/compressed/misc.h    | 13 +++++++++++++
>  3 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
> index f1add5d85da9..d0e1d386749d 100644
> --- a/arch/x86/boot/compressed/cmdline.c
> +++ b/arch/x86/boot/compressed/cmdline.c
> @@ -12,14 +12,6 @@ static inline char rdfs8(addr_t addr)
>  	return *((char *)(fs + addr));
>  }
>  #include "../cmdline.c"
> -unsigned long get_cmd_line_ptr(void)
> -{
> -	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
> -
> -	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
> -
> -	return cmd_line_ptr;
> -}
>  int cmdline_find_option(const char *option, char *buffer, int bufsize)
>  {
>  	return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index b59547ce5b19..f3286a3bef36 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -36,12 +36,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..95aacc361f78 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -25,6 +25,10 @@
>  #include <asm/bootparam.h>
>  #include <asm/desc_defs.h>
>  
> +#define _SETUP
> +#include <asm/setup.h>	/* For COMMAND_LINE_SIZE */
> +#undef _SETUP
> +
>  #define BOOT_CTYPE_H
>  #include <linux/acpi.h>
>  
> @@ -70,6 +74,15 @@ static inline void debug_puthex(unsigned long value)
>  #endif
>  
>  /* cmdline.c */
> +static inline
> +unsigned long get_cmd_line_ptr(void)
> +{
> +	unsigned long cmd_line_ptr = boot_params->hdr.cmd_line_ptr;
> +
> +	cmd_line_ptr |= (u64)boot_params->ext_cmd_line_ptr << 32;
> +
> +	return cmd_line_ptr;
> +}
>  int cmdline_find_option(const char *option, char *buffer, int bufsize);
>  int cmdline_find_option_bool(const char *option);

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 3/5] x86/boot/64: Change add_identity_map() to take size for ease of use

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:49PM -0400, Arvind Sankar wrote:
> Change back the arguments of add_identity_map() to (start, size) instead
> of (start, end). This reverts
> 
>   21cf2372618e ("x86/boot/compressed/64: Change add_identity_map() to take start and end")
> 
> since we will soon have more callers that know the size rather than the
> end address.
> 
> This also 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 <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/ident_map_64.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> index 063a60edcf99..070cda70aef3 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -90,8 +90,9 @@ static struct x86_mapping_info mapping_info;
>  /*
>   * Adds the specified range to the identity mappings.
>   */
> -static void add_identity_map(unsigned long start, unsigned long end)
> +static void add_identity_map(unsigned long start, unsigned long size)
>  {
> +	unsigned long end = start + size;

This has been discussed during the SEV-ES patch-review already and we
settled on making add_identity_map() take start and end as parameter, as
that is what kernel_ident_mapping_init() also takes as parameters.

So please keep it that way :)

Regards,

	Joerg

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

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:50PM -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 <nivedita@alum.mit.edu>
> ---
>  arch/x86/boot/compressed/ident_map_64.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> index 070cda70aef3..8edeff0d9324 100644
> --- a/arch/x86/boot/compressed/ident_map_64.c
> +++ b/arch/x86/boot/compressed/ident_map_64.c
> @@ -150,10 +150,13 @@ 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, and load the new page-table into cr3.
>  	 */

Please extend this comment to state that boot_params and command-line
need to be mapped here because they might not be touched (and thus
mapped) before jumping to the uncompressed kernel image. Otherwise no
one will remember why those need to be pre-mapped in a couple of years.

With that change and the add_identity_map() call adjusted:

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Borislav Petkov]

On Wed, Oct 07, 2020 at 03:53:48PM -0400, Arvind Sankar wrote:
> Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h for easier
> use from multiple files.

Well, I don't like that. cmdline.c *is* for cmdline-related things.
misc.h is a dumping ground for everything but the kitchen sink.

Why can't you leave it there and make it visible to other compilation
units?

-- 
Regards/Gruss,
    Boris.

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

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

[Joerg Roedel]

On Wed, Oct 07, 2020 at 03:53:50PM -0400, Arvind Sankar wrote:
> 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.

Looked again, and I think that is wrong for boot_params, which are
touched unconditionally at the beginning of extract_kernel().

For the cmdline you are right, but one of CONFIG_ACPI,
CONFIG_RANDOMIZE_BASE, CONFIG_X86_5LEVEL or CONFIG_EARLY_PRINTK is
sufficient to have it touched during this boot stage.

Regards,

	Joerg

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

[Arvind Sankar]

On Thu, Oct 08, 2020 at 11:04:20AM +0200, Joerg Roedel wrote:
> On Wed, Oct 07, 2020 at 03:53:47PM -0400, Arvind Sankar wrote:
> > 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.
> > 
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> Nice cleanup!
> 
> >  /*
> >   * Jump to the extracted kernel.
> >   */
> > -	xorl	%ebx, %ebx
> > +	movl	boot_params@GOTOFF(%ebx), %esi
> >  	jmp	*%eax
> >  SYM_FUNC_END(.Lrelocated)
> >  
> > @@ -209,6 +208,8 @@ SYM_DATA_START_LOCAL(gdt)
> >  	.quad	0x00cf92000000ffff	/* __KERNEL_DS */
> >  SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)
> >  
> > +SYM_DATA(boot_params, .long 0)
> > +
> 
> You should add a comment here that boot_params needs to be in the .data
> section because in .bss it would get zeroed out again later. Same
> applies to the 64bit version of this.
> 
> With that changed:
> 
> Reviewed-by: Joerg Roedel <jroedel@suse.de>
> 

Ok.

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Arvind Sankar]

On Thu, Oct 08, 2020 at 11:30:42AM +0200, Borislav Petkov wrote:
> On Wed, Oct 07, 2020 at 03:53:48PM -0400, Arvind Sankar wrote:
> > Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h for easier
> > use from multiple files.
> 
> Well, I don't like that. cmdline.c *is* for cmdline-related things.
> misc.h is a dumping ground for everything but the kitchen sink.
> 
> Why can't you leave it there and make it visible to other compilation
> units?
> 

Are you ok with the include of setup.h?

I made the function inline because it's a tiny function, but I can
simply add a prototype if that's preferred. KASLR does use it as one
more memory region to avoid, rather than just for parsing the command
line.

Thanks.

Re: [PATCH 3/5] x86/boot/64: Change add_identity_map() to take size for ease of use

[Arvind Sankar]

On Thu, Oct 08, 2020 at 11:14:12AM +0200, Joerg Roedel wrote:
> On Wed, Oct 07, 2020 at 03:53:49PM -0400, Arvind Sankar wrote:
> > Change back the arguments of add_identity_map() to (start, size) instead
> > of (start, end). This reverts
> > 
> >   21cf2372618e ("x86/boot/compressed/64: Change add_identity_map() to take start and end")
> > 
> > since we will soon have more callers that know the size rather than the
> > end address.
> > 
> > This also 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 <nivedita@alum.mit.edu>
> > ---
> >  arch/x86/boot/compressed/ident_map_64.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
> > index 063a60edcf99..070cda70aef3 100644
> > --- a/arch/x86/boot/compressed/ident_map_64.c
> > +++ b/arch/x86/boot/compressed/ident_map_64.c
> > @@ -90,8 +90,9 @@ static struct x86_mapping_info mapping_info;
> >  /*
> >   * Adds the specified range to the identity mappings.
> >   */
> > -static void add_identity_map(unsigned long start, unsigned long end)
> > +static void add_identity_map(unsigned long start, unsigned long size)
> >  {
> > +	unsigned long end = start + size;
> 
> This has been discussed during the SEV-ES patch-review already and we
> settled on making add_identity_map() take start and end as parameter, as
> that is what kernel_ident_mapping_init() also takes as parameters.
> 
> So please keep it that way :)
> 
> Regards,
> 
> 	Joerg

At the time, it was one caller knowing end and one knowing size, but now
there are two more callers that only know the size, so it seemed easier
to have add_identity_map() do the conversion.

Thanks.

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

[Arvind Sankar]

On Thu, Oct 08, 2020 at 11:48:36AM +0200, Joerg Roedel wrote:
> On Wed, Oct 07, 2020 at 03:53:50PM -0400, Arvind Sankar wrote:
> > 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.
> 
> Looked again, and I think that is wrong for boot_params, which are
> touched unconditionally at the beginning of extract_kernel().

Yes, command line is the only thing that actually breaks, but it is more
robust to explicitly make sure boot_params is mapped as well. There's no
specific alignment requirement for boot_params AFAICT, so at least in
theory it's possible that it would be split across a PMD boundary and
only get half-mapped in the decompressor. It's easier not to have to
worry about it.

> 
> For the cmdline you are right, but one of CONFIG_ACPI,
> CONFIG_RANDOMIZE_BASE, CONFIG_X86_5LEVEL or CONFIG_EARLY_PRINTK is
> sufficient to have it touched during this boot stage.
> 

X86_5LEVEL accesses it before the switch to the new page tables, so that
doesn't help in getting it mapped. ACPI only accesses it if KASLR is
enabled (as well as MEMORY_HOTREMOVE).

Thanks.

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

[Arvind Sankar]

On Thu, Oct 08, 2020 at 10:42:19AM +0200, Joerg Roedel wrote:
> On Wed, Oct 07, 2020 at 03:53:51PM -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.
> 
> Hmm, that is weird. Can you please share your config with which this
> happens? I have a commit in my local branch which disables page-faulting
> in the pre-decompression code before jumping to the uncompressed kernel
> image, and it did not break anything here.
> 
> Also, what compiler did you use to trigger this?
> 

The compiler is gcc-10.

QEMU usually puts the command line in the low 1Mb, so it needs to avoid
accessing the command line as well, as the stack canary is at 0x28. So
defconfig with
  AMD_MEM_ENCRYPT	enabled
  RANDOMIZE_BASE	disabled
  EARLY_PRINTK		disabled
crashes on boot if you disable the #PF handler.

I traced it down with the patch below, which produces
  $ qemu-system-x86_64 -m 2048 -kernel bzImage -append "earlyprintk=ttyS0,keep" -drive if=pflash,format=raw,readonly,file=OVMF_64.fd -nographic -enable-kvm
  
  ...
  
  Decompressing Linux... 
  Error Code: 0000000000000002
  CR2: 0x0000000003e00000
  RIP relative to _head: 0x000000000088521d
  
  Error Code: 0000000000000002
  CR2: 0x0000000004000000
  RIP relative to _head: 0x000000000088521d
  
  ...
  
  Error Code: 0000000000000002
  CR2: 0x0000000005a00000
  RIP relative to _head: 0x00000000008854d0
  Parsing ELF... done.
  Booting the kernel.
  
  Error Code: 0000000000000000
  CR2: 0x0000000000000028
  RIP relative to _head: 0xfffffffffe03b67c

That last page fault is in the main kernel at the stack canary's address.

Patch:

diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 063a60edcf99..e0578dcbaecc 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -325,6 +325,13 @@ void do_boot_page_fault(struct pt_regs *regs, unsigned long error_code)
 	unsigned long end;
 	bool ghcb_fault;
 
+	error_putstr("\nError Code: ");
+	error_puthex(error_code);
+	error_putstr("\nCR2: 0x");
+	error_puthex(address);
+	error_putstr("\nRIP relative to _head: 0x");
+	error_puthex(regs->ip - (unsigned long)_head);
+	error_putstr("\n");
 	ghcb_fault = sev_es_check_ghcb_fault(address);
 
 	address   &= PMD_MASK;
diff --git a/arch/x86/boot/early_serial_console.c b/arch/x86/boot/early_serial_console.c
index 023bf1c3de8b..d6f051e4b64b 100644
--- a/arch/x86/boot/early_serial_console.c
+++ b/arch/x86/boot/early_serial_console.c
@@ -50,6 +50,7 @@ static void parse_earlyprintk(void)
 	int pos = 0;
 	int port = 0;
 
+#if 0
 	if (cmdline_find_option("earlyprintk", arg, sizeof(arg)) > 0) {
 		char *e;
 
@@ -93,6 +94,8 @@ static void parse_earlyprintk(void)
 		if (baud == 0 || arg + pos == e)
 			baud = DEFAULT_BAUD;
 	}
+#endif
+	port = 0x3f8;
 
 	if (port)
 		early_serial_init(port, baud);

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Borislav Petkov]

On Thu, Oct 08, 2020 at 09:47:23AM -0400, Arvind Sankar wrote:
> Are you ok with the include of setup.h?

Or you could simply add cmdline.h and include that. It is high time we
started cleaning up that include hell in compressed/ and all facilities
there be nicely separated. Recently I started untangling it but it is a
serious mess. And kernel proper includes leak in there, yuck.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Arvind Sankar]

On Thu, Oct 08, 2020 at 05:10:47PM +0200, Borislav Petkov wrote:
> On Thu, Oct 08, 2020 at 09:47:23AM -0400, Arvind Sankar wrote:
> > Are you ok with the include of setup.h?
> 
> Or you could simply add cmdline.h and include that. It is high time we
> started cleaning up that include hell in compressed/ and all facilities
> there be nicely separated. Recently I started untangling it but it is a
> serious mess. And kernel proper includes leak in there, yuck.
> 

Ok, I can do that.

I'm working on a couple of separate series to clean up cmdline and the
compressed boot code a bit. I was actually planning to get rid of
boot/compressed/cmdline.c entirely, replacing it with
arch/x86/lib/cmdline.c instead: that one's better and is reusable as-is
for the decompressor stub, instead of the current hack to use the
real-mode boot stub's cmdline.c. The real mess in there is all the
includes of .c files from various places.

Re: [PATCH 2/5] x86/boot: Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h

[Borislav Petkov]

On Thu, Oct 08, 2020 at 11:30:02AM -0400, Arvind Sankar wrote:
> I'm working on a couple of separate series to clean up cmdline
> and the compressed boot code a bit. I was actually planning to
> get rid of boot/compressed/cmdline.c entirely, replacing it with
> arch/x86/lib/cmdline.c instead:

The problem with mixing code from kernel proper with the decompressor is
that when someone changes former, latter gets all those changes too and
gradual changes like that have lead to this mess. There's a reason the
two are separate and we should separate them even more. I'm even fine
with copying functionality between the two instead of sharing.

> that one's better and is reusable as-is for the decompressor stub,
> instead of the current hack to use the real-mode boot stub's
> cmdline.c. The real mess in there is all the includes of .c files from
> various places.

Yes, and that needs untangling and making all separate. This will keep
the decompressor simple and without all that undeffery/ifdeffery because
of includes leaking symbols from kernel proper and whatnot.

Thx.

-- 
Regards/Gruss,
    Boris.

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