x86/sev-es: Mitigate some HV attack vectors

From: Joerg Roedel <[email protected]>

Introduce sev_status and initialize it together with sme_me_mask to have an indicator which SEV features are enabled.

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/boot/compressed/mem_encrypt.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

arch/x86/boot/compressed/mem_encrypt.S +0 -0

index dd07e7b41b11..0effd58f0095 100644

arch/x86/boot/compressed/mem_encrypt.S +11 -1

 	push	%rdx
 
 	movq	%rsp, %rbp		/* Save current stack pointer */

@@ -81,10 +83,19 @@ SYM_FUNC_START(set_sev_encryption_mask)
 
 	bts	%rax, sme_me_mask(%rip)	/* Create the encryption mask */
 
+	/* Read sev_status */
+	movl	$MSR_AMD64_SEV, %ecx
+	rdmsr
+	shlq	$32, %rdx
+	orq	%rdx, %rax
+	movq	%rax, sev_status(%rip)
+
 .Lno_sev_mask:
 	movq	%rbp, %rsp		/* Restore original stack pointer */
 
 	pop	%rdx
+	pop	%rcx
+	pop	%rax
 	pop	%rbp
 #endif
 

@@ -96,5 +107,6 @@ SYM_FUNC_END(set_sev_encryption_mask)
 
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 	.balign	8
-SYM_DATA(sme_me_mask, .quad 0)
+SYM_DATA(sme_me_mask,		.quad 0)
+SYM_DATA(sev_status,		.quad 0)
 #endif

Signature · 1 line

2.28.0

Quoted 10 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

Introduce sev_status and initialize it together with sme_me_mask to have an indicator which SEV features are enabled.

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/boot/compressed/mem_encrypt.S | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

arch/x86/boot/compressed/mem_encrypt.S SYM_FUNC_END(get_sev_encryption_bit) · line 75

Quoted 10 lines

arch/x86/boot/compressed/mem_encrypt.S +2 -0

index dd07e7b41b11..0effd58f0095 100644
@@ -71,6 +71,8 @@ SYM_FUNC_END(get_sev_encryption_bit)
 SYM_FUNC_START(set_sev_encryption_mask)
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 	push	%rbp
+	push	%rax
+	push	%rcx

There's no need to save/restore RAX and RCX, they are callee save. This function is only called from C, so I doubt it's using a custom ABI.

arch/x86/boot/compressed/mem_encrypt.S SYM_FUNC_END(get_sev_encryption_bit) · line 75

Quoted 2 lines · Joerg Roedel

arch/x86/boot/compressed/mem_encrypt.S +2 -0

+	push	%rax
+	push	%rcx

There's no need to save/restore RAX and RCX, they are callee save. This function is only called from C, so I doubt it's using a custom ABI.

From: Joerg Roedel <[email protected]>

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.

The check function is in arch/x86/kernel/sev_verify_cbit.S so that it can be re-used in the running kernel image.

Signed-off-by Joerg Roedel <[email protected]>

---

arch/x86/kernel/sev_verify_cbit.S new file +26 -0

+
+	/*
+	 * Compare value in %r10 with memory location - If C-Bit is incorrect
+	 * this would read the encrypted data and make the check fail.
+	 */
+	cmpq	%r10, sev_check_data(%rip)
+
+	/* Restore old %cr3 */
+	movq	%r11, %cr3
+
+	/* Check CMPQ result */
+	je	3f
+
+	/*
+	 * The check failed - Prevent any forward progress to prevent ROP
+	 * attacks, invalidate the stack and go into a hlt loop.
+	 */
+	xorq	%rsp, %rsp
+	subq	$0x1000, %rsp
+2:	hlt
+	jmp 2b
+3:
+#endif
+	ret
+SYM_FUNC_END(sev_verify_cbit)
+

Signature · 1 line

2.28.0

Quoted 11 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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.

The check function is in arch/x86/kernel/sev_verify_cbit.S so that it can be re-used in the running kernel image.

Signed-off-by Joerg Roedel <[email protected]>

---

arch/x86/kernel/sev_verify_cbit.S line 46

Quoted 4 lines

arch/x86/kernel/sev_verify_cbit.S new file +4 -0

+
+	/* Store value to memory and keep it in %r10 */
+	movq	%r10, sev_check_data(%rip)
+

Does there need to be a cache flush/invalidation between this and the read below to avoid just reading back from cache, or will the hardware take care of that?

arch/x86/kernel/sev_verify_cbit.S line 51

Quoted 6 lines

arch/x86/kernel/sev_verify_cbit.S new file +5 -0

+	/* Backup current %cr3 value to restore it later */
+	movq	%cr3, %r11
+
+	/* Switch to new %cr3 - This might unmap the stack */
+	movq	%rdi, %cr3

Does there need to be a TLB flush after this? When executed from the main kernel's head code, CR4.PGE is enabled, and if the original page mapping had the global bit set (the decompressor stub sets that in the identity mapping), won't the read below still use the original encrypted mapping if we don't explicitly flush it?

Quoted 29 lines

arch/x86/kernel/sev_verify_cbit.S new file +26 -0

+
+	/*
+	 * Compare value in %r10 with memory location - If C-Bit is incorrect
+	 * this would read the encrypted data and make the check fail.
+	 */
+	cmpq	%r10, sev_check_data(%rip)
+
+	/* Restore old %cr3 */
+	movq	%r11, %cr3
+
+	/* Check CMPQ result */
+	je	3f
+
+	/*
+	 * The check failed - Prevent any forward progress to prevent ROP
+	 * attacks, invalidate the stack and go into a hlt loop.
+	 */
+	xorq	%rsp, %rsp
+	subq	$0x1000, %rsp
+2:	hlt
+	jmp 2b
+3:
+#endif
+	ret
+SYM_FUNC_END(sev_verify_cbit)
+

Signature · 1 line

2.28.0

Also, isn't it possible that the initial page tables we're running on have already been messed with and have the C-bit in the wrong location, so that this write happens decrypted?

Quoted 3 lines · Arvind Sankar

Also, isn't it possible that the initial page tables we're running on have already been messed with and have the C-bit in the wrong location, so that this write happens decrypted?

The code assumes that the page-table it is running on has the correct C bit position set and that the code which set it up verified that it is correct. For the kernel itself this is true, at least, but when booting via UEFI the check also needs to happen in the firmware.

Note that the possibilies are limited when the hypervisor reports the wrong C bit position because code fetches always assume encryption, even when the C bit is cleared in the page-table. So a wrong C bit position in the decompression stub would write the kernel image to memory unencrypted and executing it would not be possible.

Regards,

Joerg

Is it possible to take advantage of this to make the check independent of the original page tables? i.e. switch to the new pagetables, then write into .data or .bss the opcodes for a function that does

	movabs	$imm64, %rax
	jmp	*%rdi	// avoid using stack for the return

filling in the imm64 with the RDRAND value, and then try to execute it. If the C-bit value is wrong, this will probably crash, and at any rate shouldn't return with the correct value in %rax.

Quoted 8 lines · Arvind Sankar

Is it possible to take advantage of this to make the check independent of the original page tables? i.e. switch to the new pagetables, then write into .data or .bss the opcodes for a function that does

	movabs	$imm64, %rax
	jmp	*%rdi	// avoid using stack for the return

filling in the imm64 with the RDRAND value, and then try to execute it. If the C-bit value is wrong, this will probably crash, and at any rate shouldn't return with the correct value in %rax.

That could work, but is not reliable. When the C bit is wrong the CPU would essentially execute random data, which could also be a valid instruction stream. A crash is not guaranteed.

That doesn't feel like a big loss: if a malicious hypervisor wanted to induce completely random code execution, it can do that anyway by just messing with the guest-to-host translation, no?

arch/x86/kernel/sev_verify_cbit.S line 46

Quoted 4 lines · Joerg Roedel

arch/x86/kernel/sev_verify_cbit.S new file +4 -0

+
+	/* Store value to memory and keep it in %r10 */
+	movq	%r10, sev_check_data(%rip)
+

Does there need to be a cache flush/invalidation between this and the read below to avoid just reading back from cache, or will the hardware take care of that?

arch/x86/kernel/sev_verify_cbit.S line 51

Quoted 5 lines

arch/x86/kernel/sev_verify_cbit.S new file +5 -0

+	/* Backup current %cr3 value to restore it later */
+	movq	%cr3, %r11
+
+	/* Switch to new %cr3 - This might unmap the stack */
+	movq	%rdi, %cr3

Does there need to be a TLB flush after this? When executed from the main kernel's head code, CR4.PGE is enabled, and if the original page mapping had the global bit set (the decompressor stub sets that in the identity mapping), won't the read below still use the original encrypted mapping if we don't explicitly flush it?

Hi Arvind,

Quoted 9 lines · Arvind Sankar

arch/x86/kernel/sev_verify_cbit.S line 46

Quoted 4 lines · Joerg Roedel

arch/x86/kernel/sev_verify_cbit.S new file +4 -0

+
+	/* Store value to memory and keep it in %r10 */
+	movq	%r10, sev_check_data(%rip)
+

Does there need to be a cache flush/invalidation between this and the read below to avoid just reading back from cache, or will the hardware take care of that?

No, a cache flush is not needed. When the C bit position is correct, then the data will be mapped encrypted with the old and the new page-table. If the C bit position is wrong, the access goes to a different physical address.

Quoted 12 lines

arch/x86/kernel/sev_verify_cbit.S line 51

Quoted 6 lines

arch/x86/kernel/sev_verify_cbit.S new file +5 -0

+	/* Backup current %cr3 value to restore it later */
+	movq	%cr3, %r11
+
+	/* Switch to new %cr3 - This might unmap the stack */
+	movq	%rdi, %cr3

Does there need to be a TLB flush after this? When executed from the main kernel's head code, CR4.PGE is enabled, and if the original page mapping had the global bit set (the decompressor stub sets that in the identity mapping), won't the read below still use the original encrypted mapping if we don't explicitly flush it?

The check only really matters for the boot CPU, not for the secondary CPUs. IIRC at this point in boot CR4.PGE is still off.

Regards,

Joerg

The boot cpu also enables CR4.PGE -- that code is shared between boot and secondary cpus. The boot cpu jumps to the first "1" label below, just before the call to sev_verify_cbit you're adding.