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
Changes to v2:
- Use %r8/%r9 to modify %cr4 in sev_verify_cbit()
and return the new page-table pointer in that function.
Changes to v1:
- Disable CR4.PGE during C-bit test
- Do not safe/restore caller-safed registers in
set_sev_encryption_mask()
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 | 14 +++-
arch/x86/boot/compressed/misc.h | 2 +
arch/x86/kernel/head_64.S | 12 ++++
arch/x86/kernel/sev-es-shared.c | 26 +++++++
arch/x86/kernel/sev-es.c | 20 ++++--
arch/x86/kernel/sev_verify_cbit.S | 90 +++++++++++++++++++++++++
arch/x86/mm/mem_encrypt.c | 1 +
8 files changed, 158 insertions(+), 8 deletions(-)
create mode 100644 arch/x86/kernel/sev_verify_cbit.S
--
2.28.0
From: Joerg Roedel <[email protected]>
When SEV is enabled the kernel requests the C-Bit position again from
the hypervisor to built its own page-table. Since the hypervisor is an
untrusted source the C-bit position needs to be verified before the
kernel page-table is used.
Call the sev_verify_cbit() function before writing the CR3.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/kernel/head_64.S | 12 ++++++++++++
arch/x86/mm/mem_encrypt.c | 1 +
2 files changed, 13 insertions(+)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 7eb2a1c87969..cf295ff1ae00 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -161,6 +161,17 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
/* Setup early boot stage 4-/5-level pagetables. */
addq phys_base(%rip), %rax
+
+ /*
+ * For SEV guests: Verify that the C-bit is correct. A malicious
+ * hypervisor could lie about the C-bit position to perform a ROP
+ * attack on the guest by writing to the unencrypted stack and wait for
+ * the next RET instruction.
+ */
+ movq %rax, %rdi
+ call sev_verify_cbit
+
+ /* Switch to new page-table */
movq %rax, %cr3
/* Ensure I am executing from virtual addresses */
@@ -279,6 +290,7 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
SYM_CODE_END(secondary_startup_64)
#include "verify_cpu.S"
+#include "sev_verify_cbit.S"
#ifdef CONFIG_HOTPLUG_CPU
/*
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index ebb7edc8bc0a..bd9b62af2e3d 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -39,6 +39,7 @@
*/
u64 sme_me_mask __section(.data) = 0;
u64 sev_status __section(.data) = 0;
+u64 sev_check_data __section(.data) = 0;
EXPORT_SYMBOL(sme_me_mask);
DEFINE_STATIC_KEY_FALSE(sev_enable_key);
EXPORT_SYMBOL_GPL(sev_enable_key);
--
2.28.0
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 | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
index dd07e7b41b11..2192b3bd78d8 100644
--- a/arch/x86/boot/compressed/mem_encrypt.S
+++ b/arch/x86/boot/compressed/mem_encrypt.S
@@ -81,6 +81,13 @@ 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 */
@@ -96,5 +103,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
--
2.28.0
From: Joerg Roedel <[email protected]>
The early #VC handler which doesn't have a GHCB 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 early #VC handlers to make sure the hypervisor
can not pretend that SEV is disabled.
Signed-off-by: Joerg Roedel <[email protected]>
---
arch/x86/kernel/sev-es-shared.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c
index 5f83ccaab877..48bb14563dcd 100644
--- a/arch/x86/kernel/sev-es-shared.c
+++ b/arch/x86/kernel/sev-es-shared.c
@@ -178,6 +178,32 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
goto fail;
regs->dx = val >> 32;
+ /*
+ * This is a VC handler and it is only raised when SEV-ES is active,
+ * which means SEV must be active too. Do sanity checks on the CPUID
+ * results to make sure the hypervisor does not trick the kernel into
+ * the no-sev path. This could map sensitive data unencrypted and make
+ * it accessible to the hypervisor.
+ *
+ * In particular, check for:
+ * - Hypervisor CPUID bit
+ * - Availability of CPUID leaf 0x8000001f
+ * - SEV CPUID bit.
+ *
+ * The hypervisor might still report the wrong C-bit position, but this
+ * can't be checked here.
+ */
+
+ if ((fn == 1 && !(regs->cx & BIT(31))))
+ /* Hypervisor Bit */
+ goto fail;
+ else if (fn == 0x80000000 && (regs->ax < 0x8000001f))
+ /* SEV Leaf check */
+ goto fail;
+ else if ((fn == 0x8000001f && !(regs->ax & BIT(1))))
+ /* SEV Bit */
+ goto fail;
+
/* Skip over the CPUID two-byte opcode */
regs->ip += 2;
--
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..0bd1a0fc587e 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 | 90 +++++++++++++++++++++++++
4 files changed, 97 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 2192b3bd78d8..7409f2343d38 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
@@ -105,4 +108,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..d9a631c5973c 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);
+unsigned long 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..5075458ecad0
--- /dev/null
+++ b/arch/x86/kernel/sev_verify_cbit.S
@@ -0,0 +1,90 @@
+/* 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
+
+ /* Save CR4 in %r8 */
+ movq %cr4, %r8
+
+ /* Disable Global Pages */
+ movq %r8, %r9
+ andq $(~X86_CR4_PGE), %r9
+ movq %r9, %cr4
+
+ /*
+ * 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
+
+ /* Restore previous CR4 */
+ movq %r8, %cr4
+
+ /* 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
+ /* Return page-table pointer */
+ movq %rdi, %rax
+ ret
+SYM_FUNC_END(sev_verify_cbit)
+
--
2.28.0
On Wed, Oct 21, 2020 at 02:39:34PM +0200, Joerg Roedel wrote:
> 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 | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> index dd07e7b41b11..2192b3bd78d8 100644
> --- a/arch/x86/boot/compressed/mem_encrypt.S
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -81,6 +81,13 @@ 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)
A couple of lines above you call get_sev_encryption_bit() which already
reads MSR_AMD64_SEV. Why not set sev_status there too instead of reading
that MSR again here?
It can read that MSR once and use sev_status(%rip) from then on to avoid
reading that MSR multiple times...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 21, 2020 at 02:39:35PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> The early #VC handler which doesn't have a GHCB 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 early #VC handlers to make sure the hypervisor
> can not pretend that SEV is disabled.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> arch/x86/kernel/sev-es-shared.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c
> index 5f83ccaab877..48bb14563dcd 100644
> --- a/arch/x86/kernel/sev-es-shared.c
> +++ b/arch/x86/kernel/sev-es-shared.c
> @@ -178,6 +178,32 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> goto fail;
> regs->dx = val >> 32;
>
> + /*
> + * This is a VC handler and it is only raised when SEV-ES is active,
and the #VC is only raised...
> + * which means SEV must be active too. Do sanity checks on the CPUID
> + * results to make sure the hypervisor does not trick the kernel into
> + * the no-sev path. This could map sensitive data unencrypted and make
> + * it accessible to the hypervisor.
> + *
> + * In particular, check for:
> + * - Hypervisor CPUID bit
> + * - Availability of CPUID leaf 0x8000001f
> + * - SEV CPUID bit.
> + *
> + * The hypervisor might still report the wrong C-bit position, but this
> + * can't be checked here.
So why are we doing those checks here at all then? I mean, the HV
can tell us whatever it wants, i.e., make sure those checks pass but
still report the C-bit at the wrong position. Which means that those
checks are simply meh. So why are we doing them at all? To catch stupid
hypervisors who can't even lie properly to the guest? :-)
> + */
> +
> + if ((fn == 1 && !(regs->cx & BIT(31))))
> + /* Hypervisor Bit */
s/Bit/bit/g
> + goto fail;
> + else if (fn == 0x80000000 && (regs->ax < 0x8000001f))
> + /* SEV Leaf check */
> + goto fail;
> + else if ((fn == 0x8000001f && !(regs->ax & BIT(1))))
> + /* SEV Bit */
> + goto fail;
> +
> /* Skip over the CPUID two-byte opcode */
> regs->ip += 2;
>
> --
> 2.28.0
>
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 21, 2020 at 02:39:36PM +0200, Joerg Roedel wrote:
> diff --git a/arch/x86/kernel/sev_verify_cbit.S b/arch/x86/kernel/sev_verify_cbit.S
> new file mode 100644
> index 000000000000..5075458ecad0
> --- /dev/null
> +++ b/arch/x86/kernel/sev_verify_cbit.S
Why a separate file? You're using it just like verify_cpu.S and this is
kinda verifying CPU so you could simply add the functionality there...
> @@ -0,0 +1,90 @@
> +/* 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
Yeah, can you please use the callee-clobbered registers in the order as
they're used by the ABI, see arch/x86/entry/calling.h.
Because I'm looking at this and wondering are rsi, rdx and rcx somehow
live here and you're avoiding them...
Otherwise nice commenting - I like when it is properly explained what
the asm does and what it expects as input, cool.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 21, 2020 at 02:39:36PM +0200, Joerg Roedel wrote:
> 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 | 90 +++++++++++++++++++++++++
> 4 files changed, 97 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);
> }
Btw, might wanna redo them ontop of -rc1 because this looks like this after
Arvind's three fixes:
diff --git a/arch/x86/boot/compressed/ident_map_64.c b/arch/x86/boot/compressed/ident_map_64.c
index a5e5db6ada3c..81f6003553f8 100644
--- a/arch/x86/boot/compressed/ident_map_64.c
+++ b/arch/x86/boot/compressed/ident_map_64.c
@@ -162,6 +162,7 @@ void initialize_identity_maps(void *rmode)
add_identity_map((unsigned long)boot_params, (unsigned long)(boot_params + 1));
cmdline = get_cmd_line_ptr();
add_identity_map(cmdline, cmdline + COMMAND_LINE_SIZE);
+ sev_verify_cbit(top_level_pgt);
/* Load the new page-table. */
write_cr3(top_level_pgt);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 21, 2020 at 02:39:37PM +0200, Joerg Roedel wrote:
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index ebb7edc8bc0a..bd9b62af2e3d 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -39,6 +39,7 @@
> */
> u64 sme_me_mask __section(.data) = 0;
> u64 sev_status __section(.data) = 0;
> +u64 sev_check_data __section(.data) = 0;
Apparently section names are supposed to be given as strings now:
In file included from ././include/linux/compiler_types.h:65,
from <command-line>:
arch/x86/mm/mem_encrypt.c:42:30: error: expected expression before ‘.’ token
42 | u64 sev_check_data __section(.data) = 0;
| ^
./include/linux/compiler_attributes.h:257:68: note: in definition of macro ‘__section’
257 | #define __section(section) __attribute__((__section__(section)))
| ^~~~~~~
arch/x86/mm/mem_encrypt.c:42:30: error: section attribute argument not a string constant
42 | u64 sev_check_data __section(.data) = 0;
| ^
./include/linux/compiler_attributes.h:257:68: note: in definition of macro ‘__section’
257 | #define __section(section) __attribute__((__section__(section)))
| ^~~~~~~
make[2]: *** [scripts/Makefile.build:283: arch/x86/mm/mem_encrypt.o] Error 1
make[1]: *** [scripts/Makefile.build:500: arch/x86/mm] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1799: arch/x86] Error 2
make: *** Waiting for unfinished jobs....
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 21, 2020 at 02:39:38PM +0200, Joerg Roedel wrote:
> 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
"Prevent... "
> 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
^
"... via the GHCB (like with normal MMIO)... "
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Oct 27, 2020 at 11:38:46AM +0100, Borislav Petkov wrote:
> So why are we doing those checks here at all then? I mean, the HV
> can tell us whatever it wants, i.e., make sure those checks pass but
> still report the C-bit at the wrong position. Which means that those
> checks are simply meh. So why are we doing them at all? To catch stupid
> hypervisors who can't even lie properly to the guest? :-)
To avoid that the HV tricks the kernel into the no_sev boot path, where
it would map memory unencrypted and possibly leak sensitive data. The HV
can do so by pretending SEV is disabled at all and by reporting the
wrond C-bit position. Both cases need to be checked.
Regards,
Joerg
On Mon, Oct 26, 2020 at 07:27:06PM +0100, Borislav Petkov wrote:
> A couple of lines above you call get_sev_encryption_bit() which already
> reads MSR_AMD64_SEV. Why not set sev_status there too instead of reading
> that MSR again here?
>
> It can read that MSR once and use sev_status(%rip) from then on to avoid
> reading that MSR multiple times...
Right, makes sense. I updated the patch.
On Wed, Oct 28, 2020 at 09:23:52AM +0100, Joerg Roedel wrote:
> On Mon, Oct 26, 2020 at 07:27:06PM +0100, Borislav Petkov wrote:
> > A couple of lines above you call get_sev_encryption_bit() which already
> > reads MSR_AMD64_SEV. Why not set sev_status there too instead of reading
> > that MSR again here?
> >
> > It can read that MSR once and use sev_status(%rip) from then on to avoid
> > reading that MSR multiple times...
>
> Right, makes sense. I updated the patch.
Hang on, get_sev_encryption_bit() is also called from startup_32(),
so it can't contain any 64-bit instructions to set sev_status.
On Tue, Oct 27, 2020 at 12:08:12PM +0100, Borislav Petkov wrote:
> On Wed, Oct 21, 2020 at 02:39:36PM +0200, Joerg Roedel wrote:
> > diff --git a/arch/x86/kernel/sev_verify_cbit.S b/arch/x86/kernel/sev_verify_cbit.S
> > new file mode 100644
> > index 000000000000..5075458ecad0
> > --- /dev/null
> > +++ b/arch/x86/kernel/sev_verify_cbit.S
>
> Why a separate file? You're using it just like verify_cpu.S and this is
> kinda verifying CPU so you could simply add the functionality there...
verify_cpu.S is also used on 32bit and this function is 64bit code. It
can be made working with some #ifdef'fery but I think it is cleaner to
just keep it in a separate file, also given that sev_verify_cbit() is
not needed at every place verify_cpu() is called.
> Yeah, can you please use the callee-clobbered registers in the order as
> they're used by the ABI, see arch/x86/entry/calling.h.
>
> Because I'm looking at this and wondering are rsi, rdx and rcx somehow
> live here and you're avoiding them...
Makes sense, will update the function.
Regards,
Joerg
On Wed, Oct 28, 2020 at 12:50:07PM -0400, Arvind Sankar wrote:
> On Wed, Oct 28, 2020 at 09:23:52AM +0100, Joerg Roedel wrote:
> > On Mon, Oct 26, 2020 at 07:27:06PM +0100, Borislav Petkov wrote:
> > > A couple of lines above you call get_sev_encryption_bit() which already
> > > reads MSR_AMD64_SEV. Why not set sev_status there too instead of reading
> > > that MSR again here?
> > >
> > > It can read that MSR once and use sev_status(%rip) from then on to avoid
> > > reading that MSR multiple times...
> >
> > Right, makes sense. I updated the patch.
>
> Hang on, get_sev_encryption_bit() is also called from startup_32(),
> so it can't contain any 64-bit instructions to set sev_status.
Yeah, figured that out too and discussed it with Boris. Decided to leave
it as-is and add a comment why the MSR is re-read.
Thanks,
Joerg