From: Joerg Roedel <[email protected]>
Hi,
here are some enhancements to the SEV(-ES) code in the Linux kernel to
self-protect it against some newly detected hypervisor attacks. There
are 3 attacks addressed here:
1) Hypervisor does not present the SEV-enabled bit via CPUID
2) The Hypervisor presents the wrong C-bit position via CPUID
3) An encrypted RAM page is mapped as MMIO in the nested
page-table, causing #VC exceptions and possible leak of the
data to the hypervisor or data/code injection from the
Hypervisor.
The attacks are described in more detail in this paper:
https://arxiv.org/abs/2010.07094
Please review.
Thanks,
Joerg
Joerg Roedel (5):
x86/boot/compressed/64: Introduce sev_status
x86/boot/compressed/64: Add CPUID sanity check to early #VC handler
x86/boot/compressed/64: Check SEV encryption in 64-bit boot-path
x86/head/64: Check SEV encryption before switching to kernel
page-table
x86/sev-es: Do not support MMIO to/from encrypted memory
arch/x86/boot/compressed/ident_map_64.c | 1 +
arch/x86/boot/compressed/mem_encrypt.S | 18 +++++-
arch/x86/boot/compressed/misc.h | 2 +
arch/x86/kernel/head_64.S | 14 ++++-
arch/x86/kernel/sev-es-shared.c | 26 +++++++++
arch/x86/kernel/sev-es.c | 20 ++++---
arch/x86/kernel/sev_verify_cbit.S | 77 +++++++++++++++++++++++++
arch/x86/mm/mem_encrypt.c | 1 +
8 files changed, 150 insertions(+), 9 deletions(-)
create mode 100644 arch/x86/kernel/sev_verify_cbit.S
--
2.28.0
From: Joerg Roedel <[email protected]>
MMIO memory is usually not mapped encrypted, so there is no reason to
support emulated MMIO when it is mapped encrypted.
This prevents a possible hypervisor attack where it maps a RAM page as
an MMIO page in the nested page-table, so that any guest access to it
will trigger a #VC exception and leak the data on that page to the
hypervisor or allows the hypervisor to inject data into the guest.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/kernel/sev-es.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 4a96726fbaf8..421fe0203c68 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -374,8 +374,8 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,
return ES_EXCEPTION;
}
-static bool vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
- unsigned long vaddr, phys_addr_t *paddr)
+static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
+ unsigned long vaddr, phys_addr_t *paddr)
{
unsigned long va = (unsigned long)vaddr;
unsigned int level;
@@ -394,15 +394,19 @@ static bool vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
if (user_mode(ctxt->regs))
ctxt->fi.error_code |= X86_PF_USER;
- return false;
+ return ES_EXCEPTION;
}
+ if (WARN_ON_ONCE(pte_val(*pte) & _PAGE_ENC))
+ /* Emulated MMIO to/from encrypted memory not supported */
+ return ES_UNSUPPORTED;
+
pa = (phys_addr_t)pte_pfn(*pte) << PAGE_SHIFT;
pa |= va & ~page_level_mask(level);
*paddr = pa;
- return true;
+ return ES_OK;
}
/* Include code shared with pre-decompression boot stage */
@@ -731,6 +735,7 @@ static enum es_result vc_do_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
{
u64 exit_code, exit_info_1, exit_info_2;
unsigned long ghcb_pa = __pa(ghcb);
+ enum es_result res;
phys_addr_t paddr;
void __user *ref;
@@ -740,11 +745,12 @@ static enum es_result vc_do_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
exit_code = read ? SVM_VMGEXIT_MMIO_READ : SVM_VMGEXIT_MMIO_WRITE;
- if (!vc_slow_virt_to_phys(ghcb, ctxt, (unsigned long)ref, &paddr)) {
- if (!read)
+ res = vc_slow_virt_to_phys(ghcb, ctxt, (unsigned long)ref, &paddr);
+ if (res != ES_OK) {
+ if (res == ES_EXCEPTION && !read)
ctxt->fi.error_code |= X86_PF_WRITE;
- return ES_EXCEPTION;
+ return res;
}
exit_info_1 = paddr;
--
2.28.0
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/boot/compressed/ident_map_64.c | 1 +
arch/x86/boot/compressed/mem_encrypt.S | 4 ++
arch/x86/boot/compressed/misc.h | 2 +
arch/x86/kernel/sev_verify_cbit.S | 77 +++++++++++++++++++++++++
4 files changed, 84 insertions(+)
create mode 100644 arch/x86/kernel/sev_verify_cbit.S
diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index 063a60edcf99..73abba3312a7 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -153,6 +153,7 @@ void initialize_identity_maps(void)
* into cr3.
*/
add_identity_map((unsigned long)_head, (unsigned long)_end);
+ sev_verify_cbit(top_level_pgt);
write_cr3(top_level_pgt);
}
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index 0effd58f0095..1786d5f02825 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -68,6 +68,9 @@ SYM_FUNC_START(get_sev_encryption_bit)
SYM_FUNC_END(get_sev_encryption_bit)
.code64
+
+#include "../../kernel/sev_verify_cbit.S"
+
SYM_FUNC_START(set_sev_encryption_mask)
#ifdef CONFIG_AMD_MEM_ENCRYPT
push %rbp
@@ -109,4 +112,5 @@ SYM_FUNC_END(set_sev_encryption_mask)
.balign 8
SYM_DATA(sme_me_mask, .quad 0)
SYM_DATA(sev_status, .quad 0)
+SYM_DATA(sev_check_data, .quad 0)
#endif
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 6d31f1b4c4d1..53f4848ad392 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -159,4 +159,6 @@ void boot_page_fault(void);
void boot_stage1_vc(void);
void boot_stage2_vc(void);
+void sev_verify_cbit(unsigned long cr3);
+
#endif /* BOOT_COMPRESSED_MISC_H */
diff --git a/arch/x86/kernel/sev_verify_cbit.S b/arch/x86/kernel/sev_verify_cbit.S
new file mode 100644
index 000000000000..5b6f61465437
--- /dev/null
+++ b/arch/x86/kernel/sev_verify_cbit.S
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * sev_verify_cbit.S - Code for verification of the C-bit position reported
+ * by the Hypervisor when running with SEV enabled.
+ *
+ * Copyright (c) 2020 Joerg Roedel ([email protected])
+ *
+ * Implements sev_verify_cbit() which is called before switching to a new
+ * long-mode page-table at boot.
+ *
+ * It verifies that the C-bit position is correct by writing a random value to
+ * an encrypted memory location while on the current page-table. Then it
+ * switches to the new page-table to verify the memory content is still the
+ * same. After that it switches back to the current page-table and when the
+ * check succeeded it returns. If the check failed the code invalidates the
+ * stack pointer and goes into a hlt loop. The stack-pointer is invalidated to
+ * make sure no interrupt or exception can get the CPU out of the hlt loop.
+ *
+ * New page-table pointer is expected in %rdi (first parameter)
+ *
+ */
+SYM_FUNC_START(sev_verify_cbit)
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+ /* First check if a C-bit was detected */
+ movq sme_me_mask(%rip), %r10
+ testq %r10, %r10
+ jz 3f
+
+ /* sme_me_mask != 0 could mean SME or SEV - Check also for SEV */
+ movq sev_status(%rip), %r10
+ testq %r10, %r10
+ jz 3f
+
+ /*
+ * Verified that running under SEV - now get a random value using
+ * RDRAND. This instruction is mandatory when running as an SEV guest.
+ *
+ * Don't bail out of the loop if RDRAND returns errors. It is better to
+ * prevent forward progress than to work with a non-random value here.
+ */
+1: rdrand %r10
+ jnc 1b
+
+ /* Store value to memory and keep it in %r10 */
+ movq %r10, sev_check_data(%rip)
+
+ /* Backup current %cr3 value to restore it later */
+ movq %cr3, %r11
+
+ /* Switch to new %cr3 - This might unmap the stack */
+ movq %rdi, %cr3
+
+ /*
+ * 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)
+
--
2.28.0
On Mon, Oct 19, 2020 at 05:22:47PM -0400, Arvind Sankar wrote:
> 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.
You are right, in the real kernel image PGE gets enabled early. I added
code to save and restore CR4 in sev_verify_cbit() and disable PGE during
the test.
Thanks,
Joerg
On Tue, Oct 20, 2020 at 10:33:12AM -0400, Arvind Sankar wrote:
> 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?
Yes, but relying on defined behavior is still better. Undefined behavior
could also mean it jumps to some other code which then leaks data.
Regards,
Joerg