[PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Hi,

these patches add support for the 32-bit boot in the decompressor
code. This is needed to boot an SEV-ES guest on some firmware and grub
versions. The patches also add the necessary CPUID sanity checks and a
32-bit version of the C-bit check.

Other updates included here:

	1. Add code to shut down exception handling in the
	   decompressor code before jumping to the real kernel.
	   Once in the real kernel it is not safe anymore to jump
	   back to the decompressor code via exceptions.

	2. Replace open-coded hlt loops with proper calls to
	   sev_es_terminate().

Please review.

Thanks,

	Joerg

Joerg Roedel (7):
  x86/boot/compressed/64: Cleanup exception handling before booting
    kernel
  x86/boot/compressed/64: Reload CS in startup_32
  x86/boot/compressed/64: Setup IDT in startup_32 boot path
  x86/boot/compressed/64: Add 32-bit boot #VC handler
  x86/boot/compressed/64: Add CPUID sanity check to 32-bit boot-path
  x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path
  x86/sev-es: Replace open-coded hlt-loops with sev_es_terminate()

 arch/x86/boot/compressed/head_64.S     | 168 ++++++++++++++++++++++++-
 arch/x86/boot/compressed/idt_64.c      |  14 +++
 arch/x86/boot/compressed/mem_encrypt.S | 114 ++++++++++++++++-
 arch/x86/boot/compressed/misc.c        |   7 +-
 arch/x86/boot/compressed/misc.h        |   6 +
 arch/x86/boot/compressed/sev-es.c      |  12 +-
 arch/x86/kernel/sev-es-shared.c        |  10 +-
 7 files changed, 307 insertions(+), 24 deletions(-)

-- 
2.30.0

[PATCH 1/7] x86/boot/compressed/64: Cleanup exception handling before booting kernel

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Disable the exception handling before booting the kernel to make sure
any exceptions that happen during early kernel boot are not directed to
the pre-decompression code.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/idt_64.c | 14 ++++++++++++++
 arch/x86/boot/compressed/misc.c   |  7 ++-----
 arch/x86/boot/compressed/misc.h   |  6 ++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/compressed/idt_64.c b/arch/x86/boot/compressed/idt_64.c
index 804a502ee0d2..9b93567d663a 100644
--- a/arch/x86/boot/compressed/idt_64.c
+++ b/arch/x86/boot/compressed/idt_64.c
@@ -52,3 +52,17 @@ void load_stage2_idt(void)
 
 	load_boot_idt(&boot_idt_desc);
 }
+
+void cleanup_exception_handling(void)
+{
+	/*
+	 * Flush GHCB from cache and map it encrypted again when running as
+	 * SEV-ES guest.
+	 */
+	sev_es_shutdown_ghcb();
+
+	/* Set a null-idt, disabling #PF and #VC handling */
+	boot_idt_desc.size    = 0;
+	boot_idt_desc.address = 0;
+	load_boot_idt(&boot_idt_desc);
+}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 267e7f93050e..cc9fd0e8766a 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -443,11 +443,8 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
 	handle_relocations(output, output_len, virt_addr);
 	debug_putstr("done.\nBooting the kernel.\n");
 
-	/*
-	 * Flush GHCB from cache and map it encrypted again when running as
-	 * SEV-ES guest.
-	 */
-	sev_es_shutdown_ghcb();
+	/* Disable exception handling before booting the kernel */
+	cleanup_exception_handling();
 
 	return output;
 }
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 901ea5ebec22..e5612f035498 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -155,6 +155,12 @@ extern pteval_t __default_kernel_pte_mask;
 extern gate_desc boot_idt[BOOT_IDT_ENTRIES];
 extern struct desc_ptr boot_idt_desc;
 
+#ifdef CONFIG_X86_64
+void cleanup_exception_handling(void);
+#else
+static inline void cleanup_exception_handling(void) { }
+#endif
+
 /* IDT Entry Points */
 void boot_page_fault(void);
 void boot_stage1_vc(void);
-- 
2.30.0

[PATCH 2/7] x86/boot/compressed/64: Reload CS in startup_32

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Exception handling in the startup_32 boot path requires the CS
selector to be correctly set up. Reload it from the current GDT.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/head_64.S | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index e94874f4bbc1..c59c80ca546d 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -107,9 +107,16 @@ SYM_FUNC_START(startup_32)
 	movl	%eax, %gs
 	movl	%eax, %ss
 
-/* setup a stack and make sure cpu supports long mode. */
+	/* Setup a stack and load CS from current GDT */
 	leal	rva(boot_stack_end)(%ebp), %esp
 
+	pushl	$__KERNEL32_CS
+	leal	rva(1f)(%ebp), %eax
+	pushl	%eax
+	lretl
+1:
+
+	/* Make sure cpu supports long mode. */
 	call	verify_cpu
 	testl	%eax, %eax
 	jnz	.Lno_longmode
-- 
2.30.0

[PATCH 3/7] x86/boot/compressed/64: Setup IDT in startup_32 boot path

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This boot path needs exception handling when it is used with SEV-ES.
Setup an IDT and provide a helper function to write IDT entries for
use in 32-bit protected mode.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/head_64.S | 73 +++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index c59c80ca546d..8deeec78cdb4 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -116,6 +116,11 @@ SYM_FUNC_START(startup_32)
 	lretl
 1:
 
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+	/* Setup Exception handling for SEV-ES */
+	call	startup32_load_idt
+#endif
+
 	/* Make sure cpu supports long mode. */
 	call	verify_cpu
 	testl	%eax, %eax
@@ -528,7 +533,6 @@ SYM_FUNC_END_ALIAS(efi_stub_entry)
 
 	.text
 SYM_FUNC_START_LOCAL_NOALIGN(.Lrelocated)
-
 /*
  * Clear BSS (stack is currently empty)
  */
@@ -701,6 +705,19 @@ 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)
+#endif
+
 #ifdef CONFIG_EFI_STUB
 SYM_DATA(image_offset, .long 0)
 #endif
@@ -793,6 +810,60 @@ SYM_DATA_START_LOCAL(loaded_image_proto)
 SYM_DATA_END(loaded_image_proto)
 #endif
 
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+	__HEAD
+	.code32
+/*
+ * Write an IDT entry into boot32_idt
+ *
+ * Parameters:
+ *
+ * %eax:	Handler address
+ * %edx:	Vector number
+ *
+ * Physical offset is expected in %ebp
+ */
+SYM_FUNC_START(startup32_set_idt_entry)
+	push    %ebx
+	push    %ecx
+
+	/* IDT entry address to %ebx */
+	leal    rva(boot32_idt)(%ebp), %ebx
+	shl	$3, %edx
+	addl    %edx, %ebx
+
+	/* Build IDT entry, lower 4 bytes */
+	movl    %eax, %edx
+	andl    $0x0000ffff, %edx
+	movl    $__KERNEL32_CS, %ecx
+	shl     $16, %ecx
+	orl     %ecx, %edx
+
+	/* Store lower 4 bytes to IDT */
+	movl    %edx, (%ebx)
+
+	/* Build IDT entry, upper 4 bytes */
+	movl    %eax, %edx
+	andl    $0xffff0000, %edx
+	orl     $0x00008e00, %edx
+
+	/* Store upper 4 bytes to IDT */
+	movl    %edx, 4(%ebx)
+
+	pop     %ecx
+	pop     %ebx
+	ret
+SYM_FUNC_END(startup32_set_idt_entry)
+
+SYM_FUNC_START(startup32_load_idt)
+	/* Load IDT */
+	leal	rva(boot32_idt)(%ebp), %eax
+	movl	%eax, rva(boot32_idt_desc+2)(%ebp)
+	lidt    rva(boot32_idt_desc)(%ebp)
+
+	ret
+SYM_FUNC_END(startup32_load_idt)
+#endif
 /*
  * Stack and heap for uncompression
  */
-- 
2.30.0

Re: [PATCH 3/7] x86/boot/compressed/64: Setup IDT in startup_32 boot path

[Borislav Petkov]

On Wed, Feb 10, 2021 at 11:21:31AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This boot path needs exception handling when it is used with SEV-ES.

For ?

Let's explain pls.

> Setup an IDT and provide a helper function to write IDT entries for
> use in 32-bit protected mode.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/boot/compressed/head_64.S | 73 +++++++++++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index c59c80ca546d..8deeec78cdb4 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -116,6 +116,11 @@ SYM_FUNC_START(startup_32)
>  	lretl
>  1:
>  
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +	/* Setup Exception handling for SEV-ES */
> +	call	startup32_load_idt
> +#endif
> +

You can push that ifdeffery out of the main path (diff ontop):

---
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 8deeec78cdb4..cb5a6849fb29 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -116,10 +116,7 @@ SYM_FUNC_START(startup_32)
 	lretl
 1:
 
-#ifdef CONFIG_AMD_MEM_ENCRYPT
-	/* Setup Exception handling for SEV-ES */
 	call	startup32_load_idt
-#endif
 
 	/* Make sure cpu supports long mode. */
 	call	verify_cpu
@@ -854,16 +851,18 @@ SYM_FUNC_START(startup32_set_idt_entry)
 	pop     %ebx
 	ret
 SYM_FUNC_END(startup32_set_idt_entry)
+#endif
 
+/* Setup Exception handling for SEV-ES */
 SYM_FUNC_START(startup32_load_idt)
+#ifdef CONFIG_AMD_MEM_ENCRYPT
 	/* Load IDT */
 	leal	rva(boot32_idt)(%ebp), %eax
 	movl	%eax, rva(boot32_idt_desc+2)(%ebp)
 	lidt    rva(boot32_idt_desc)(%ebp)
-
+#endif
 	ret
 SYM_FUNC_END(startup32_load_idt)
-#endif
 /*
  * Stack and heap for uncompression
  */
---

> +SYM_FUNC_START(startup32_set_idt_entry)
> +	push    %ebx
> +	push    %ecx
> +
> +	/* IDT entry address to %ebx */
> +	leal    rva(boot32_idt)(%ebp), %ebx
> +	shl	$3, %edx
> +	addl    %edx, %ebx
> +
> +	/* Build IDT entry, lower 4 bytes */
> +	movl    %eax, %edx

Let's add some side comments here:

 +	andl    $0x0000ffff, %edx		# Target code segment offset [15:0]


 +	movl    $__KERNEL32_CS, %ecx		# Target code segment selector 
> +	shl     $16, %ecx
> +	orl     %ecx, %edx
> +
> +	/* Store lower 4 bytes to IDT */
> +	movl    %edx, (%ebx)
> +
> +	/* 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

so that a reader like me can quickly find interrupt gates in the docs.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH 4/7] x86/boot/compressed/64: Add 32-bit boot #VC handler

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Add a #VC exception handler which is used when the kernel still executes
in protected mode. This boot-path already uses CPUID, which will cause #VC
exceptions in an SEV-ES guest.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/head_64.S     |  6 ++
 arch/x86/boot/compressed/mem_encrypt.S | 77 +++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 8deeec78cdb4..eadaa0a082b8 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -34,6 +34,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/bootparam.h>
 #include <asm/desc_defs.h>
+#include <asm/trapnr.h>
 #include "pgtable.h"
 
 /*
@@ -856,6 +857,11 @@ SYM_FUNC_START(startup32_set_idt_entry)
 SYM_FUNC_END(startup32_set_idt_entry)
 
 SYM_FUNC_START(startup32_load_idt)
+	/* #VC handler */
+	leal    rva(startup32_vc_handler)(%ebp), %eax
+	movl    $X86_TRAP_VC, %edx
+	call    startup32_set_idt_entry
+
 	/* Load IDT */
 	leal	rva(boot32_idt)(%ebp), %eax
 	movl	%eax, rva(boot32_idt_desc+2)(%ebp)
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index aa561795efd1..350ecb56c7e4 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -67,10 +67,85 @@ SYM_FUNC_START(get_sev_encryption_bit)
 	ret
 SYM_FUNC_END(get_sev_encryption_bit)
 
+/*
+ * Emit code to request an CPUID register from the Hypervisor using
+ * the MSR-based protocol.
+ *
+ * fn: The register containing the CPUID function
+ * reg: Register requested
+ *	1 = EAX
+ *	2 = EBX
+ *	3 = ECX
+ *	4 = EDX
+ *
+ * Result is in EDX. Jumps to .Lfail on error
+ */
+.macro	SEV_ES_REQ_CPUID fn:req reg:req
+	/* Request CPUID[%ebx].EAX */
+	movl	$\reg, %eax
+	shll	$30, %eax
+	orl	$0x00000004, %eax
+	movl	\fn, %edx
+	movl	$MSR_AMD64_SEV_ES_GHCB, %ecx
+	wrmsr
+	rep; vmmcall
+	rdmsr
+	/* Check response code */
+	andl	$0xfff, %eax
+	cmpl	$5, %eax
+	jne	.Lfail
+	/* All good */
+.endm
+
+SYM_CODE_START(startup32_vc_handler)
+	pushl	%eax
+	pushl	%ebx
+	pushl	%ecx
+	pushl	%edx
+
+	/* Keep CPUID function in %ebx */
+	movl	%eax, %ebx
+
+	/* Check if error-code == SVM_EXIT_CPUID */
+	cmpl	$0x72, 16(%esp)
+	jne	.Lfail
+
+	/* Request CPUID[%ebx].EAX */
+	SEV_ES_REQ_CPUID fn=%ebx reg=0
+	movl	%edx, 12(%esp)
+
+	/* Request CPUID[%ebx].EBX */
+	SEV_ES_REQ_CPUID fn=%ebx reg=1
+	movl	%edx, 8(%esp)
+
+	/* Request CPUID[%ebx].ECX */
+	SEV_ES_REQ_CPUID fn=%ebx reg=2
+	movl	%edx, 4(%esp)
+
+	/* Request CPUID[%ebx].EDX */
+	SEV_ES_REQ_CPUID fn=%ebx reg=3
+	movl	%edx, (%esp)
+
+	popl	%edx
+	popl	%ecx
+	popl	%ebx
+	popl	%eax
+
+	/* Remove error code */
+	addl	$4, %esp
+
+	/* Jump over CPUID instruction */
+	addl	$2, (%esp)
+
+	iret
+.Lfail:
+	hlt
+	jmp .Lfail
+SYM_CODE_END(startup32_vc_handler)
+
 	.code64
 
 #include "../../kernel/sev_verify_cbit.S"
-
 SYM_FUNC_START(set_sev_encryption_mask)
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 	push	%rbp
-- 
2.30.0

Re: [PATCH 4/7] x86/boot/compressed/64: Add 32-bit boot #VC handler

[Borislav Petkov]

On Wed, Feb 10, 2021 at 11:21:32AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Add a #VC exception handler which is used when the kernel still executes
> in protected mode. This boot-path already uses CPUID, which will cause #VC
> exceptions in an SEV-ES guest.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/boot/compressed/head_64.S     |  6 ++
>  arch/x86/boot/compressed/mem_encrypt.S | 77 +++++++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index 8deeec78cdb4..eadaa0a082b8 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -34,6 +34,7 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/bootparam.h>
>  #include <asm/desc_defs.h>
> +#include <asm/trapnr.h>
>  #include "pgtable.h"
>  
>  /*
> @@ -856,6 +857,11 @@ SYM_FUNC_START(startup32_set_idt_entry)
>  SYM_FUNC_END(startup32_set_idt_entry)
>  
>  SYM_FUNC_START(startup32_load_idt)
> +	/* #VC handler */
> +	leal    rva(startup32_vc_handler)(%ebp), %eax
> +	movl    $X86_TRAP_VC, %edx
> +	call    startup32_set_idt_entry
> +
>  	/* Load IDT */
>  	leal	rva(boot32_idt)(%ebp), %eax
>  	movl	%eax, rva(boot32_idt_desc+2)(%ebp)
> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> index aa561795efd1..350ecb56c7e4 100644
> --- a/arch/x86/boot/compressed/mem_encrypt.S
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -67,10 +67,85 @@ SYM_FUNC_START(get_sev_encryption_bit)
>  	ret
>  SYM_FUNC_END(get_sev_encryption_bit)
>  
> +/*
> + * Emit code to request an CPUID register from the Hypervisor using
> + * the MSR-based protocol.
> + *
> + * fn: The register containing the CPUID function
> + * reg: Register requested
> + *	1 = EAX
> + *	2 = EBX
> + *	3 = ECX
> + *	4 = EDX
> + *
> + * Result is in EDX. Jumps to .Lfail on error
> + */
> +.macro	SEV_ES_REQ_CPUID fn:req reg:req

I'm wondering - instead of replicating this 4 times, can this be a
function which you CALL? You do have a stack so you should be able to.

> +	/* Request CPUID[%ebx].EAX */
> +	movl	$\reg, %eax
> +	shll	$30, %eax
> +	orl	$0x00000004, %eax
> +	movl	\fn, %edx
> +	movl	$MSR_AMD64_SEV_ES_GHCB, %ecx
> +	wrmsr
> +	rep; vmmcall
> +	rdmsr
> +	/* Check response code */

Before you do that, I guess you wanna check:

GHCBData[29:12] – Reserved, must be zero

in the HV response.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH 5/7] x86/boot/compressed/64: Add CPUID sanity check to 32-bit boot-path

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The 32-bit #VC handler has no GHCB and can only handle CPUID exit codes.
It is needed by the early boot code to handle #VC exceptions raised in
verify_cpu() and to get the position of the C bit.

But the CPUID information comes from the hypervisor, which is untrusted
and might return results which trick the guest into the no-SEV boot path
with no C bit set in the page-tables. All data written to memory would
then be unencrypted and could leak sensitive data to the hypervisor.

Add sanity checks to the 32-bit boot #VC handler to make sure the
hypervisor does not pretend that SEV is not enabled.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/mem_encrypt.S | 36 ++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 350ecb56c7e4..091502cde070 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -126,6 +126,34 @@ SYM_CODE_START(startup32_vc_handler)
 	SEV_ES_REQ_CPUID fn=%ebx reg=3
 	movl	%edx, (%esp)
 
+	/*
+	 * Sanity check CPUID results from the Hypervisor. See comment in
+	 * do_vc_no_ghcb() for more details on why this is necessary.
+	 */
+
+	/* Fail if Hypervisor bit not set in CPUID[1].ECX[31] */
+	cmpl    $1, %ebx
+	jne     .Lcheck_leaf
+	btl     $31, 4(%esp)
+	jnc     .Lfail
+	jmp     .Ldone
+
+.Lcheck_leaf:
+	/* Fail if SEV leaf not available in CPUID[0x80000000].EAX */
+	cmpl    $0x80000000, %ebx
+	jne     .Lcheck_sev
+	cmpl    $0x8000001f, 12(%esp)
+	jb      .Lfail
+	jmp     .Ldone
+
+.Lcheck_sev:
+	/* Fail if SEV bit not set in CPUID[0x8000001f].EAX[1] */
+	cmpl    $0x8000001f, %ebx
+	jne     .Ldone
+	btl     $1, 12(%esp)
+	jnc     .Lfail
+
+.Ldone:
 	popl	%edx
 	popl	%ecx
 	popl	%ebx
@@ -139,6 +167,14 @@ SYM_CODE_START(startup32_vc_handler)
 
 	iret
 .Lfail:
+	/* Send terminate request to Hypervisor */
+	movl    $0x100, %eax
+	xorl    %edx, %edx
+	movl    $MSR_AMD64_SEV_ES_GHCB, %ecx
+	wrmsr
+	rep; vmmcall
+
+	/* If request fails, go to hlt loop */
 	hlt
 	jmp .Lfail
 SYM_CODE_END(startup32_vc_handler)
-- 
2.30.0

[PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Check whether the hypervisor reported the correct C-bit when running as
an SEV guest. Using a wrong C-bit position could be used to leak
sensitive data from the guest to the hypervisor.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/head_64.S     | 80 ++++++++++++++++++++++++++
 arch/x86/boot/compressed/mem_encrypt.S |  1 +
 2 files changed, 81 insertions(+)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index eadaa0a082b8..047af1cba041 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -185,11 +185,18 @@ SYM_FUNC_START(startup_32)
 	 */
 	call	get_sev_encryption_bit
 	xorl	%edx, %edx
+#ifdef	CONFIG_AMD_MEM_ENCRYPT
 	testl	%eax, %eax
 	jz	1f
 	subl	$32, %eax	/* Encryption bit is always above bit 31 */
 	bts	%eax, %edx	/* Set encryption mask for page tables */
+	/*
+	 * Store the sme_me_mask as an indicator that SEV is active. It will be
+	 * set again in startup_64().
+	 */
+	movl	%edx, rva(sme_me_mask+4)(%ebp)
 1:
+#endif
 
 	/* Initialize Page tables to 0 */
 	leal	rva(pgtable)(%ebx), %edi
@@ -274,6 +281,9 @@ SYM_FUNC_START(startup_32)
 	movl	%esi, %edx
 1:
 #endif
+	/* Check if the C-bit position is correct when SEV is active */
+	call	sev_startup32_cbit_check
+
 	pushl	$__KERNEL_CS
 	pushl	%eax
 
@@ -870,6 +880,76 @@ SYM_FUNC_START(startup32_load_idt)
 	ret
 SYM_FUNC_END(startup32_load_idt)
 #endif
+
+/*
+ * 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(sev_startup32_cbit_check)
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+	pushl	%eax
+	pushl	%ebx
+	pushl	%ecx
+	pushl	%edx
+
+	/* Check for non-zero sev_status */
+	movl	rva(sev_status)(%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 */
+	movl	%eax, rva(sev_check_data)(%ebp)
+	movl	%ebx, rva(sev_check_data+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)
+	jne	3f
+	cmpl	%ebx, rva(sev_check_data+4)(%ebp)
+	jne	3f
+
+	movl	%edx, %cr0	/* Restore previous %cr0 */
+
+	jmp	4f
+
+3:	/* Check failed - hlt the machine */
+	hlt
+	jmp	3b
+
+4:
+	popl	%edx
+	popl	%ecx
+	popl	%ebx
+	popl	%eax
+#endif
+	ret
+SYM_FUNC_END(sev_startup32_cbit_check)
 /*
  * Stack and heap for uncompression
  */
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 091502cde070..b80fed167903 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -7,6 +7,7 @@
  * Author: Tom Lendacky <thomas.lendacky@amd.com>
  */
 
+#define rva(X) ((X) - startup_32)
 #include <linux/linkage.h>
 
 #include <asm/processor-flags.h>
-- 
2.30.0

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Dave Hansen]

On 2/10/21 2:21 AM, Joerg Roedel wrote:
> +1:	rdrand	%eax
> +	jnc	1b
> +2:	rdrand	%ebx
> +	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)
> +
> +	/* 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)
> +	jne	3f
> +	cmpl	%ebx, rva(sev_check_data+4)(%ebp)
> +	jne	3f
> +
> +	movl	%edx, %cr0	/* Restore previous %cr0 */
> +
> +	jmp	4f

This is all very cute.  But, if this fails, it means that the .data
section is now garbage, right?.  I guess failing here is less
entertaining than trying to run the kernel with random garbage in .data,
but it doesn't make it very far either way, right?

Why bother with rdrand, though?  Couldn't you just pick any old piece of
.data and compare before and after?

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Joerg Roedel]

On Wed, Feb 10, 2021 at 08:25:11AM -0800, Dave Hansen wrote:
> This is all very cute.  But, if this fails, it means that the .data
> section is now garbage, right?.  I guess failing here is less
> entertaining than trying to run the kernel with random garbage in .data,
> but it doesn't make it very far either way, right?

Yes, if this fails the .data section is garbage, and more importantly,
the .text section of the decompressed kernel image would be garbage too.
The kernel won't get very far, but could possibly be tricked into
releasing secrets to the hypervisor.

> Why bother with rdrand, though?  Couldn't you just pick any old piece of
> .data and compare before and after?

It is important that the Hypervisor can't predict what data will be
written. It is written with paging off, so it will implicitly be
encrypted. If the Hypervisor knows the data, it could use the small time
window until it is read again to remap the gpa to a page with the
expected data.

Regards,

	Joerg

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Dave Hansen]

On 2/10/21 2:21 AM, Joerg Roedel wrote:
> +	/* Store to memory and keep it in the registers */
> +	movl	%eax, rva(sev_check_data)(%ebp)
> +	movl	%ebx, rva(sev_check_data+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)
> +	jne	3f
> +	cmpl	%ebx, rva(sev_check_data+4)(%ebp)
> +	jne	3f

Also, I know that turning paging on is a *BIG* barrier.  But, I didn't
think it has any effect on the caches.

I would expect that the underlying physical address of 'sev_check_data'
would change when paging gets enabled because paging sets the C bit.
So, how does the write of 'sev_check_data' get out of the caches and
into memory where it can be read back with the new physical address?

I think there's some bit of the SEV architecture that I'm missing.

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Tom Lendacky]

On 2/10/21 10:47 AM, Dave Hansen wrote:
> On 2/10/21 2:21 AM, Joerg Roedel wrote:
>> +	/* Store to memory and keep it in the registers */
>> +	movl	%eax, rva(sev_check_data)(%ebp)
>> +	movl	%ebx, rva(sev_check_data+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)
>> +	jne	3f
>> +	cmpl	%ebx, rva(sev_check_data+4)(%ebp)
>> +	jne	3f
> 
> Also, I know that turning paging on is a *BIG* barrier.  But, I didn't
> think it has any effect on the caches.
> 
> I would expect that the underlying physical address of 'sev_check_data'
> would change when paging gets enabled because paging sets the C bit.
> So, how does the write of 'sev_check_data' get out of the caches and
> into memory where it can be read back with the new physical address?
> 
> I think there's some bit of the SEV architecture that I'm missing.

Non-paging memory accesses are always considered private (APM Volume 2, 
15.34.4) and thus are cached with the C bit set.

Thanks,
Tom

> 

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Borislav Petkov]

On Wed, Feb 10, 2021 at 11:21:34AM +0100, Joerg Roedel wrote:
> +	/*
> +	 * Store the sme_me_mask as an indicator that SEV is active. It will be
> +	 * set again in startup_64().

So why bother? Or does something needs it before that?

...

> +SYM_FUNC_START(sev_startup32_cbit_check)

s/sev_startup32_cbit_check/startup32_check_sev_cbit/

I guess.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 6/7] x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path

[Joerg Roedel]

On Tue, Mar 02, 2021 at 08:43:53PM +0100, Borislav Petkov wrote:
> On Wed, Feb 10, 2021 at 11:21:34AM +0100, Joerg Roedel wrote:
> > +	/*
> > +	 * Store the sme_me_mask as an indicator that SEV is active. It will be
> > +	 * set again in startup_64().
> 
> So why bother? Or does something needs it before that?

This was actually a bug. The startup32_check_sev_cbit() needs something
to skip the check when SEV is not active. Therefore the value is set
here in sme_me_mask, but the function later checks sev_status.

I fixed it by setting sev_status to 1 here (indicates SEV is active).

Regards,

	Joerg

[PATCH 7/7] x86/sev-es: Replace open-coded hlt-loops with sev_es_terminate()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

There are a few places left in the SEV-ES C code where hlt loops and/or
terminate requests are implemented. Replace them all with calls to
sev_es_terminate().

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/boot/compressed/sev-es.c | 12 +++---------
 arch/x86/kernel/sev-es-shared.c   | 10 +++-------
 2 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/x86/boot/compressed/sev-es.c b/arch/x86/boot/compressed/sev-es.c
index 27826c265aab..d904bd56b3e3 100644
--- a/arch/x86/boot/compressed/sev-es.c
+++ b/arch/x86/boot/compressed/sev-es.c
@@ -200,14 +200,8 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
 	}
 
 finish:
-	if (result == ES_OK) {
+	if (result == ES_OK)
 		vc_finish_insn(&ctxt);
-	} else if (result != ES_RETRY) {
-		/*
-		 * For now, just halt the machine. That makes debugging easier,
-		 * later we just call sev_es_terminate() here.
-		 */
-		while (true)
-			asm volatile("hlt\n");
-	}
+	else if (result != ES_RETRY)
+		sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
 }
diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c
index cdc04d091242..7c34be61258e 100644
--- a/arch/x86/kernel/sev-es-shared.c
+++ b/arch/x86/kernel/sev-es-shared.c
@@ -24,7 +24,7 @@ static bool __init sev_es_check_cpu_features(void)
 	return true;
 }
 
-static void sev_es_terminate(unsigned int reason)
+static void __noreturn sev_es_terminate(unsigned int reason)
 {
 	u64 val = GHCB_SEV_TERMINATE;
 
@@ -210,12 +210,8 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
 	return;
 
 fail:
-	sev_es_wr_ghcb_msr(GHCB_SEV_TERMINATE);
-	VMGEXIT();
-
-	/* Shouldn't get here - if we do halt the machine */
-	while (true)
-		asm volatile("hlt\n");
+	/* Terminate the guest */
+	sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST);
 }
 
 static enum es_result vc_insn_string_read(struct es_em_ctxt *ctxt,
-- 
2.30.0

Re: [PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[Konrad Rzeszutek Wilk]

On Wed, Feb 10, 2021 at 11:21:28AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Hi,
> 
> these patches add support for the 32-bit boot in the decompressor
> code. This is needed to boot an SEV-ES guest on some firmware and grub
> versions. The patches also add the necessary CPUID sanity checks and a

Could you expand a bit please?

What GRUB versions are we talking about (CC-ing Daniel Kiper, who owns
GRUB).

By 'some firmware' we talking SeaBIOS?

> 32-bit version of the C-bit check.
> 
> Other updates included here:
> 
> 	1. Add code to shut down exception handling in the
> 	   decompressor code before jumping to the real kernel.
> 	   Once in the real kernel it is not safe anymore to jump
> 	   back to the decompressor code via exceptions.
> 
> 	2. Replace open-coded hlt loops with proper calls to
> 	   sev_es_terminate().
> 
> Please review.
> 
> Thanks,
> 
> 	Joerg
> 
> Joerg Roedel (7):
>   x86/boot/compressed/64: Cleanup exception handling before booting
>     kernel
>   x86/boot/compressed/64: Reload CS in startup_32
>   x86/boot/compressed/64: Setup IDT in startup_32 boot path
>   x86/boot/compressed/64: Add 32-bit boot #VC handler
>   x86/boot/compressed/64: Add CPUID sanity check to 32-bit boot-path
>   x86/boot/compressed/64: Check SEV encryption in 32-bit boot-path
>   x86/sev-es: Replace open-coded hlt-loops with sev_es_terminate()
> 
>  arch/x86/boot/compressed/head_64.S     | 168 ++++++++++++++++++++++++-
>  arch/x86/boot/compressed/idt_64.c      |  14 +++
>  arch/x86/boot/compressed/mem_encrypt.S | 114 ++++++++++++++++-
>  arch/x86/boot/compressed/misc.c        |   7 +-
>  arch/x86/boot/compressed/misc.h        |   6 +
>  arch/x86/boot/compressed/sev-es.c      |  12 +-
>  arch/x86/kernel/sev-es-shared.c        |  10 +-
>  7 files changed, 307 insertions(+), 24 deletions(-)
> 
> -- 
> 2.30.0
> 

Re: [PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[Joerg Roedel]

Hi Konrad,

On Wed, Feb 10, 2021 at 09:58:35AM -0500, Konrad Rzeszutek Wilk wrote:
> What GRUB versions are we talking about (CC-ing Daniel Kiper, who owns
> GRUB).

I think this was about 32-bit GRUB builds used by distributions. I
personally tested it with a kernel which has EFI support disabled, in
this case the OVMF firmware will also boot into the startup_32 boot
path.

> By 'some firmware' we talking SeaBIOS?

No, SeaBIOS is not supported for SEV-ES, only OVMF has handling for #VC
so far.

Regards,

	Joerg

Re: [PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[Konrad Rzeszutek Wilk]

On Wed, Feb 10, 2021 at 04:12:25PM +0100, Joerg Roedel wrote:
> Hi Konrad,
> 
> On Wed, Feb 10, 2021 at 09:58:35AM -0500, Konrad Rzeszutek Wilk wrote:
> > What GRUB versions are we talking about (CC-ing Daniel Kiper, who owns
> > GRUB).
> 
> I think this was about 32-bit GRUB builds used by distributions. I
> personally tested it with a kernel which has EFI support disabled, in
> this case the OVMF firmware will also boot into the startup_32 boot
> path.

I think I am missing something obvious here - but why would you want
EFI support disabled?

Or is the idea that "legacy" OSes can nicely run under AMD SEV?
But since you are having a kernel patch that is not "legacy OS" anymore.

> 
> > By 'some firmware' we talking SeaBIOS?
> 
> No, SeaBIOS is not supported for SEV-ES, only OVMF has handling for #VC
> so far.
> 
> Regards,
> 
> 	Joerg

Re: [PATCH 0/7] x86/seves: Support 32-bit boot path and other updates

[Joerg Roedel]

On Wed, Feb 10, 2021 at 10:19:38AM -0500, Konrad Rzeszutek Wilk wrote:
> I think I am missing something obvious here - but why would you want
> EFI support disabled?

I don't want EFI support disabled, this is just a way to trigger this
boot-path. In real life it is triggered by 32-bit GRUB EFI builds. But I
havn't had one of those for testing, so I used another way to trigger
this path.

Regards,

	Joerg