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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
Move get_cmd_line_ptr() and COMMAND_LINE_SIZE into misc.h for easier
use from multiple files.
Signed-off-by: Arvind Sankar <[email protected]>
---
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
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?
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
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 <[email protected]>
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
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.
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
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.
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 <[email protected]>
>
> 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 <[email protected]>
>
Ok.
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);
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
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 <[email protected]>
> > ---
> > 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.