2022-11-22 16:46:22

by Ard Biesheuvel

Subject: [PATCH v3 00/17] x86: head_64.S spring cleaning

After doing some cleanup work on the EFI code in head_64.S, the mixed
mode code in particular, I noticed that the memory encryption pieces
could use some attention as well, so I cleaned that up too.

Changes since v2:
- add some clarifying comments to the EFI mixed mode changes
- include patch to make the EFI handover protocol optional that was sent
out separately before
- rebase onto tip/master

Changes since v1:
- at Boris's request, split the patches into smaller ones that are
easier to review

Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Michael Roth <[email protected]>

Ard Biesheuvel (17):
x86/compressed: efi-mixed: rename efi_thunk_64.S to efi-mixed.S
x86/compressed: efi-mixed: move 32-bit entrypoint code into .text
section
x86/compressed: efi-mixed: move bootargs parsing out of 32-bit startup
code
x86/compressed: efi-mixed: move efi32_pe_entry into .text section
x86/compressed: efi-mixed: move efi32_entry out of head_64.S
x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S
x86/compressed: efi: merge multiple definitions of image_offset into
one
x86/compressed: efi-mixed: simplify IDT/GDT preserve/restore
x86/compressed: avoid touching ECX in startup32_set_idt_entry()
x86/compressed: pull global variable ref up into startup32_load_idt()
x86/compressed: move startup32_load_idt() into .text section
x86/compressed: move startup32_load_idt() out of head_64.S
x86/compressed: move startup32_check_sev_cbit() into .text
x86/compressed: move startup32_check_sev_cbit() out of head_64.S
x86/compressed: adhere to calling convention in
get_sev_encryption_bit()
x86/compressed: only build mem_encrypt.S if AMD_MEM_ENCRYPT=y
efi: x86: Make the deprecated EFI handover protocol optional

arch/x86/Kconfig | 17 +
arch/x86/boot/compressed/Makefile | 8 +-
arch/x86/boot/compressed/efi_mixed.S | 351 ++++++++++++++++++++
arch/x86/boot/compressed/efi_thunk_64.S | 195 -----------
arch/x86/boot/compressed/head_32.S | 4 -
arch/x86/boot/compressed/head_64.S | 303 +----------------
arch/x86/boot/compressed/mem_encrypt.S | 152 ++++++++-
arch/x86/boot/header.S | 2 +-
arch/x86/boot/tools/build.c | 2 +
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
10 files changed, 533 insertions(+), 503 deletions(-)
create mode 100644 arch/x86/boot/compressed/efi_mixed.S
delete mode 100644 arch/x86/boot/compressed/efi_thunk_64.S

--
2.35.1

2022-11-22 16:59:38

by Ard Biesheuvel

Subject: [PATCH v3 12/17] x86/compressed: move startup32_load_idt() out of head_64.S

Now that startup32_load_idt() has been refactored into an ordinary
callable function, move it into mem-encrypt.S where it belongs.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 72 --------------------
arch/x86/boot/compressed/mem_encrypt.S | 72 +++++++++++++++++++-
2 files changed, 71 insertions(+), 73 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 2d42852d5b828209..97b2167f128cbefe 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -715,78 +715,6 @@ SYM_DATA_START(boot_idt)
.endr
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

-#ifdef CONFIG_AMD_MEM_ENCRYPT
-SYM_DATA_START(boot32_idt_desc)
- .word boot32_idt_end - boot32_idt - 1
- .long 0
-SYM_DATA_END(boot32_idt_desc)
- .balign 8
-SYM_DATA_START(boot32_idt)
- .rept 32
- .quad 0
- .endr
-SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
-
- .text
- .code32
-/*
- * Write an IDT entry into boot32_idt
- *
- * Parameters:
- *
- * %eax: Handler address
- * %edx: Vector number
- * %ecx: IDT address
- */
-SYM_FUNC_START_LOCAL(startup32_set_idt_entry)
- /* IDT entry address to %ecx */
- leal (%ecx, %edx, 8), %ecx
-
- /* Build IDT entry, lower 4 bytes */
- movl %eax, %edx
- andl $0x0000ffff, %edx # Target code segment offset [15:0]
- orl $(__KERNEL32_CS << 16), %edx # Target code segment selector
-
- /* Store lower 4 bytes to IDT */
- movl %edx, (%ecx)
-
- /* Build IDT entry, upper 4 bytes */
- movl %eax, %edx
- andl $0xffff0000, %edx # Target code segment offset [31:16]
- orl $0x00008e00, %edx # Present, Type 32-bit Interrupt Gate
-
- /* Store upper 4 bytes to IDT */
- movl %edx, 4(%ecx)
-
- RET
-SYM_FUNC_END(startup32_set_idt_entry)
-
-SYM_FUNC_START(startup32_load_idt)
- push %ebp
- push %ebx
-
- call 1f
-1: pop %ebp
-
- leal (boot32_idt - 1b)(%ebp), %ebx
-
- /* #VC handler */
- leal (startup32_vc_handler - 1b)(%ebp), %eax
- movl $X86_TRAP_VC, %edx
- movl %ebx, %ecx
- call startup32_set_idt_entry
-
- /* Load IDT */
- leal (boot32_idt_desc - 1b)(%ebp), %ecx
- movl %ebx, 2(%ecx)
- lidt (%ecx)
-
- pop %ebx
- pop %ebp
- RET
-SYM_FUNC_END(startup32_load_idt)
-#endif
-
/*
* Check for the correct C-bit position when the startup_32 boot-path is used.
*
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index a73e4d783cae20b6..6747e5e4c696637c 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -12,6 +12,8 @@
#include <asm/processor-flags.h>
#include <asm/msr.h>
#include <asm/asm-offsets.h>
+#include <asm/segment.h>
+#include <asm/trapnr.h>

.text
.code32
@@ -98,7 +100,7 @@ SYM_CODE_START_LOCAL(sev_es_req_cpuid)
jmp 1b
SYM_CODE_END(sev_es_req_cpuid)

-SYM_CODE_START(startup32_vc_handler)
+SYM_CODE_START_LOCAL(startup32_vc_handler)
pushl %eax
pushl %ebx
pushl %ecx
@@ -184,6 +186,63 @@ SYM_CODE_START(startup32_vc_handler)
jmp .Lfail
SYM_CODE_END(startup32_vc_handler)

+/*
+ * Write an IDT entry into boot32_idt
+ *
+ * Parameters:
+ *
+ * %eax: Handler address
+ * %edx: Vector number
+ * %ecx: IDT address
+ */
+SYM_FUNC_START_LOCAL(startup32_set_idt_entry)
+ /* IDT entry address to %ecx */
+ leal (%ecx, %edx, 8), %ecx
+
+ /* Build IDT entry, lower 4 bytes */
+ movl %eax, %edx
+ andl $0x0000ffff, %edx # Target code segment offset [15:0]
+ orl $(__KERNEL32_CS << 16), %edx # Target code segment selector
+
+ /* Store lower 4 bytes to IDT */
+ movl %edx, (%ecx)
+
+ /* Build IDT entry, upper 4 bytes */
+ movl %eax, %edx
+ andl $0xffff0000, %edx # Target code segment offset [31:16]
+ orl $0x00008e00, %edx # Present, Type 32-bit Interrupt Gate
+
+ /* Store upper 4 bytes to IDT */
+ movl %edx, 4(%ecx)
+
+ RET
+SYM_FUNC_END(startup32_set_idt_entry)
+
+SYM_FUNC_START(startup32_load_idt)
+ push %ebp
+ push %ebx
+
+ call 1f
+1: pop %ebp
+
+ leal (boot32_idt - 1b)(%ebp), %ebx
+
+ /* #VC handler */
+ leal (startup32_vc_handler - 1b)(%ebp), %eax
+ movl $X86_TRAP_VC, %edx
+ movl %ebx, %ecx
+ call startup32_set_idt_entry
+
+ /* Load IDT */
+ leal (boot32_idt_desc - 1b)(%ebp), %ecx
+ movl %ebx, 2(%ecx)
+ lidt (%ecx)
+
+ pop %ebx
+ pop %ebp
+ RET
+SYM_FUNC_END(startup32_load_idt)
+
.code64

#include "../../kernel/sev_verify_cbit.S"
@@ -195,4 +254,15 @@ SYM_CODE_END(startup32_vc_handler)
SYM_DATA(sme_me_mask, .quad 0)
SYM_DATA(sev_status, .quad 0)
SYM_DATA(sev_check_data, .quad 0)
+
+SYM_DATA_START_LOCAL(boot32_idt)
+ .rept 32
+ .quad 0
+ .endr
+SYM_DATA_END(boot32_idt)
+
+SYM_DATA_START_LOCAL(boot32_idt_desc)
+ .word . - boot32_idt - 1
+ .long 0
+SYM_DATA_END(boot32_idt_desc)
#endif
--
2.35.1

2022-11-22 17:01:17

by Ard Biesheuvel

Subject: [PATCH v3 14/17] x86/compressed: move startup32_check_sev_cbit() out of head_64.S

Now that the startup32_check_sev_cbit() routine can execute from
anywhere and behaves like an ordinary function, we no longer need to
keep it in head_64.S.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 71 --------------------
arch/x86/boot/compressed/mem_encrypt.S | 68 +++++++++++++++++++
2 files changed, 68 insertions(+), 71 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 272b2e97456f0dcf..0cfc8ce273a2731c 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -718,77 +718,6 @@ SYM_DATA_START(boot_idt)
.endr
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

-/*
- * Check for the correct C-bit position when the startup_32 boot-path is used.
- *
- * The check makes use of the fact that all memory is encrypted when paging is
- * disabled. The function creates 64 bits of random data using the RDRAND
- * instruction. RDRAND is mandatory for SEV guests, so always available. If the
- * hypervisor violates that the kernel will crash right here.
- *
- * The 64 bits of random data are stored to a memory location and at the same
- * time kept in the %eax and %ebx registers. Since encryption is always active
- * when paging is off the random data will be stored encrypted in main memory.
- *
- * Then paging is enabled. When the C-bit position is correct all memory is
- * still mapped encrypted and comparing the register values with memory will
- * succeed. An incorrect C-bit position will map all memory unencrypted, so that
- * the compare will use the encrypted random data and fail.
- */
-#ifdef CONFIG_AMD_MEM_ENCRYPT
- .text
-SYM_FUNC_START(startup32_check_sev_cbit)
- pushl %ebx
- pushl %ebp
-
- call 0f
-0: popl %ebp
-
- /* Check for non-zero sev_status */
- movl (sev_status - 0b)(%ebp), %eax
- testl %eax, %eax
- jz 4f
-
- /*
- * Get two 32-bit random values - Don't bail out if RDRAND fails
- * because it is better to prevent forward progress if no random value
- * can be gathered.
- */
-1: rdrand %eax
- jnc 1b
-2: rdrand %ebx
- jnc 2b
-
- /* Store to memory and keep it in the registers */
- leal (sev_check_data - 0b)(%ebp), %ebp
- movl %eax, 0(%ebp)
- movl %ebx, 4(%ebp)
-
- /* Enable paging to see if encryption is active */
- movl %cr0, %edx /* Backup %cr0 in %edx */
- movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
- movl %ecx, %cr0
-
- cmpl %eax, 0(%ebp)
- jne 3f
- cmpl %ebx, 4(%ebp)
- jne 3f
-
- movl %edx, %cr0 /* Restore previous %cr0 */
-
- jmp 4f
-
-3: /* Check failed - hlt the machine */
- hlt
- jmp 3b
-
-4:
- popl %ebp
- popl %ebx
- RET
-SYM_FUNC_END(startup32_check_sev_cbit)
-#endif
-
/*
* Stack and heap for uncompression
*/
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 6747e5e4c696637c..14cf04a1ed091655 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -243,6 +243,74 @@ SYM_FUNC_START(startup32_load_idt)
RET
SYM_FUNC_END(startup32_load_idt)

+/*
+ * Check for the correct C-bit position when the startup_32 boot-path is used.
+ *
+ * The check makes use of the fact that all memory is encrypted when paging is
+ * disabled. The function creates 64 bits of random data using the RDRAND
+ * instruction. RDRAND is mandatory for SEV guests, so always available. If the
+ * hypervisor violates that the kernel will crash right here.
+ *
+ * The 64 bits of random data are stored to a memory location and at the same
+ * time kept in the %eax and %ebx registers. Since encryption is always active
+ * when paging is off the random data will be stored encrypted in main memory.
+ *
+ * Then paging is enabled. When the C-bit position is correct all memory is
+ * still mapped encrypted and comparing the register values with memory will
+ * succeed. An incorrect C-bit position will map all memory unencrypted, so that
+ * the compare will use the encrypted random data and fail.
+ */
+SYM_FUNC_START(startup32_check_sev_cbit)
+ pushl %ebx
+ pushl %ebp
+
+ call 0f
+0: popl %ebp
+
+ /* Check for non-zero sev_status */
+ movl (sev_status - 0b)(%ebp), %eax
+ testl %eax, %eax
+ jz 4f
+
+ /*
+ * Get two 32-bit random values - Don't bail out if RDRAND fails
+ * because it is better to prevent forward progress if no random value
+ * can be gathered.
+ */
+1: rdrand %eax
+ jnc 1b
+2: rdrand %ebx
+ jnc 2b
+
+ /* Store to memory and keep it in the registers */
+ leal (sev_check_data - 0b)(%ebp), %ebp
+ movl %eax, 0(%ebp)
+ movl %ebx, 4(%ebp)
+
+ /* Enable paging to see if encryption is active */
+ movl %cr0, %edx /* Backup %cr0 in %edx */
+ movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
+ movl %ecx, %cr0
+
+ cmpl %eax, 0(%ebp)
+ jne 3f
+ cmpl %ebx, 4(%ebp)
+ jne 3f
+
+ movl %edx, %cr0 /* Restore previous %cr0 */
+
+ jmp 4f
+
+3: /* Check failed - hlt the machine */
+ hlt
+ jmp 3b
+
+4:
+ popl %ebp
+ popl %ebx
+ RET
+SYM_FUNC_END(startup32_check_sev_cbit)
+
.code64

#include "../../kernel/sev_verify_cbit.S"
--
2.35.1

2022-11-22 17:03:48

by Ard Biesheuvel

Subject: [PATCH v3 13/17] x86/compressed: move startup32_check_sev_cbit() into .text

Move startup32_check_sev_cbit() into the .text section and turn it into
an ordinary function using the ordinary 32-bit calling convention,
instead of saving/restoring the registers that are known to be live at
the only call site. This improves maintainability, and makes it possible
to move this function out of head_64.S and into a separate compilation
unit that is specific to memory encryption.

Note that this requires the call site to be moved before the mixed mode
check, as %eax will be live otherwise.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 35 +++++++++++---------
1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 97b2167f128cbefe..272b2e97456f0dcf 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -259,6 +259,11 @@ SYM_FUNC_START(startup_32)
movl $__BOOT_TSS, %eax
ltr %ax

+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ /* Check if the C-bit position is correct when SEV is active */
+ call startup32_check_sev_cbit
+#endif
+
/*
* Setup for the jump to 64bit mode
*
@@ -276,8 +281,6 @@ SYM_FUNC_START(startup_32)
leal rva(startup_64_mixed_mode)(%ebp), %eax
1:
#endif
- /* Check if the C-bit position is correct when SEV is active */
- call startup32_check_sev_cbit

pushl $__KERNEL_CS
pushl %eax
@@ -732,16 +735,17 @@ SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)
* succeed. An incorrect C-bit position will map all memory unencrypted, so that
* the compare will use the encrypted random data and fail.
*/
- __HEAD
-SYM_FUNC_START(startup32_check_sev_cbit)
#ifdef CONFIG_AMD_MEM_ENCRYPT
- pushl %eax
+ .text
+SYM_FUNC_START(startup32_check_sev_cbit)
pushl %ebx
- pushl %ecx
- pushl %edx
+ pushl %ebp
+
+ call 0f
+0: popl %ebp

/* Check for non-zero sev_status */
- movl rva(sev_status)(%ebp), %eax
+ movl (sev_status - 0b)(%ebp), %eax
testl %eax, %eax
jz 4f

@@ -756,17 +760,18 @@ SYM_FUNC_START(startup32_check_sev_cbit)
jnc 2b

/* Store to memory and keep it in the registers */
- movl %eax, rva(sev_check_data)(%ebp)
- movl %ebx, rva(sev_check_data+4)(%ebp)
+ leal (sev_check_data - 0b)(%ebp), %ebp
+ movl %eax, 0(%ebp)
+ movl %ebx, 4(%ebp)

/* Enable paging to see if encryption is active */
movl %cr0, %edx /* Backup %cr0 in %edx */
movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
movl %ecx, %cr0

- cmpl %eax, rva(sev_check_data)(%ebp)
+ cmpl %eax, 0(%ebp)
jne 3f
- cmpl %ebx, rva(sev_check_data+4)(%ebp)
+ cmpl %ebx, 4(%ebp)
jne 3f

movl %edx, %cr0 /* Restore previous %cr0 */
@@ -778,13 +783,11 @@ SYM_FUNC_START(startup32_check_sev_cbit)
jmp 3b

4:
- popl %edx
- popl %ecx
+ popl %ebp
popl %ebx
- popl %eax
-#endif
RET
SYM_FUNC_END(startup32_check_sev_cbit)
+#endif

/*
* Stack and heap for uncompression
--
2.35.1

2022-11-22 17:18:46

by Ard Biesheuvel

Subject: [PATCH v3 06/17] x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S

Move the implementation of efi32_pe_entry() into efi-mixed.S, which is a
more suitable location that only gets built if EFI mixed mode is
actually enabled.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/efi_mixed.S | 82 ++++++++++++++++++
arch/x86/boot/compressed/head_64.S | 87 +-------------------
2 files changed, 83 insertions(+), 86 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_mixed.S b/arch/x86/boot/compressed/efi_mixed.S
index 3487484ac1fd5c6c..8844d8ed4b1c7561 100644
--- a/arch/x86/boot/compressed/efi_mixed.S
+++ b/arch/x86/boot/compressed/efi_mixed.S
@@ -257,6 +257,88 @@ SYM_FUNC_START(efi32_entry)
jmp startup_32
SYM_FUNC_END(efi32_entry)

+#define ST32_boottime 60 // offsetof(efi_system_table_32_t, boottime)
+#define BS32_handle_protocol 88 // offsetof(efi_boot_services_32_t, handle_protocol)
+#define LI32_image_base 32 // offsetof(efi_loaded_image_32_t, image_base)
+
+/*
+ * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
+ * efi_system_table_32_t *sys_table)
+ */
+SYM_FUNC_START(efi32_pe_entry)
+ pushl %ebp
+ movl %esp, %ebp
+ pushl %eax // dummy push to allocate loaded_image
+
+ pushl %ebx // save callee-save registers
+ pushl %edi
+
+ call verify_cpu // check for long mode support
+ testl %eax, %eax
+ movl $0x80000003, %eax // EFI_UNSUPPORTED
+ jnz 2f
+
+ call 1f
+1: pop %ebx
+
+ /* Get the loaded image protocol pointer from the image handle */
+ leal -4(%ebp), %eax
+ pushl %eax // &loaded_image
+ leal (loaded_image_proto - 1b)(%ebx), %eax
+ pushl %eax // pass the GUID address
+ pushl 8(%ebp) // pass the image handle
+
+ /*
+ * Note the alignment of the stack frame.
+ * sys_table
+ * handle <-- 16-byte aligned on entry by ABI
+ * return address
+ * frame pointer
+ * loaded_image <-- local variable
+ * saved %ebx <-- 16-byte aligned here
+ * saved %edi
+ * &loaded_image
+ * &loaded_image_proto
+ * handle <-- 16-byte aligned for call to handle_protocol
+ */
+
+ movl 12(%ebp), %eax // sys_table
+ movl ST32_boottime(%eax), %eax // sys_table->boottime
+ call *BS32_handle_protocol(%eax) // sys_table->boottime->handle_protocol
+ addl $12, %esp // restore argument space
+ testl %eax, %eax
+ jnz 2f
+
+ movl 8(%ebp), %ecx // image_handle
+ movl 12(%ebp), %edx // sys_table
+ movl -4(%ebp), %esi // loaded_image
+ movl LI32_image_base(%esi), %esi // loaded_image->image_base
+ leal (startup_32 - 1b)(%ebx), %ebp // runtime address of startup_32
+ /*
+ * We need to set the image_offset variable here since startup_32() will
+ * use it before we get to the 64-bit efi_pe_entry() in C code.
+ */
+ subl %esi, %ebp // calculate image_offset
+ movl %ebp, (image_offset - 1b)(%ebx) // save image_offset
+ xorl %esi, %esi
+ jmp efi32_entry // pass %ecx, %edx, %esi
+ // no other registers remain live
+
+2: popl %edi // restore callee-save registers
+ popl %ebx
+ leave
+ RET
+SYM_FUNC_END(efi32_pe_entry)
+
+ .section ".rodata"
+ /* EFI loaded image protocol GUID */
+ .balign 4
+SYM_DATA_START_LOCAL(loaded_image_proto)
+ .long 0x5b1b31a1
+ .word 0x9562, 0x11d2
+ .byte 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b
+SYM_DATA_END(loaded_image_proto)
+
.data
.balign 8
SYM_DATA_START_LOCAL(efi32_boot_gdt)
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 8fa92ce5610e5329..987f0a3c284fad43 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -681,6 +681,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
jmp 1b
SYM_FUNC_END(.Lno_longmode)

+ .globl verify_cpu
#include "../../kernel/verify_cpu.S"

.data
@@ -728,92 +729,6 @@ SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
-#ifdef CONFIG_EFI_MIXED
-#define ST32_boottime 60 // offsetof(efi_system_table_32_t, boottime)
-#define BS32_handle_protocol 88 // offsetof(efi_boot_services_32_t, handle_protocol)
-#define LI32_image_base 32 // offsetof(efi_loaded_image_32_t, image_base)
-
- .text
- .code32
-SYM_FUNC_START(efi32_pe_entry)
-/*
- * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
- * efi_system_table_32_t *sys_table)
- */
-
- pushl %ebp
- movl %esp, %ebp
- pushl %eax // dummy push to allocate loaded_image
-
- pushl %ebx // save callee-save registers
- pushl %edi
-
- call verify_cpu // check for long mode support
- testl %eax, %eax
- movl $0x80000003, %eax // EFI_UNSUPPORTED
- jnz 2f
-
- call 1f
-1: pop %ebx
-
- /* Get the loaded image protocol pointer from the image handle */
- leal -4(%ebp), %eax
- pushl %eax // &loaded_image
- leal (loaded_image_proto - 1b)(%ebx), %eax
- pushl %eax // pass the GUID address
- pushl 8(%ebp) // pass the image handle
-
- /*
- * Note the alignment of the stack frame.
- * sys_table
- * handle <-- 16-byte aligned on entry by ABI
- * return address
- * frame pointer
- * loaded_image <-- local variable
- * saved %ebx <-- 16-byte aligned here
- * saved %edi
- * &loaded_image
- * &loaded_image_proto
- * handle <-- 16-byte aligned for call to handle_protocol
- */
-
- movl 12(%ebp), %eax // sys_table
- movl ST32_boottime(%eax), %eax // sys_table->boottime
- call *BS32_handle_protocol(%eax) // sys_table->boottime->handle_protocol
- addl $12, %esp // restore argument space
- testl %eax, %eax
- jnz 2f
-
- movl 8(%ebp), %ecx // image_handle
- movl 12(%ebp), %edx // sys_table
- movl -4(%ebp), %esi // loaded_image
- movl LI32_image_base(%esi), %esi // loaded_image->image_base
- leal (startup_32 - 1b)(%ebx), %ebp // runtime address of startup_32
- /*
- * We need to set the image_offset variable here since startup_32() will
- * use it before we get to the 64-bit efi_pe_entry() in C code.
- */
- subl %esi, %ebp // calculate image_offset
- movl %ebp, (image_offset - 1b)(%ebx) // save image_offset
- xorl %esi, %esi
- jmp efi32_entry // pass %ecx, %edx, %esi
- // no other registers remain live
-
-2: popl %edi // restore callee-save registers
- popl %ebx
- leave
- RET
-SYM_FUNC_END(efi32_pe_entry)
-
- .section ".rodata"
- /* EFI loaded image protocol GUID */
- .balign 4
-SYM_DATA_START_LOCAL(loaded_image_proto)
- .long 0x5b1b31a1
- .word 0x9562, 0x11d2
- .byte 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b
-SYM_DATA_END(loaded_image_proto)
-#endif

#ifdef CONFIG_AMD_MEM_ENCRYPT
__HEAD
--
2.35.1

2022-11-22 17:21:50

by Ard Biesheuvel

Subject: [PATCH v3 07/17] x86/compressed: efi: merge multiple definitions of image_offset into one

There is no need for head_32.S and head_64.S both declaring a copy of
the globale 'image_offset' variable, so drop those and make the extern C
declaration the definition.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/head_32.S | 4 ----
arch/x86/boot/compressed/head_64.S | 4 ----
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 3b354eb9516df416..6589ddd4cfaf2cb6 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -208,10 +208,6 @@ SYM_DATA_START_LOCAL(gdt)
.quad 0x00cf92000000ffff /* __KERNEL_DS */
SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)

-#ifdef CONFIG_EFI_STUB
-SYM_DATA(image_offset, .long 0)
-#endif
-
/*
* Stack and heap for uncompression
*/
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 987f0a3c284fad43..66ad3ab802ca9d0c 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -726,10 +726,6 @@ SYM_DATA_START(boot32_idt)
SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
#endif

-#ifdef CONFIG_EFI_STUB
-SYM_DATA(image_offset, .long 0)
-#endif
-
#ifdef CONFIG_AMD_MEM_ENCRYPT
__HEAD
.code32
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 33a7811e12c65642..cb5f0befee57d2ec 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -23,7 +23,7 @@

const efi_system_table_t *efi_system_table;
const efi_dxe_services_table_t *efi_dxe_table;
-extern u32 image_offset;
+u32 image_offset;
static efi_loaded_image_t *image = NULL;

static efi_status_t
--
2.35.1

2022-11-22 17:22:14

by Ard Biesheuvel

Subject: [PATCH v3 15/17] x86/compressed: adhere to calling convention in get_sev_encryption_bit()

Make get_sev_encryption_bit() follow the ordinary i386 calling
convention, and only call it if CONFIG_AMD_MEM_ENCRYPT is actually
enabled. This clarifies the calling code, and makes it more
maintainable.

Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 5 +++--
arch/x86/boot/compressed/mem_encrypt.S | 10 ----------
2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 0cfc8ce273a2731c..dd18216cff5c37e0 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -188,12 +188,13 @@ SYM_FUNC_START(startup_32)
*/
/*
* If SEV is active then set the encryption mask in the page tables.
- * This will insure that when the kernel is copied and decompressed
+ * This will ensure that when the kernel is copied and decompressed
* it will be done so encrypted.
*/
- call get_sev_encryption_bit
xorl %edx, %edx
#ifdef CONFIG_AMD_MEM_ENCRYPT
+ call get_sev_encryption_bit
+ xorl %edx, %edx
testl %eax, %eax
jz 1f
subl $32, %eax /* Encryption bit is always above bit 31 */
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 14cf04a1ed091655..e69674588a31c81f 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -18,12 +18,7 @@
.text
.code32
SYM_FUNC_START(get_sev_encryption_bit)
- xor %eax, %eax
-
-#ifdef CONFIG_AMD_MEM_ENCRYPT
push %ebx
- push %ecx
- push %edx

movl $0x80000000, %eax /* CPUID to check the highest leaf */
cpuid
@@ -54,12 +49,7 @@ SYM_FUNC_START(get_sev_encryption_bit)
xor %eax, %eax

.Lsev_exit:
- pop %edx
- pop %ecx
pop %ebx
-
-#endif /* CONFIG_AMD_MEM_ENCRYPT */
-
RET
SYM_FUNC_END(get_sev_encryption_bit)

--
2.35.1

2022-11-22 21:00:46

by Tom Lendacky

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On 11/22/22 10:10, Ard Biesheuvel wrote:
> After doing some cleanup work on the EFI code in head_64.S, the mixed
> mode code in particular, I noticed that the memory encryption pieces
> could use some attention as well, so I cleaned that up too.
>
> Changes since v2:
> - add some clarifying comments to the EFI mixed mode changes
> - include patch to make the EFI handover protocol optional that was sent
> out separately before
> - rebase onto tip/master
>
> Changes since v1:
> - at Boris's request, split the patches into smaller ones that are
> easier to review
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Michael Roth <[email protected]>

This causes an SEV guest to blow up on boot in the early boot code. It
looks like the stack pointer is not valid and it triple faults on a pushq
instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
startup_64).

Here is the Qemu register dump:
RAX=00000000029cc260 RBX=ffffffffdd98c000 RCX=0000000000000010 RDX=0000000000000002
RSI=000000003dec1000 RDI=0000000000000000 RBP=ffffffffdb000000 RSP=ffffffffde36e000
R8 =000000003dec1410 R9 =000000003dec13fc R10=000000000000006c R11=0000000000000000
R12=0000000000000000 R13=0000000000000001 R14=0000000000000004 R15=000000003eacdf44
RIP=0000000002000263 RFL=00200002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 0000000000000000 00000000 00000000
CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
SS =0000 0000000000000000 00000000 00000000
DS =0000 0000000000000000 00000000 00000000
FS =0000 0000000000000000 00000000 00000000
GS =0000 0000000000000000 00000000 00000000
LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
GDT= 00000000029cc270 0000002f
IDT= 000000003f55e018 00000fff
CR0=80010033 CR2=ffffffffde36dff8 CR3=000000003fc01000 CR4=00000668
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=00000000ffff0ff0 DR7=0000000000000400
EFER=0000000000000d00

Thanks,
Tom

>
> Ard Biesheuvel (17):
> x86/compressed: efi-mixed: rename efi_thunk_64.S to efi-mixed.S
> x86/compressed: efi-mixed: move 32-bit entrypoint code into .text
> section
> x86/compressed: efi-mixed: move bootargs parsing out of 32-bit startup
> code
> x86/compressed: efi-mixed: move efi32_pe_entry into .text section
> x86/compressed: efi-mixed: move efi32_entry out of head_64.S
> x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S
> x86/compressed: efi: merge multiple definitions of image_offset into
> one
> x86/compressed: efi-mixed: simplify IDT/GDT preserve/restore
> x86/compressed: avoid touching ECX in startup32_set_idt_entry()
> x86/compressed: pull global variable ref up into startup32_load_idt()
> x86/compressed: move startup32_load_idt() into .text section
> x86/compressed: move startup32_load_idt() out of head_64.S
> x86/compressed: move startup32_check_sev_cbit() into .text
> x86/compressed: move startup32_check_sev_cbit() out of head_64.S
> x86/compressed: adhere to calling convention in
> get_sev_encryption_bit()
> x86/compressed: only build mem_encrypt.S if AMD_MEM_ENCRYPT=y
> efi: x86: Make the deprecated EFI handover protocol optional
>
> arch/x86/Kconfig | 17 +
> arch/x86/boot/compressed/Makefile | 8 +-
> arch/x86/boot/compressed/efi_mixed.S | 351 ++++++++++++++++++++
> arch/x86/boot/compressed/efi_thunk_64.S | 195 -----------
> arch/x86/boot/compressed/head_32.S | 4 -
> arch/x86/boot/compressed/head_64.S | 303 +----------------
> arch/x86/boot/compressed/mem_encrypt.S | 152 ++++++++-
> arch/x86/boot/header.S | 2 +-
> arch/x86/boot/tools/build.c | 2 +
> drivers/firmware/efi/libstub/x86-stub.c | 2 +-
> 10 files changed, 533 insertions(+), 503 deletions(-)
> create mode 100644 arch/x86/boot/compressed/efi_mixed.S
> delete mode 100644 arch/x86/boot/compressed/efi_thunk_64.S
>

2022-11-22 21:45:35

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <[email protected]> wrote:
>
> On 11/22/22 10:10, Ard Biesheuvel wrote:
> > After doing some cleanup work on the EFI code in head_64.S, the mixed
> > mode code in particular, I noticed that the memory encryption pieces
> > could use some attention as well, so I cleaned that up too.
> >
> > Changes since v2:
> > - add some clarifying comments to the EFI mixed mode changes
> > - include patch to make the EFI handover protocol optional that was sent
> > out separately before
> > - rebase onto tip/master
> >
> > Changes since v1:
> > - at Boris's request, split the patches into smaller ones that are
> > easier to review
> >
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Dave Hansen <[email protected]>
> > Cc: Michael Roth <[email protected]>
>
> This causes an SEV guest to blow up on boot in the early boot code. It
> looks like the stack pointer is not valid and it triple faults on a pushq
> instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
> startup_64).
>

Thanks for the report.

So the mystery here (at least to me) is that all the changes are to
the 32-bit code, and startup_64 reloads the stack pointer from the
symbol

Does your config have CONFIG_EFI_MIXED enabled?

Can I reproduce this fully emulated with QEMU? Or do I need a SEV host?

> Here is the Qemu register dump:
> RAX=00000000029cc260 RBX=ffffffffdd98c000 RCX=0000000000000010 RDX=0000000000000002
> RSI=000000003dec1000 RDI=0000000000000000 RBP=ffffffffdb000000 RSP=ffffffffde36e000
> R8 =000000003dec1410 R9 =000000003dec13fc R10=000000000000006c R11=0000000000000000
> R12=0000000000000000 R13=0000000000000001 R14=0000000000000004 R15=000000003eacdf44
> RIP=0000000002000263 RFL=00200002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 0000000000000000 00000000 00000000
> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
> SS =0000 0000000000000000 00000000 00000000
> DS =0000 0000000000000000 00000000 00000000
> FS =0000 0000000000000000 00000000 00000000
> GS =0000 0000000000000000 00000000 00000000
> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
> GDT= 00000000029cc270 0000002f
> IDT= 000000003f55e018 00000fff
> CR0=80010033 CR2=ffffffffde36dff8 CR3=000000003fc01000 CR4=00000668
> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> DR6=00000000ffff0ff0 DR7=0000000000000400
> EFER=0000000000000d00
>
> Thanks,
> Tom
>
> >
> > Ard Biesheuvel (17):
> > x86/compressed: efi-mixed: rename efi_thunk_64.S to efi-mixed.S
> > x86/compressed: efi-mixed: move 32-bit entrypoint code into .text
> > section
> > x86/compressed: efi-mixed: move bootargs parsing out of 32-bit startup
> > code
> > x86/compressed: efi-mixed: move efi32_pe_entry into .text section
> > x86/compressed: efi-mixed: move efi32_entry out of head_64.S
> > x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S
> > x86/compressed: efi: merge multiple definitions of image_offset into
> > one
> > x86/compressed: efi-mixed: simplify IDT/GDT preserve/restore
> > x86/compressed: avoid touching ECX in startup32_set_idt_entry()
> > x86/compressed: pull global variable ref up into startup32_load_idt()
> > x86/compressed: move startup32_load_idt() into .text section
> > x86/compressed: move startup32_load_idt() out of head_64.S
> > x86/compressed: move startup32_check_sev_cbit() into .text
> > x86/compressed: move startup32_check_sev_cbit() out of head_64.S
> > x86/compressed: adhere to calling convention in
> > get_sev_encryption_bit()
> > x86/compressed: only build mem_encrypt.S if AMD_MEM_ENCRYPT=y
> > efi: x86: Make the deprecated EFI handover protocol optional
> >
> > arch/x86/Kconfig | 17 +
> > arch/x86/boot/compressed/Makefile | 8 +-
> > arch/x86/boot/compressed/efi_mixed.S | 351 ++++++++++++++++++++
> > arch/x86/boot/compressed/efi_thunk_64.S | 195 -----------
> > arch/x86/boot/compressed/head_32.S | 4 -
> > arch/x86/boot/compressed/head_64.S | 303 +----------------
> > arch/x86/boot/compressed/mem_encrypt.S | 152 ++++++++-
> > arch/x86/boot/header.S | 2 +-
> > arch/x86/boot/tools/build.c | 2 +
> > drivers/firmware/efi/libstub/x86-stub.c | 2 +-
> > 10 files changed, 533 insertions(+), 503 deletions(-)
> > create mode 100644 arch/x86/boot/compressed/efi_mixed.S
> > delete mode 100644 arch/x86/boot/compressed/efi_thunk_64.S
> >

2022-11-22 22:03:37

by Tom Lendacky

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On 11/22/22 15:42, Ard Biesheuvel wrote:
> On Tue, 22 Nov 2022 at 22:37, Ard Biesheuvel <[email protected]> wrote:
>>
>> On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <[email protected]> wrote:
>>>
>>> On 11/22/22 10:10, Ard Biesheuvel wrote:
>>>> After doing some cleanup work on the EFI code in head_64.S, the mixed
>>>> mode code in particular, I noticed that the memory encryption pieces
>>>> could use some attention as well, so I cleaned that up too.
>>>>
>>>> Changes since v2:
>>>> - add some clarifying comments to the EFI mixed mode changes
>>>> - include patch to make the EFI handover protocol optional that was sent
>>>> out separately before
>>>> - rebase onto tip/master
>>>>
>>>> Changes since v1:
>>>> - at Boris's request, split the patches into smaller ones that are
>>>> easier to review
>>>>
>>>> Cc: Thomas Gleixner <[email protected]>
>>>> Cc: Ingo Molnar <[email protected]>
>>>> Cc: Borislav Petkov <[email protected]>
>>>> Cc: Dave Hansen <[email protected]>
>>>> Cc: Michael Roth <[email protected]>
>>>
>>> This causes an SEV guest to blow up on boot in the early boot code. It
>>> looks like the stack pointer is not valid and it triple faults on a pushq
>>> instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
>>> startup_64).
>>>
>>
>> Thanks for the report.
>>
>> So the mystery here (at least to me) is that all the changes are to
>> the 32-bit code, and startup_64 reloads the stack pointer from the
>> symbol
>>
>> Does your config have CONFIG_EFI_MIXED enabled?
>>
>> Can I reproduce this fully emulated with QEMU? Or do I need a SEV host?
>>
>
> Also, mind giving this a quick spin?

Just saw this after I sent out my email. Yes, this fixes it.

Thanks,
Tom

>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c
> b/drivers/firmware/efi/libstub/x86-stub.c
> index cb5f0befee57..1af11d34bc6c 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -23,7 +23,7 @@
>
> const efi_system_table_t *efi_system_table;
> const efi_dxe_services_table_t *efi_dxe_table;
> -u32 image_offset;
> +u32 __section(".data") image_offset;
> static efi_loaded_image_t *image = NULL;
>
> static efi_status_t
>
> Thanks,
> Ard.

2022-11-22 22:07:17

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Tue, 22 Nov 2022 at 22:37, Ard Biesheuvel <[email protected]> wrote:
>
> On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <[email protected]> wrote:
> >
> > On 11/22/22 10:10, Ard Biesheuvel wrote:
> > > After doing some cleanup work on the EFI code in head_64.S, the mixed
> > > mode code in particular, I noticed that the memory encryption pieces
> > > could use some attention as well, so I cleaned that up too.
> > >
> > > Changes since v2:
> > > - add some clarifying comments to the EFI mixed mode changes
> > > - include patch to make the EFI handover protocol optional that was sent
> > > out separately before
> > > - rebase onto tip/master
> > >
> > > Changes since v1:
> > > - at Boris's request, split the patches into smaller ones that are
> > > easier to review
> > >
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: Ingo Molnar <[email protected]>
> > > Cc: Borislav Petkov <[email protected]>
> > > Cc: Dave Hansen <[email protected]>
> > > Cc: Michael Roth <[email protected]>
> >
> > This causes an SEV guest to blow up on boot in the early boot code. It
> > looks like the stack pointer is not valid and it triple faults on a pushq
> > instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
> > startup_64).
> >
>
> Thanks for the report.
>
> So the mystery here (at least to me) is that all the changes are to
> the 32-bit code, and startup_64 reloads the stack pointer from the
> symbol
>
> Does your config have CONFIG_EFI_MIXED enabled?
>
> Can I reproduce this fully emulated with QEMU? Or do I need a SEV host?
>

Also, mind giving this a quick spin?

diff --git a/drivers/firmware/efi/libstub/x86-stub.c
b/drivers/firmware/efi/libstub/x86-stub.c
index cb5f0befee57..1af11d34bc6c 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -23,7 +23,7 @@

const efi_system_table_t *efi_system_table;
const efi_dxe_services_table_t *efi_dxe_table;
-u32 image_offset;
+u32 __section(".data") image_offset;
static efi_loaded_image_t *image = NULL;

static efi_status_t

Thanks,
Ard.

2022-11-22 22:08:07

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Tue, 22 Nov 2022 at 22:51, Tom Lendacky <[email protected]> wrote:
>
> On 11/22/22 15:42, Ard Biesheuvel wrote:
> > On Tue, 22 Nov 2022 at 22:37, Ard Biesheuvel <[email protected]> wrote:
> >>
> >> On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <[email protected]> wrote:
> >>>
> >>> On 11/22/22 10:10, Ard Biesheuvel wrote:
> >>>> After doing some cleanup work on the EFI code in head_64.S, the mixed
> >>>> mode code in particular, I noticed that the memory encryption pieces
> >>>> could use some attention as well, so I cleaned that up too.
> >>>>
> >>>> Changes since v2:
> >>>> - add some clarifying comments to the EFI mixed mode changes
> >>>> - include patch to make the EFI handover protocol optional that was sent
> >>>> out separately before
> >>>> - rebase onto tip/master
> >>>>
> >>>> Changes since v1:
> >>>> - at Boris's request, split the patches into smaller ones that are
> >>>> easier to review
> >>>>
> >>>> Cc: Thomas Gleixner <[email protected]>
> >>>> Cc: Ingo Molnar <[email protected]>
> >>>> Cc: Borislav Petkov <[email protected]>
> >>>> Cc: Dave Hansen <[email protected]>
> >>>> Cc: Michael Roth <[email protected]>
> >>>
> >>> This causes an SEV guest to blow up on boot in the early boot code. It
> >>> looks like the stack pointer is not valid and it triple faults on a pushq
> >>> instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
> >>> startup_64).
> >>>
> >>
> >> Thanks for the report.
> >>
> >> So the mystery here (at least to me) is that all the changes are to
> >> the 32-bit code, and startup_64 reloads the stack pointer from the
> >> symbol
> >>
> >> Does your config have CONFIG_EFI_MIXED enabled?
> >>
> >> Can I reproduce this fully emulated with QEMU? Or do I need a SEV host?
> >>
> >
> > Also, mind giving this a quick spin?
>
> Just saw this after I sent out my email. Yes, this fixes it.
>

Excellent, thanks for testing.

2022-11-22 22:46:30

by Tom Lendacky

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On 11/22/22 15:37, Ard Biesheuvel wrote:
> On Tue, 22 Nov 2022 at 21:48, Tom Lendacky <[email protected]> wrote:
>>
>> On 11/22/22 10:10, Ard Biesheuvel wrote:
>>> After doing some cleanup work on the EFI code in head_64.S, the mixed
>>> mode code in particular, I noticed that the memory encryption pieces
>>> could use some attention as well, so I cleaned that up too.
>>>
>>> Changes since v2:
>>> - add some clarifying comments to the EFI mixed mode changes
>>> - include patch to make the EFI handover protocol optional that was sent
>>> out separately before
>>> - rebase onto tip/master
>>>
>>> Changes since v1:
>>> - at Boris's request, split the patches into smaller ones that are
>>> easier to review
>>>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> Cc: Borislav Petkov <[email protected]>
>>> Cc: Dave Hansen <[email protected]>
>>> Cc: Michael Roth <[email protected]>
>>
>> This causes an SEV guest to blow up on boot in the early boot code. It
>> looks like the stack pointer is not valid and it triple faults on a pushq
>> instruction (pushq $__KERNEL_CS in arch/x86/boot/compressed/head_64.S of
>> startup_64).
>>
>
> Thanks for the report.
>
> So the mystery here (at least to me) is that all the changes are to
> the 32-bit code, and startup_64 reloads the stack pointer from the
> symbol

I bisected it to:

99b7f4b23d9f ("x86/boot/compressed, efi: Merge multiple definitions of image_offset into one")

And doing the following fixed it:

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index cb5f0befee57..a0bfd31358ba 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -23,7 +23,7 @@

const efi_system_table_t *efi_system_table;
const efi_dxe_services_table_t *efi_dxe_table;
-u32 image_offset;
+u32 image_offset __section(".data");
static efi_loaded_image_t *image = NULL;

static efi_status_t

I assume it has to do with being in .data vs .bss and not being explicitly
cleared with the encryption bit set. With the change to put image_offset in
the .data section, it is read as zero, where as when it was in the .bss
section it was reading "ciphertext".

Thanks,
Tom

>
> Does your config have CONFIG_EFI_MIXED enabled?
>
> Can I reproduce this fully emulated with QEMU? Or do I need a SEV host?
>
>> Here is the Qemu register dump:
>> RAX=00000000029cc260 RBX=ffffffffdd98c000 RCX=0000000000000010 RDX=0000000000000002
>> RSI=000000003dec1000 RDI=0000000000000000 RBP=ffffffffdb000000 RSP=ffffffffde36e000
>> R8 =000000003dec1410 R9 =000000003dec13fc R10=000000000000006c R11=0000000000000000
>> R12=0000000000000000 R13=0000000000000001 R14=0000000000000004 R15=000000003eacdf44
>> RIP=0000000002000263 RFL=00200002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
>> ES =0000 0000000000000000 00000000 00000000
>> CS =0038 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
>> SS =0000 0000000000000000 00000000 00000000
>> DS =0000 0000000000000000 00000000 00000000
>> FS =0000 0000000000000000 00000000 00000000
>> GS =0000 0000000000000000 00000000 00000000
>> LDT=0000 0000000000000000 0000ffff 00008200 DPL=0 LDT
>> TR =0000 0000000000000000 0000ffff 00008b00 DPL=0 TSS64-busy
>> GDT= 00000000029cc270 0000002f
>> IDT= 000000003f55e018 00000fff
>> CR0=80010033 CR2=ffffffffde36dff8 CR3=000000003fc01000 CR4=00000668
>> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
>> DR6=00000000ffff0ff0 DR7=0000000000000400
>> EFER=0000000000000d00
>>
>> Thanks,
>> Tom
>>
>>>
>>> Ard Biesheuvel (17):
>>> x86/compressed: efi-mixed: rename efi_thunk_64.S to efi-mixed.S
>>> x86/compressed: efi-mixed: move 32-bit entrypoint code into .text
>>> section
>>> x86/compressed: efi-mixed: move bootargs parsing out of 32-bit startup
>>> code
>>> x86/compressed: efi-mixed: move efi32_pe_entry into .text section
>>> x86/compressed: efi-mixed: move efi32_entry out of head_64.S
>>> x86/compressed: efi-mixed: move efi32_pe_entry() out of head_64.S
>>> x86/compressed: efi: merge multiple definitions of image_offset into
>>> one
>>> x86/compressed: efi-mixed: simplify IDT/GDT preserve/restore
>>> x86/compressed: avoid touching ECX in startup32_set_idt_entry()
>>> x86/compressed: pull global variable ref up into startup32_load_idt()
>>> x86/compressed: move startup32_load_idt() into .text section
>>> x86/compressed: move startup32_load_idt() out of head_64.S
>>> x86/compressed: move startup32_check_sev_cbit() into .text
>>> x86/compressed: move startup32_check_sev_cbit() out of head_64.S
>>> x86/compressed: adhere to calling convention in
>>> get_sev_encryption_bit()
>>> x86/compressed: only build mem_encrypt.S if AMD_MEM_ENCRYPT=y
>>> efi: x86: Make the deprecated EFI handover protocol optional
>>>
>>> arch/x86/Kconfig | 17 +
>>> arch/x86/boot/compressed/Makefile | 8 +-
>>> arch/x86/boot/compressed/efi_mixed.S | 351 ++++++++++++++++++++
>>> arch/x86/boot/compressed/efi_thunk_64.S | 195 -----------
>>> arch/x86/boot/compressed/head_32.S | 4 -
>>> arch/x86/boot/compressed/head_64.S | 303 +----------------
>>> arch/x86/boot/compressed/mem_encrypt.S | 152 ++++++++-
>>> arch/x86/boot/header.S | 2 +-
>>> arch/x86/boot/tools/build.c | 2 +
>>> drivers/firmware/efi/libstub/x86-stub.c | 2 +-
>>> 10 files changed, 533 insertions(+), 503 deletions(-)
>>> create mode 100644 arch/x86/boot/compressed/efi_mixed.S
>>> delete mode 100644 arch/x86/boot/compressed/efi_thunk_64.S
>>>

2022-11-22 23:15:13

by Borislav Petkov

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
> I bisected it to:
>
> 99b7f4b23d9f ("x86/boot/compressed, efi: Merge multiple definitions of image_offset into one")
>
> And doing the following fixed it:
>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index cb5f0befee57..a0bfd31358ba 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -23,7 +23,7 @@
> const efi_system_table_t *efi_system_table;
> const efi_dxe_services_table_t *efi_dxe_table;
> -u32 image_offset;
> +u32 image_offset __section(".data");
> static efi_loaded_image_t *image = NULL;
> static efi_status_t
>
> I assume it has to do with being in .data vs .bss and not being explicitly
> cleared with the encryption bit set. With the change to put image_offset in
> the .data section, it is read as zero, where as when it was in the .bss
> section it was reading "ciphertext".

Thank you both. I'll refresh the set and run it here tomorrow too, to
make sure it boots here too.

Thx.

--
Regards/Gruss,
Boris.

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

2022-11-23 11:01:22

by Borislav Petkov

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index cb5f0befee57..a0bfd31358ba 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -23,7 +23,7 @@
> const efi_system_table_t *efi_system_table;
> const efi_dxe_services_table_t *efi_dxe_table;
> -u32 image_offset;
> +u32 image_offset __section(".data");
> static efi_loaded_image_t *image = NULL;
> static efi_status_t
>
> I assume it has to do with being in .data vs .bss and not being explicitly
> cleared with the encryption bit set. With the change to put image_offset in
> the .data section, it is read as zero, where as when it was in the .bss
> section it was reading "ciphertext".

Hmm, two points about this:

1. Can we do

u32 image_offset __bss_decrypted;

here instead? We have this special section just for that fun and it
self-documents this way.

2. Also, why does my SEV-ES guest boot just fine without that change?

[ 0.000000] Linux version 6.1.0-rc6+ (root@ml) (gcc (Debian 11.3.0-1) 11.3.0, GNU ld (GNU Binutils for Debian) 2.38) #1 SMP PREEMPT_DYNAMIC Wed Nov 23 11:27:17 CET 2022
...
[ 0.336132] Memory Encryption Features active: AMD SEV SEV-ES

Thx.

--
Regards/Gruss,
Boris.

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

2022-11-23 11:10:43

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Wed, 23 Nov 2022 at 11:49, Borislav Petkov <[email protected]> wrote:
>
> On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index cb5f0befee57..a0bfd31358ba 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -23,7 +23,7 @@
> > const efi_system_table_t *efi_system_table;
> > const efi_dxe_services_table_t *efi_dxe_table;
> > -u32 image_offset;
> > +u32 image_offset __section(".data");
> > static efi_loaded_image_t *image = NULL;
> > static efi_status_t
> >
> > I assume it has to do with being in .data vs .bss and not being explicitly
> > cleared with the encryption bit set. With the change to put image_offset in
> > the .data section, it is read as zero, where as when it was in the .bss
> > section it was reading "ciphertext".
>
> Hmm, two points about this:
>
> 1. Can we do
>
> u32 image_offset __bss_decrypted;
>
> here instead? We have this special section just for that fun and it
> self-documents this way.
>

The patch moves it from .data to .bss inadvertently, and I am not
convinced Tom's analysis is entirely accurate: we may simply have
garbage in image_offset if we access it before .bss gets cleared.

> 2. Also, why does my SEV-ES guest boot just fine without that change?
>

Indeed, so it needs to be in .data


> [ 0.000000] Linux version 6.1.0-rc6+ (root@ml) (gcc (Debian 11.3.0-1) 11.3.0, GNU ld (GNU Binutils for Debian) 2.38) #1 SMP PREEMPT_DYNAMIC Wed Nov 23 11:27:17 CET 2022
> ...
> [ 0.336132] Memory Encryption Features active: AMD SEV SEV-ES
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

2022-11-23 12:21:34

by Borislav Petkov

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Wed, Nov 23, 2022 at 11:52:32AM +0100, Ard Biesheuvel wrote:
> The patch moves it from .data to .bss inadvertently, and I am not
> convinced Tom's analysis is entirely accurate: we may simply have
> garbage in image_offset if we access it before .bss gets cleared.

That should not be too hard to find out: add an endless loop in asm in
the guest right after the first image_offset access:

1:
jmp 1b

and then dump its value.

Or Tom might have an even better solution.

But looking at the code, BSS clearing happens later, at .Lrelocated and
the EFI stub comes before it. AFAICT.

--
Regards/Gruss,
Boris.

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

2022-11-23 12:40:13

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Wed, 23 Nov 2022 at 12:09, Borislav Petkov <[email protected]> wrote:
>
> On Wed, Nov 23, 2022 at 11:52:32AM +0100, Ard Biesheuvel wrote:
> > The patch moves it from .data to .bss inadvertently, and I am not
> > convinced Tom's analysis is entirely accurate: we may simply have
> > garbage in image_offset if we access it before .bss gets cleared.
>
> That should not be too hard to find out: add an endless loop in asm in
> the guest right after the first image_offset access:
>
> 1:
> jmp 1b
>
> and then dump its value.
>
> Or Tom might have an even better solution.
>
> But looking at the code, BSS clearing happens later, at .Lrelocated and
> the EFI stub comes before it. AFAICT.
>

Indeed. And moving it back into .data makes the most sense in any case
- the point of the patch is to drop the duplicate definitions from asm
code, not to move it into a different section.

The reason I hadn't spotted this is because my boot chain always sets
the value of image_offset during the boot, and does not rely on the
statically initialized value at all.

2022-11-23 14:42:52

by Tom Lendacky

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On 11/23/22 04:49, Borislav Petkov wrote:
> On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>> index cb5f0befee57..a0bfd31358ba 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -23,7 +23,7 @@
>> const efi_system_table_t *efi_system_table;
>> const efi_dxe_services_table_t *efi_dxe_table;
>> -u32 image_offset;
>> +u32 image_offset __section(".data");
>> static efi_loaded_image_t *image = NULL;
>> static efi_status_t
>>
>> I assume it has to do with being in .data vs .bss and not being explicitly
>> cleared with the encryption bit set. With the change to put image_offset in
>> the .data section, it is read as zero, where as when it was in the .bss
>> section it was reading "ciphertext".
>
> Hmm, two points about this:
>
> 1. Can we do
>
> u32 image_offset __bss_decrypted;
>
> here instead? We have this special section just for that fun and it
> self-documents this way.

Yes, but __bss_decrypted is for the main kernel, not the decompression kernel.

The original value was in the .data section of the assembler (before the
patch moved it), which gets initialized when loaded. Having it in the .bss
section where you hope that memory was zeroed before hand is the issue.

>
> 2. Also, why does my SEV-ES guest boot just fine without that change?
>
> [ 0.000000] Linux version 6.1.0-rc6+ (root@ml) (gcc (Debian 11.3.0-1) 11.3.0, GNU ld (GNU Binutils for Debian) 2.38) #1 SMP PREEMPT_DYNAMIC Wed Nov 23 11:27:17 CET 2022
> ...
> [ 0.336132] Memory Encryption Features active: AMD SEV SEV-ES

Are you booting directly using the -kernel option on Qemu or going through
the bootloader. It was only when using Grub that the problem appeared for me.

Thanks,
Tom

>
> Thx.
>

2022-11-23 14:46:29

by Tom Lendacky

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On 11/23/22 04:52, Ard Biesheuvel wrote:
> On Wed, 23 Nov 2022 at 11:49, Borislav Petkov <[email protected]> wrote:
>>
>> On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
>>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>>> index cb5f0befee57..a0bfd31358ba 100644
>>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>>> @@ -23,7 +23,7 @@
>>> const efi_system_table_t *efi_system_table;
>>> const efi_dxe_services_table_t *efi_dxe_table;
>>> -u32 image_offset;
>>> +u32 image_offset __section(".data");
>>> static efi_loaded_image_t *image = NULL;
>>> static efi_status_t
>>>
>>> I assume it has to do with being in .data vs .bss and not being explicitly
>>> cleared with the encryption bit set. With the change to put image_offset in
>>> the .data section, it is read as zero, where as when it was in the .bss
>>> section it was reading "ciphertext".
>>
>> Hmm, two points about this:
>>
>> 1. Can we do
>>
>> u32 image_offset __bss_decrypted;
>>
>> here instead? We have this special section just for that fun and it
>> self-documents this way.
>>
>
> The patch moves it from .data to .bss inadvertently, and I am not
> convinced Tom's analysis is entirely accurate: we may simply have
> garbage in image_offset if we access it before .bss gets cleared.

When running non-encrypted, I imagine all the memory is cleared to zero as
part of Qemu allocating it. As soon as you put an SEV guest on top of
that, host made zeroes will not appear as zeroes to the SEV guest, rather
they will be decrypted and end up looking like ciphertext (hence the
random values I kept seeing in image_offset). The SEV guest must
explicitly clear it, which is why having it in .bss doesn't work for SEV.

Thanks,
Tom

>
>> 2. Also, why does my SEV-ES guest boot just fine without that change?
>>
>
> Indeed, so it needs to be in .data
>
>
>> [ 0.000000] Linux version 6.1.0-rc6+ (root@ml) (gcc (Debian 11.3.0-1) 11.3.0, GNU ld (GNU Binutils for Debian) 2.38) #1 SMP PREEMPT_DYNAMIC Wed Nov 23 11:27:17 CET 2022
>> ...
>> [ 0.336132] Memory Encryption Features active: AMD SEV SEV-ES
>>
>> Thx.
>>
>> --
>> Regards/Gruss,
>> Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette

2022-11-23 15:05:37

by Ard Biesheuvel

Subject: Re: [PATCH v3 00/17] x86: head_64.S spring cleaning

On Wed, 23 Nov 2022 at 15:16, Tom Lendacky <[email protected]> wrote:
>
> On 11/23/22 04:52, Ard Biesheuvel wrote:
> > On Wed, 23 Nov 2022 at 11:49, Borislav Petkov <[email protected]> wrote:
> >>
> >> On Tue, Nov 22, 2022 at 03:49:29PM -0600, Tom Lendacky wrote:
> >>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> >>> index cb5f0befee57..a0bfd31358ba 100644
> >>> --- a/drivers/firmware/efi/libstub/x86-stub.c
> >>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> >>> @@ -23,7 +23,7 @@
> >>> const efi_system_table_t *efi_system_table;
> >>> const efi_dxe_services_table_t *efi_dxe_table;
> >>> -u32 image_offset;
> >>> +u32 image_offset __section(".data");
> >>> static efi_loaded_image_t *image = NULL;
> >>> static efi_status_t
> >>>
> >>> I assume it has to do with being in .data vs .bss and not being explicitly
> >>> cleared with the encryption bit set. With the change to put image_offset in
> >>> the .data section, it is read as zero, where as when it was in the .bss
> >>> section it was reading "ciphertext".
> >>
> >> Hmm, two points about this:
> >>
> >> 1. Can we do
> >>
> >> u32 image_offset __bss_decrypted;
> >>
> >> here instead? We have this special section just for that fun and it
> >> self-documents this way.
> >>
> >
> > The patch moves it from .data to .bss inadvertently, and I am not
> > convinced Tom's analysis is entirely accurate: we may simply have
> > garbage in image_offset if we access it before .bss gets cleared.
>
> When running non-encrypted, I imagine all the memory is cleared to zero as
> part of Qemu allocating it. As soon as you put an SEV guest on top of
> that, host made zeroes will not appear as zeroes to the SEV guest, rather
> they will be decrypted and end up looking like ciphertext (hence the
> random values I kept seeing in image_offset). The SEV guest must
> explicitly clear it, which is why having it in .bss doesn't work for SEV.
>

Yes, QEMU will probably clear it, but GRUB, shim, etc do a terrible
job at implementing image loading correctly, i.e., not bothering to
parse the PE/COFF header at all, but just copying the image into
memory and invoke it at a fixed offset of 0x290 bytes into the image
(the famous EFI handover protocol, see the last patch in this series
if you want to know more :-))

This also means that the balance of the file size vs the image size
(where BSS lives) is completely ignored, and we actually have to
relocate the image from the EFI stub in such cases, because we cannot
be sure that the BSS does not overlap with memory that is already in
use, given the top down allocation strategy the EFI usually employs.

In summary, I guess there are multiple way how we might end up in this
situation, and simply putting the variable back into .data where it
came from is probably the best approach here.

2022-11-24 08:21:09

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed: Adhere to calling convention in get_sev_encryption_bit()

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: 30c9ca16a5271ba6f8ad9c86507ff1c789c94677
Gitweb: https://git.kernel.org/tip/30c9ca16a5271ba6f8ad9c86507ff1c789c94677
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:15 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 24 Nov 2022 08:57:41 +01:00

x86/boot/compressed: Adhere to calling convention in get_sev_encryption_bit()

Make get_sev_encryption_bit() follow the ordinary i386 calling
convention, and only call it if CONFIG_AMD_MEM_ENCRYPT is actually
enabled. This clarifies the calling code, and makes it more
maintainable.

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/head_64.S | 5 +++--
arch/x86/boot/compressed/mem_encrypt.S | 10 ----------
2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index db577fb..6ba2c21 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -180,12 +180,13 @@ SYM_FUNC_START(startup_32)
*/
/*
* If SEV is active then set the encryption mask in the page tables.
- * This will insure that when the kernel is copied and decompressed
+ * This will ensure that when the kernel is copied and decompressed
* it will be done so encrypted.
*/
- call get_sev_encryption_bit
xorl %edx, %edx
#ifdef CONFIG_AMD_MEM_ENCRYPT
+ call get_sev_encryption_bit
+ xorl %edx, %edx
testl %eax, %eax
jz 1f
subl $32, %eax /* Encryption bit is always above bit 31 */
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 14cf04a..e696745 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -18,12 +18,7 @@
.text
.code32
SYM_FUNC_START(get_sev_encryption_bit)
- xor %eax, %eax
-
-#ifdef CONFIG_AMD_MEM_ENCRYPT
push %ebx
- push %ecx
- push %edx

movl $0x80000000, %eax /* CPUID to check the highest leaf */
cpuid
@@ -54,12 +49,7 @@ SYM_FUNC_START(get_sev_encryption_bit)
xor %eax, %eax

.Lsev_exit:
- pop %edx
- pop %ecx
pop %ebx
-
-#endif /* CONFIG_AMD_MEM_ENCRYPT */
-
RET
SYM_FUNC_END(get_sev_encryption_bit)

2022-11-24 08:25:44

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed, efi: Merge multiple definitions of image_offset into one

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: 4b52016247aeaa55ca3e3bc2e03cd91114c145c2
Gitweb: https://git.kernel.org/tip/4b52016247aeaa55ca3e3bc2e03cd91114c145c2
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:07 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 24 Nov 2022 08:55:55 +01:00

x86/boot/compressed, efi: Merge multiple definitions of image_offset into one

There is no need for head_32.S and head_64.S both declaring a copy of
the global 'image_offset' variable, so drop those and make the extern C
declaration the definition.

When image_offset is moved to the .c file, it needs to be placed
particularly in the .data section because it lands by default in the
.bss section which is cleared too late, in .Lrelocated, before the first
access to it and thus garbage gets read, leading to SEV guests exploding
in early boot.

This happens only when the SEV guest kernel is loaded through grub. If
supplied with qemu's -kernel command line option, that memory is always
cleared upfront by qemu and all is fine there.

[ bp: Expand commit message with SEV aspect. ]

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/head_32.S | 4 ----
arch/x86/boot/compressed/head_64.S | 4 ----
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 3b354eb..6589ddd 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -208,10 +208,6 @@ SYM_DATA_START_LOCAL(gdt)
.quad 0x00cf92000000ffff /* __KERNEL_DS */
SYM_DATA_END_LABEL(gdt, SYM_L_LOCAL, gdt_end)

-#ifdef CONFIG_EFI_STUB
-SYM_DATA(image_offset, .long 0)
-#endif
-
/*
* Stack and heap for uncompression
*/
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 36f37f9..34d0395 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -718,10 +718,6 @@ SYM_DATA_START(boot32_idt)
SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
#endif

-#ifdef CONFIG_EFI_STUB
-SYM_DATA(image_offset, .long 0)
-#endif
-
#ifdef CONFIG_AMD_MEM_ENCRYPT
__HEAD
.code32
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 33a7811..a0bfd31 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -23,7 +23,7 @@

const efi_system_table_t *efi_system_table;
const efi_dxe_services_table_t *efi_dxe_table;
-extern u32 image_offset;
+u32 image_offset __section(".data");
static efi_loaded_image_t *image = NULL;

static efi_status_t

2022-11-24 08:29:32

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed: Move startup32_load_idt() out of head_64.S

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: 9ea813be3d345dfb8ac5bf6fbb29e6a63647a39d
Gitweb: https://git.kernel.org/tip/9ea813be3d345dfb8ac5bf6fbb29e6a63647a39d
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:12 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 24 Nov 2022 08:57:41 +01:00

x86/boot/compressed: Move startup32_load_idt() out of head_64.S

Now that startup32_load_idt() has been refactored into an ordinary
callable function, move it into mem-encrypt.S where it belongs.

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/head_64.S | 72 +-------------------------
arch/x86/boot/compressed/mem_encrypt.S | 72 ++++++++++++++++++++++++-
2 files changed, 71 insertions(+), 73 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 7aa147f..16cccc2 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -707,78 +707,6 @@ SYM_DATA_START(boot_idt)
.endr
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

-#ifdef CONFIG_AMD_MEM_ENCRYPT
-SYM_DATA_START(boot32_idt_desc)
- .word boot32_idt_end - boot32_idt - 1
- .long 0
-SYM_DATA_END(boot32_idt_desc)
- .balign 8
-SYM_DATA_START(boot32_idt)
- .rept 32
- .quad 0
- .endr
-SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
-
- .text
- .code32
-/*
- * Write an IDT entry into boot32_idt
- *
- * Parameters:
- *
- * %eax: Handler address
- * %edx: Vector number
- * %ecx: IDT address
- */
-SYM_FUNC_START_LOCAL(startup32_set_idt_entry)
- /* IDT entry address to %ecx */
- leal (%ecx, %edx, 8), %ecx
-
- /* Build IDT entry, lower 4 bytes */
- movl %eax, %edx
- andl $0x0000ffff, %edx # Target code segment offset [15:0]
- orl $(__KERNEL32_CS << 16), %edx # Target code segment selector
-
- /* Store lower 4 bytes to IDT */
- movl %edx, (%ecx)
-
- /* Build IDT entry, upper 4 bytes */
- movl %eax, %edx
- andl $0xffff0000, %edx # Target code segment offset [31:16]
- orl $0x00008e00, %edx # Present, Type 32-bit Interrupt Gate
-
- /* Store upper 4 bytes to IDT */
- movl %edx, 4(%ecx)
-
- RET
-SYM_FUNC_END(startup32_set_idt_entry)
-
-SYM_FUNC_START(startup32_load_idt)
- push %ebp
- push %ebx
-
- call 1f
-1: pop %ebp
-
- leal (boot32_idt - 1b)(%ebp), %ebx
-
- /* #VC handler */
- leal (startup32_vc_handler - 1b)(%ebp), %eax
- movl $X86_TRAP_VC, %edx
- movl %ebx, %ecx
- call startup32_set_idt_entry
-
- /* Load IDT */
- leal (boot32_idt_desc - 1b)(%ebp), %ecx
- movl %ebx, 2(%ecx)
- lidt (%ecx)
-
- pop %ebx
- pop %ebp
- RET
-SYM_FUNC_END(startup32_load_idt)
-#endif
-
/*
* Check for the correct C-bit position when the startup_32 boot-path is used.
*
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index a73e4d7..6747e5e 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -12,6 +12,8 @@
#include <asm/processor-flags.h>
#include <asm/msr.h>
#include <asm/asm-offsets.h>
+#include <asm/segment.h>
+#include <asm/trapnr.h>

.text
.code32
@@ -98,7 +100,7 @@ SYM_CODE_START_LOCAL(sev_es_req_cpuid)
jmp 1b
SYM_CODE_END(sev_es_req_cpuid)

-SYM_CODE_START(startup32_vc_handler)
+SYM_CODE_START_LOCAL(startup32_vc_handler)
pushl %eax
pushl %ebx
pushl %ecx
@@ -184,6 +186,63 @@ SYM_CODE_START(startup32_vc_handler)
jmp .Lfail
SYM_CODE_END(startup32_vc_handler)

+/*
+ * Write an IDT entry into boot32_idt
+ *
+ * Parameters:
+ *
+ * %eax: Handler address
+ * %edx: Vector number
+ * %ecx: IDT address
+ */
+SYM_FUNC_START_LOCAL(startup32_set_idt_entry)
+ /* IDT entry address to %ecx */
+ leal (%ecx, %edx, 8), %ecx
+
+ /* Build IDT entry, lower 4 bytes */
+ movl %eax, %edx
+ andl $0x0000ffff, %edx # Target code segment offset [15:0]
+ orl $(__KERNEL32_CS << 16), %edx # Target code segment selector
+
+ /* Store lower 4 bytes to IDT */
+ movl %edx, (%ecx)
+
+ /* Build IDT entry, upper 4 bytes */
+ movl %eax, %edx
+ andl $0xffff0000, %edx # Target code segment offset [31:16]
+ orl $0x00008e00, %edx # Present, Type 32-bit Interrupt Gate
+
+ /* Store upper 4 bytes to IDT */
+ movl %edx, 4(%ecx)
+
+ RET
+SYM_FUNC_END(startup32_set_idt_entry)
+
+SYM_FUNC_START(startup32_load_idt)
+ push %ebp
+ push %ebx
+
+ call 1f
+1: pop %ebp
+
+ leal (boot32_idt - 1b)(%ebp), %ebx
+
+ /* #VC handler */
+ leal (startup32_vc_handler - 1b)(%ebp), %eax
+ movl $X86_TRAP_VC, %edx
+ movl %ebx, %ecx
+ call startup32_set_idt_entry
+
+ /* Load IDT */
+ leal (boot32_idt_desc - 1b)(%ebp), %ecx
+ movl %ebx, 2(%ecx)
+ lidt (%ecx)
+
+ pop %ebx
+ pop %ebp
+ RET
+SYM_FUNC_END(startup32_load_idt)
+
.code64

#include "../../kernel/sev_verify_cbit.S"
@@ -195,4 +254,15 @@ SYM_CODE_END(startup32_vc_handler)
SYM_DATA(sme_me_mask, .quad 0)
SYM_DATA(sev_status, .quad 0)
SYM_DATA(sev_check_data, .quad 0)
+
+SYM_DATA_START_LOCAL(boot32_idt)
+ .rept 32
+ .quad 0
+ .endr
+SYM_DATA_END(boot32_idt)
+
+SYM_DATA_START_LOCAL(boot32_idt_desc)
+ .word . - boot32_idt - 1
+ .long 0
+SYM_DATA_END(boot32_idt_desc)
#endif

2022-11-24 08:45:58

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed: Move startup32_check_sev_cbit() out of head_64.S

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: 9d7eaae6a071ff1f718e0aa5e610bb712f8cc632
Gitweb: https://git.kernel.org/tip/9d7eaae6a071ff1f718e0aa5e610bb712f8cc632
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:14 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 24 Nov 2022 08:57:41 +01:00

x86/boot/compressed: Move startup32_check_sev_cbit() out of head_64.S

Now that the startup32_check_sev_cbit() routine can execute from
anywhere and behaves like an ordinary function, it can be moved where it
belongs.

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/head_64.S | 71 +-------------------------
arch/x86/boot/compressed/mem_encrypt.S | 68 ++++++++++++++++++++++++-
2 files changed, 68 insertions(+), 71 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 30ba541..db577fb 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -711,77 +711,6 @@ SYM_DATA_START(boot_idt)
SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)

/*
- * Check for the correct C-bit position when the startup_32 boot-path is used.
- *
- * The check makes use of the fact that all memory is encrypted when paging is
- * disabled. The function creates 64 bits of random data using the RDRAND
- * instruction. RDRAND is mandatory for SEV guests, so always available. If the
- * hypervisor violates that the kernel will crash right here.
- *
- * The 64 bits of random data are stored to a memory location and at the same
- * time kept in the %eax and %ebx registers. Since encryption is always active
- * when paging is off the random data will be stored encrypted in main memory.
- *
- * Then paging is enabled. When the C-bit position is correct all memory is
- * still mapped encrypted and comparing the register values with memory will
- * succeed. An incorrect C-bit position will map all memory unencrypted, so that
- * the compare will use the encrypted random data and fail.
- */
-#ifdef CONFIG_AMD_MEM_ENCRYPT
- .text
-SYM_FUNC_START(startup32_check_sev_cbit)
- pushl %ebx
- pushl %ebp
-
- call 0f
-0: popl %ebp
-
- /* Check for non-zero sev_status */
- movl (sev_status - 0b)(%ebp), %eax
- testl %eax, %eax
- jz 4f
-
- /*
- * Get two 32-bit random values - Don't bail out if RDRAND fails
- * because it is better to prevent forward progress if no random value
- * can be gathered.
- */
-1: rdrand %eax
- jnc 1b
-2: rdrand %ebx
- jnc 2b
-
- /* Store to memory and keep it in the registers */
- leal (sev_check_data - 0b)(%ebp), %ebp
- movl %eax, 0(%ebp)
- movl %ebx, 4(%ebp)
-
- /* Enable paging to see if encryption is active */
- movl %cr0, %edx /* Backup %cr0 in %edx */
- movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
- movl %ecx, %cr0
-
- cmpl %eax, 0(%ebp)
- jne 3f
- cmpl %ebx, 4(%ebp)
- jne 3f
-
- movl %edx, %cr0 /* Restore previous %cr0 */
-
- jmp 4f
-
-3: /* Check failed - hlt the machine */
- hlt
- jmp 3b
-
-4:
- popl %ebp
- popl %ebx
- RET
-SYM_FUNC_END(startup32_check_sev_cbit)
-#endif
-
-/*
* Stack and heap for uncompression
*/
.bss
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 6747e5e..14cf04a 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -243,6 +243,74 @@ SYM_FUNC_START(startup32_load_idt)
RET
SYM_FUNC_END(startup32_load_idt)

+/*
+ * Check for the correct C-bit position when the startup_32 boot-path is used.
+ *
+ * The check makes use of the fact that all memory is encrypted when paging is
+ * disabled. The function creates 64 bits of random data using the RDRAND
+ * instruction. RDRAND is mandatory for SEV guests, so always available. If the
+ * hypervisor violates that the kernel will crash right here.
+ *
+ * The 64 bits of random data are stored to a memory location and at the same
+ * time kept in the %eax and %ebx registers. Since encryption is always active
+ * when paging is off the random data will be stored encrypted in main memory.
+ *
+ * Then paging is enabled. When the C-bit position is correct all memory is
+ * still mapped encrypted and comparing the register values with memory will
+ * succeed. An incorrect C-bit position will map all memory unencrypted, so that
+ * the compare will use the encrypted random data and fail.
+ */
+SYM_FUNC_START(startup32_check_sev_cbit)
+ pushl %ebx
+ pushl %ebp
+
+ call 0f
+0: popl %ebp
+
+ /* Check for non-zero sev_status */
+ movl (sev_status - 0b)(%ebp), %eax
+ testl %eax, %eax
+ jz 4f
+
+ /*
+ * Get two 32-bit random values - Don't bail out if RDRAND fails
+ * because it is better to prevent forward progress if no random value
+ * can be gathered.
+ */
+1: rdrand %eax
+ jnc 1b
+2: rdrand %ebx
+ jnc 2b
+
+ /* Store to memory and keep it in the registers */
+ leal (sev_check_data - 0b)(%ebp), %ebp
+ movl %eax, 0(%ebp)
+ movl %ebx, 4(%ebp)
+
+ /* Enable paging to see if encryption is active */
+ movl %cr0, %edx /* Backup %cr0 in %edx */
+ movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
+ movl %ecx, %cr0
+
+ cmpl %eax, 0(%ebp)
+ jne 3f
+ cmpl %ebx, 4(%ebp)
+ jne 3f
+
+ movl %edx, %cr0 /* Restore previous %cr0 */
+
+ jmp 4f
+
+3: /* Check failed - hlt the machine */
+ hlt
+ jmp 3b
+
+4:
+ popl %ebp
+ popl %ebx
+ RET
+SYM_FUNC_END(startup32_check_sev_cbit)
+
.code64

#include "../../kernel/sev_verify_cbit.S"

2022-11-24 08:52:15

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed: Move startup32_check_sev_cbit() into .text

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: b5d854cd4b6a314edd6c15dabc4233b84a0f8e5e
Gitweb: https://git.kernel.org/tip/b5d854cd4b6a314edd6c15dabc4233b84a0f8e5e
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:13 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Thu, 24 Nov 2022 08:57:41 +01:00

x86/boot/compressed: Move startup32_check_sev_cbit() into .text

Move startup32_check_sev_cbit() into the .text section and turn it into
an ordinary function using the ordinary 32-bit calling convention,
instead of saving/restoring the registers that are known to be live at
the only call site. This improves maintainability, and makes it possible
to move this function out of head_64.S and into a separate compilation
unit that is specific to memory encryption.

Note that this requires the call site to be moved before the mixed mode
check, as %eax will be live otherwise.

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/head_64.S | 35 +++++++++++++++--------------
1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 16cccc2..30ba541 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -251,6 +251,11 @@ SYM_FUNC_START(startup_32)
movl $__BOOT_TSS, %eax
ltr %ax

+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ /* Check if the C-bit position is correct when SEV is active */
+ call startup32_check_sev_cbit
+#endif
+
/*
* Setup for the jump to 64bit mode
*
@@ -268,8 +273,6 @@ SYM_FUNC_START(startup_32)
leal rva(startup_64_mixed_mode)(%ebp), %eax
1:
#endif
- /* Check if the C-bit position is correct when SEV is active */
- call startup32_check_sev_cbit

pushl $__KERNEL_CS
pushl %eax
@@ -724,16 +727,17 @@ SYM_DATA_END_LABEL(boot_idt, SYM_L_GLOBAL, boot_idt_end)
* succeed. An incorrect C-bit position will map all memory unencrypted, so that
* the compare will use the encrypted random data and fail.
*/
- __HEAD
-SYM_FUNC_START(startup32_check_sev_cbit)
#ifdef CONFIG_AMD_MEM_ENCRYPT
- pushl %eax
+ .text
+SYM_FUNC_START(startup32_check_sev_cbit)
pushl %ebx
- pushl %ecx
- pushl %edx
+ pushl %ebp
+
+ call 0f
+0: popl %ebp

/* Check for non-zero sev_status */
- movl rva(sev_status)(%ebp), %eax
+ movl (sev_status - 0b)(%ebp), %eax
testl %eax, %eax
jz 4f

@@ -748,17 +752,18 @@ SYM_FUNC_START(startup32_check_sev_cbit)
jnc 2b

/* Store to memory and keep it in the registers */
- movl %eax, rva(sev_check_data)(%ebp)
- movl %ebx, rva(sev_check_data+4)(%ebp)
+ leal (sev_check_data - 0b)(%ebp), %ebp
+ movl %eax, 0(%ebp)
+ movl %ebx, 4(%ebp)

/* Enable paging to see if encryption is active */
movl %cr0, %edx /* Backup %cr0 in %edx */
movl $(X86_CR0_PG | X86_CR0_PE), %ecx /* Enable Paging and Protected mode */
movl %ecx, %cr0

- cmpl %eax, rva(sev_check_data)(%ebp)
+ cmpl %eax, 0(%ebp)
jne 3f
- cmpl %ebx, rva(sev_check_data+4)(%ebp)
+ cmpl %ebx, 4(%ebp)
jne 3f

movl %edx, %cr0 /* Restore previous %cr0 */
@@ -770,13 +775,11 @@ SYM_FUNC_START(startup32_check_sev_cbit)
jmp 3b

4:
- popl %edx
- popl %ecx
+ popl %ebp
popl %ebx
- popl %eax
-#endif
RET
SYM_FUNC_END(startup32_check_sev_cbit)
+#endif

/*
* Stack and heap for uncompression

2022-11-24 08:52:42

by tip-bot2 for Alexey Makhalov

Subject: [tip: x86/boot] x86/boot/compressed: Move efi32_pe_entry() out of head_64.S

The following commit has been merged into the x86/boot branch of tip:

Commit-ID: 7f22ca396778fea9332d83ec2359dbe8396e9a06
Gitweb: https://git.kernel.org/tip/7f22ca396778fea9332d83ec2359dbe8396e9a06
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Tue, 22 Nov 2022 17:10:06 +01:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Tue, 22 Nov 2022 19:24:57 +01:00

x86/boot/compressed: Move efi32_pe_entry() out of head_64.S

Move the implementation of efi32_pe_entry() into efi-mixed.S, which is a
more suitable location that only gets built if EFI mixed mode is
actually enabled.

Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
arch/x86/boot/compressed/efi_mixed.S | 82 +++++++++++++++++++++++++-
arch/x86/boot/compressed/head_64.S | 87 +---------------------------
2 files changed, 83 insertions(+), 86 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_mixed.S b/arch/x86/boot/compressed/efi_mixed.S
index 3487484..8844d8e 100644
--- a/arch/x86/boot/compressed/efi_mixed.S
+++ b/arch/x86/boot/compressed/efi_mixed.S
@@ -257,6 +257,88 @@ SYM_FUNC_START(efi32_entry)
jmp startup_32
SYM_FUNC_END(efi32_entry)

+#define ST32_boottime 60 // offsetof(efi_system_table_32_t, boottime)
+#define BS32_handle_protocol 88 // offsetof(efi_boot_services_32_t, handle_protocol)
+#define LI32_image_base 32 // offsetof(efi_loaded_image_32_t, image_base)
+
+/*
+ * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
+ * efi_system_table_32_t *sys_table)
+ */
+SYM_FUNC_START(efi32_pe_entry)
+ pushl %ebp
+ movl %esp, %ebp
+ pushl %eax // dummy push to allocate loaded_image
+
+ pushl %ebx // save callee-save registers
+ pushl %edi
+
+ call verify_cpu // check for long mode support
+ testl %eax, %eax
+ movl $0x80000003, %eax // EFI_UNSUPPORTED
+ jnz 2f
+
+ call 1f
+1: pop %ebx
+
+ /* Get the loaded image protocol pointer from the image handle */
+ leal -4(%ebp), %eax
+ pushl %eax // &loaded_image
+ leal (loaded_image_proto - 1b)(%ebx), %eax
+ pushl %eax // pass the GUID address
+ pushl 8(%ebp) // pass the image handle
+
+ /*
+ * Note the alignment of the stack frame.
+ * sys_table
+ * handle <-- 16-byte aligned on entry by ABI
+ * return address
+ * frame pointer
+ * loaded_image <-- local variable
+ * saved %ebx <-- 16-byte aligned here
+ * saved %edi
+ * &loaded_image
+ * &loaded_image_proto
+ * handle <-- 16-byte aligned for call to handle_protocol
+ */
+
+ movl 12(%ebp), %eax // sys_table
+ movl ST32_boottime(%eax), %eax // sys_table->boottime
+ call *BS32_handle_protocol(%eax) // sys_table->boottime->handle_protocol
+ addl $12, %esp // restore argument space
+ testl %eax, %eax
+ jnz 2f
+
+ movl 8(%ebp), %ecx // image_handle
+ movl 12(%ebp), %edx // sys_table
+ movl -4(%ebp), %esi // loaded_image
+ movl LI32_image_base(%esi), %esi // loaded_image->image_base
+ leal (startup_32 - 1b)(%ebx), %ebp // runtime address of startup_32
+ /*
+ * We need to set the image_offset variable here since startup_32() will
+ * use it before we get to the 64-bit efi_pe_entry() in C code.
+ */
+ subl %esi, %ebp // calculate image_offset
+ movl %ebp, (image_offset - 1b)(%ebx) // save image_offset
+ xorl %esi, %esi
+ jmp efi32_entry // pass %ecx, %edx, %esi
+ // no other registers remain live
+
+2: popl %edi // restore callee-save registers
+ popl %ebx
+ leave
+ RET
+SYM_FUNC_END(efi32_pe_entry)
+
+ .section ".rodata"
+ /* EFI loaded image protocol GUID */
+ .balign 4
+SYM_DATA_START_LOCAL(loaded_image_proto)
+ .long 0x5b1b31a1
+ .word 0x9562, 0x11d2
+ .byte 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b
+SYM_DATA_END(loaded_image_proto)
+
.data
.balign 8
SYM_DATA_START_LOCAL(efi32_boot_gdt)
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 9cd2a28..36f37f9 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -673,6 +673,7 @@ SYM_FUNC_START_LOCAL_NOALIGN(.Lno_longmode)
jmp 1b
SYM_FUNC_END(.Lno_longmode)

+ .globl verify_cpu
#include "../../kernel/verify_cpu.S"

.data
@@ -720,92 +721,6 @@ SYM_DATA_END_LABEL(boot32_idt, SYM_L_GLOBAL, boot32_idt_end)
#ifdef CONFIG_EFI_STUB
SYM_DATA(image_offset, .long 0)
#endif
-#ifdef CONFIG_EFI_MIXED
-#define ST32_boottime 60 // offsetof(efi_system_table_32_t, boottime)
-#define BS32_handle_protocol 88 // offsetof(efi_boot_services_32_t, handle_protocol)
-#define LI32_image_base 32 // offsetof(efi_loaded_image_32_t, image_base)
-
- .text
- .code32
-SYM_FUNC_START(efi32_pe_entry)
-/*
- * efi_status_t efi32_pe_entry(efi_handle_t image_handle,
- * efi_system_table_32_t *sys_table)
- */
-
- pushl %ebp
- movl %esp, %ebp
- pushl %eax // dummy push to allocate loaded_image
-
- pushl %ebx // save callee-save registers
- pushl %edi
-
- call verify_cpu // check for long mode support
- testl %eax, %eax
- movl $0x80000003, %eax // EFI_UNSUPPORTED
- jnz 2f
-
- call 1f
-1: pop %ebx
-
- /* Get the loaded image protocol pointer from the image handle */
- leal -4(%ebp), %eax
- pushl %eax // &loaded_image
- leal (loaded_image_proto - 1b)(%ebx), %eax
- pushl %eax // pass the GUID address
- pushl 8(%ebp) // pass the image handle
-
- /*
- * Note the alignment of the stack frame.
- * sys_table
- * handle <-- 16-byte aligned on entry by ABI
- * return address
- * frame pointer
- * loaded_image <-- local variable
- * saved %ebx <-- 16-byte aligned here
- * saved %edi
- * &loaded_image
- * &loaded_image_proto
- * handle <-- 16-byte aligned for call to handle_protocol
- */
-
- movl 12(%ebp), %eax // sys_table
- movl ST32_boottime(%eax), %eax // sys_table->boottime
- call *BS32_handle_protocol(%eax) // sys_table->boottime->handle_protocol
- addl $12, %esp // restore argument space
- testl %eax, %eax
- jnz 2f
-
- movl 8(%ebp), %ecx // image_handle
- movl 12(%ebp), %edx // sys_table
- movl -4(%ebp), %esi // loaded_image
- movl LI32_image_base(%esi), %esi // loaded_image->image_base
- leal (startup_32 - 1b)(%ebx), %ebp // runtime address of startup_32
- /*
- * We need to set the image_offset variable here since startup_32() will
- * use it before we get to the 64-bit efi_pe_entry() in C code.
- */
- subl %esi, %ebp // calculate image_offset
- movl %ebp, (image_offset - 1b)(%ebx) // save image_offset
- xorl %esi, %esi
- jmp efi32_entry // pass %ecx, %edx, %esi
- // no other registers remain live
-
-2: popl %edi // restore callee-save registers
- popl %ebx
- leave
- RET
-SYM_FUNC_END(efi32_pe_entry)
-
- .section ".rodata"
- /* EFI loaded image protocol GUID */
- .balign 4
-SYM_DATA_START_LOCAL(loaded_image_proto)
- .long 0x5b1b31a1
- .word 0x9562, 0x11d2
- .byte 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b
-SYM_DATA_END(loaded_image_proto)
-#endif

#ifdef CONFIG_AMD_MEM_ENCRYPT
__HEAD