| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ca76ed3b-5835-9f1b-7e10-dd417249b7bd@amd.com>
Date: Thu, 18 May 2023 15:16:18 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Ard Biesheuvel <ardb@...nel.org>, linux-efi@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Evgeniy Baskov <baskov@...ras.ru>,
Borislav Petkov <bp@...en8.de>,
Andy Lutomirski <luto@...nel.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Alexey Khoroshilov <khoroshilov@...ras.ru>,
Peter Jones <pjones@...hat.com>,
Gerd Hoffmann <kraxel@...hat.com>,
Dave Young <dyoung@...hat.com>,
Mario Limonciello <mario.limonciello@....com>,
Kees Cook <keescook@...omium.org>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2 17/20] x86: efistub: Check SEV/SNP support while
running in the firmware
On 5/8/23 02:03, Ard Biesheuvel wrote:
> The decompressor executes in an environment with little or no access to
> a console, and without any ability to return an error back to the caller
> (the bootloader). So the only recourse we have when the SEV/SNP context
> is not quite what the kernel expects is to terminate the guest entirely.
>
> This is a bit harsh, and also unnecessary when booting via the EFI stub,
> given that it provides all the support that SEV guests need to probe the
> underlying platform.
>
> So let's do the SEV initialization and SNP feature check before calling
> ExitBootServices(), and simply return with an error if the SNP feature
> mask is not as expected.
My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
Turns out that sev_es_negotiate_protocol() used to be called when no #VC
exceptions were being generated before a valid GHCB was setup. Because
of that the current GHCB MSR value was not saved/restored. But now,
sev_es_negotiate_protocol() is called earlier in the boot process and
there are still messages being issued by UEFI, e.g.:
SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)
Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
and an SNP guest crashed after fixing sev_es_negotiate_protocol().
The following changes got me past everything:
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 3a5b0c9c4fcc..23450628d41c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
*/
static u64 get_hv_features(void)
{
- u64 val;
+ u64 val, save;
if (ghcb_version < 2)
return 0;
+ save = sev_es_rd_ghcb_msr();
+
sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
VMGEXIT();
-
val = sev_es_rd_ghcb_msr();
+
+ sev_es_wr_ghcb_msr(save);
+
if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
return 0;
@@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)
static bool sev_es_negotiate_protocol(void)
{
- u64 val;
+ u64 val, save;
+
+ save = sev_es_rd_ghcb_msr();
/* Do the GHCB protocol version negotiation */
sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
VMGEXIT();
val = sev_es_rd_ghcb_msr();
+ sev_es_wr_ghcb_msr(save);
+
if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
return false;
Thanks,
Tom
>
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> ---
> arch/x86/boot/compressed/sev.c | 12 ++++++++----
> arch/x86/include/asm/sev.h | 4 ++++
> drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
> 3 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 014b89c890887b9a..19c40873fdd209b5 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
> */
> #define SNP_FEATURES_PRESENT (0)
>
> +u64 snp_get_unsupported_features(void)
> +{
> + if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> + return 0;
> + return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +}
> +
> void snp_check_features(void)
> {
> u64 unsupported;
>
> - if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> - return;
> -
> /*
> * Terminate the boot if hypervisor has enabled any feature lacking
> * guest side implementation. Pass on the unsupported features mask through
> * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
> * as part of the guest boot failure.
> */
> - unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> + unsupported = snp_get_unsupported_features();
> if (unsupported) {
> if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
> __sev_es_nmi_complete();
> }
> extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +extern void sev_enable(struct boot_params *bp);
>
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
> {
> @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
> bool snp_init(struct boot_params *bp);
> void __init __noreturn snp_abort(void);
> int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> +u64 snp_get_unsupported_features(void);
> #else
> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> static inline void sev_es_ist_exit(void) { }
> static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> static inline void sev_es_nmi_complete(void) { }
> static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline void sev_enable(struct boot_params *bp) { }
> static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> static inline void setup_ghcb(void) { }
> @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
> {
> return -ENOTTY;
> }
> +static inline u64 snp_get_unsupported_features(void) { return 0; }
> #endif
>
> #endif
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index ce8434fce0c37982..33d11ba78f1d8c4f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -15,6 +15,7 @@
> #include <asm/setup.h>
> #include <asm/desc.h>
> #include <asm/boot.h>
> +#include <asm/sev.h>
>
> #include "efistub.h"
>
> @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
> p->efi->efi_memmap_size = map->map_size;
>
> + /*
> + * Call the SEV init code while still running with the firmware's
> + * GDT/IDT, so #VC exceptions will be handled by EFI.
> + */
> + if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> + u64 unsupported;
> +
> + sev_enable(p->boot_params);
> + unsupported = snp_get_unsupported_features();
> + if (unsupported) {
> + efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> + unsupported);
> + return EFI_UNSUPPORTED;
> + }
> + }
> +
> return EFI_SUCCESS;
> }
>
Powered by blists - more mailing lists