| Message ID | 20230602101313.3557775-21-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi/x86: Avoid bare metal decompressor during EFI boot | expand |
On 6/2/23 05:13, Ard Biesheuvel wrote: > Before refactoring the EFI stub boot flow to avoid the legacy bare metal > decompressor, duplicate the SNP feature check in the EFI stub before > handing over to the kernel proper. > > The SNP feature check can be performed while running under the EFI boot > services, which means we can fail gracefully and return an error to the > bootloader if the loaded kernel does not implement support for all the > features that the hypervisor enabled. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/boot/compressed/sev.c | 74 ++++++++++++-------- > arch/x86/include/asm/sev.h | 4 ++ > drivers/firmware/efi/libstub/x86-stub.c | 17 +++++ > 3 files changed, 67 insertions(+), 28 deletions(-) > > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c > index 014b89c890887b9a..be021e24f1ece421 100644 > --- a/arch/x86/boot/compressed/sev.c > +++ b/arch/x86/boot/compressed/sev.c > +void sev_enable(struct boot_params *bp) > +{ > + unsigned int eax, ebx, ecx, edx; > bool snp; > > /* > @@ -358,37 +391,14 @@ void sev_enable(struct boot_params *bp) > */ > snp = snp_init(bp); > > - /* Check for the SME/SEV support leaf */ > - eax = 0x80000000; > - ecx = 0; > - native_cpuid(&eax, &ebx, &ecx, &edx); > - if (eax < 0x8000001f) > - return; > - > - /* > - * Check for the SME/SEV feature: > - * CPUID Fn8000_001F[EAX] > - * - Bit 0 - Secure Memory Encryption support > - * - Bit 1 - Secure Encrypted Virtualization support > - * CPUID Fn8000_001F[EBX] > - * - Bits 5:0 - Pagetable bit position used to indicate encryption > - */ > - eax = 0x8000001f; > - ecx = 0; > - native_cpuid(&eax, &ebx, &ecx, &edx); > - /* Check whether SEV is supported */ > - if (!(eax & BIT(1))) { > + /* Set the SME mask if this is an SEV guest. */ > + sev_status = sev_get_status(); > + if (!(sev_status & MSR_AMD64_SEV_ENABLED)) { > if (snp) > error("SEV-SNP support indicated by CC blob, but not CPUID."); > return; > } > > - /* Set the SME mask if this is an SEV guest. */ > - boot_rdmsr(MSR_AMD64_SEV, &m); > - sev_status = m.q; > - if (!(sev_status & MSR_AMD64_SEV_ENABLED)) > - return; > - > /* Negotiate the GHCB protocol version. */ > if (sev_status & MSR_AMD64_SEV_ES_ENABLED) { > if (!sev_es_negotiate_protocol()) > @@ -409,6 +419,14 @@ void sev_enable(struct boot_params *bp) > if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED)) > error("SEV-SNP supported indicated by CC blob, but not SEV status MSR."); > > + /* > + * Check for the SME/SEV feature: > + * CPUID Fn8000_001F[EBX] > + * - Bits 5:0 - Pagetable bit position used to indicate encryption > + */ > + eax = 0x8000001f; > + ecx = 0; > + native_cpuid(&eax, &ebx, &ecx, &edx); This causes SEV-ES / SEV-SNP to crash. This goes back to a previous comment where calling either sev_es_negotiate_protocol() or get_hv_features() blows away the GHCB value in the GHCB MSR and as soon as the CPUID instruction is executed the boot blows up. Even if we move this up to be done earlier, we can complete this function successfully but then blow up further on. So you probably have to modify the routines in question to save and restore the GHCB MSR value. Thanks, Tom > sme_me_mask = BIT_ULL(ebx & 0x3f); > } >
On Fri, 2 Jun 2023 at 22:39, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 6/2/23 15:38, Tom Lendacky wrote: > > On 6/2/23 05:13, Ard Biesheuvel wrote: > >> Before refactoring the EFI stub boot flow to avoid the legacy bare metal > >> decompressor, duplicate the SNP feature check in the EFI stub before > >> handing over to the kernel proper. > >> > >> The SNP feature check can be performed while running under the EFI boot > >> services, which means we can fail gracefully and return an error to the > >> bootloader if the loaded kernel does not implement support for all the > >> features that the hypervisor enabled. > >> > >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >> --- > >> arch/x86/boot/compressed/sev.c | 74 ++++++++++++-------- > >> arch/x86/include/asm/sev.h | 4 ++ > >> drivers/firmware/efi/libstub/x86-stub.c | 17 +++++ > >> 3 files changed, 67 insertions(+), 28 deletions(-) > >> > >> diff --git a/arch/x86/boot/compressed/sev.c > >> b/arch/x86/boot/compressed/sev.c > >> index 014b89c890887b9a..be021e24f1ece421 100644 > >> --- a/arch/x86/boot/compressed/sev.c > >> +++ b/arch/x86/boot/compressed/sev.c > > > > > >> +void sev_enable(struct boot_params *bp) > >> +{ > >> + unsigned int eax, ebx, ecx, edx; > >> bool snp; > >> /* > >> @@ -358,37 +391,14 @@ void sev_enable(struct boot_params *bp) > >> */ > >> snp = snp_init(bp); > >> - /* Check for the SME/SEV support leaf */ > >> - eax = 0x80000000; > >> - ecx = 0; > >> - native_cpuid(&eax, &ebx, &ecx, &edx); > >> - if (eax < 0x8000001f) > >> - return; > >> - > >> - /* > >> - * Check for the SME/SEV feature: > >> - * CPUID Fn8000_001F[EAX] > >> - * - Bit 0 - Secure Memory Encryption support > >> - * - Bit 1 - Secure Encrypted Virtualization support > >> - * CPUID Fn8000_001F[EBX] > >> - * - Bits 5:0 - Pagetable bit position used to indicate encryption > >> - */ > >> - eax = 0x8000001f; > >> - ecx = 0; > >> - native_cpuid(&eax, &ebx, &ecx, &edx); > >> - /* Check whether SEV is supported */ > >> - if (!(eax & BIT(1))) { > >> + /* Set the SME mask if this is an SEV guest. */ > >> + sev_status = sev_get_status(); > >> + if (!(sev_status & MSR_AMD64_SEV_ENABLED)) { > >> if (snp) > >> error("SEV-SNP support indicated by CC blob, but not > >> CPUID."); > >> return; > >> } > >> - /* Set the SME mask if this is an SEV guest. */ > >> - boot_rdmsr(MSR_AMD64_SEV, &m); > >> - sev_status = m.q; > >> - if (!(sev_status & MSR_AMD64_SEV_ENABLED)) > >> - return; > >> - > >> /* Negotiate the GHCB protocol version. */ > >> if (sev_status & MSR_AMD64_SEV_ES_ENABLED) { > >> if (!sev_es_negotiate_protocol()) > >> @@ -409,6 +419,14 @@ void sev_enable(struct boot_params *bp) > >> if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED)) > >> error("SEV-SNP supported indicated by CC blob, but not SEV > >> status MSR."); > >> + /* > >> + * Check for the SME/SEV feature: > >> + * CPUID Fn8000_001F[EBX] > >> + * - Bits 5:0 - Pagetable bit position used to indicate encryption > >> + */ > >> + eax = 0x8000001f; > >> + ecx = 0; > >> + native_cpuid(&eax, &ebx, &ecx, &edx); > > > > This causes SEV-ES / SEV-SNP to crash. > > > > This goes back to a previous comment where calling either > > sev_es_negotiate_protocol() or get_hv_features() blows away the GHCB value > > in the GHCB MSR and as soon as the CPUID instruction is executed the boot > > blows up. > > > > Even if we move this up to be done earlier, we can complete this function > > successfully but then blow up further on. > > > > So you probably have to modify the routines in question to save and > > restore the GHCB MSR value. > > I should clarify that it doesn't in fact cause a problem until the final > patch is applied and this path is taken. > Could we just move the CPUID call to the start of the function?
On Sat, 3 Jun 2023 at 00:01, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 6/2/23 16:29, Ard Biesheuvel wrote: > > On Fri, 2 Jun 2023 at 22:39, Tom Lendacky <thomas.lendacky@amd.com> wrote: > >> > >> On 6/2/23 15:38, Tom Lendacky wrote: > >>> On 6/2/23 05:13, Ard Biesheuvel wrote: > >>>> Before refactoring the EFI stub boot flow to avoid the legacy bare metal > >>>> decompressor, duplicate the SNP feature check in the EFI stub before > >>>> handing over to the kernel proper. > >>>> > >>>> The SNP feature check can be performed while running under the EFI boot > >>>> services, which means we can fail gracefully and return an error to the > >>>> bootloader if the loaded kernel does not implement support for all the > >>>> features that the hypervisor enabled. > >>>> > >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >>>> --- > >>>> arch/x86/boot/compressed/sev.c | 74 ++++++++++++-------- > >>>> arch/x86/include/asm/sev.h | 4 ++ > >>>> drivers/firmware/efi/libstub/x86-stub.c | 17 +++++ > >>>> 3 files changed, 67 insertions(+), 28 deletions(-) > >>>> > >>>> diff --git a/arch/x86/boot/compressed/sev.c > >>>> b/arch/x86/boot/compressed/sev.c > >>>> index 014b89c890887b9a..be021e24f1ece421 100644 > >>>> --- a/arch/x86/boot/compressed/sev.c > >>>> +++ b/arch/x86/boot/compressed/sev.c > >>> > >>> > >>>> +void sev_enable(struct boot_params *bp) > >>>> +{ > >>>> + unsigned int eax, ebx, ecx, edx; > >>>> bool snp; > >>>> /* > >>>> @@ -358,37 +391,14 @@ void sev_enable(struct boot_params *bp) > >>>> */ > >>>> snp = snp_init(bp); > >>>> - /* Check for the SME/SEV support leaf */ > >>>> - eax = 0x80000000; > >>>> - ecx = 0; > >>>> - native_cpuid(&eax, &ebx, &ecx, &edx); > >>>> - if (eax < 0x8000001f) > >>>> - return; > >>>> - > >>>> - /* > >>>> - * Check for the SME/SEV feature: > >>>> - * CPUID Fn8000_001F[EAX] > >>>> - * - Bit 0 - Secure Memory Encryption support > >>>> - * - Bit 1 - Secure Encrypted Virtualization support > >>>> - * CPUID Fn8000_001F[EBX] > >>>> - * - Bits 5:0 - Pagetable bit position used to indicate encryption > >>>> - */ > >>>> - eax = 0x8000001f; > >>>> - ecx = 0; > >>>> - native_cpuid(&eax, &ebx, &ecx, &edx); > >>>> - /* Check whether SEV is supported */ > >>>> - if (!(eax & BIT(1))) { > >>>> + /* Set the SME mask if this is an SEV guest. */ > >>>> + sev_status = sev_get_status(); > >>>> + if (!(sev_status & MSR_AMD64_SEV_ENABLED)) { > >>>> if (snp) > >>>> error("SEV-SNP support indicated by CC blob, but not > >>>> CPUID."); > >>>> return; > >>>> } > >>>> - /* Set the SME mask if this is an SEV guest. */ > >>>> - boot_rdmsr(MSR_AMD64_SEV, &m); > >>>> - sev_status = m.q; > >>>> - if (!(sev_status & MSR_AMD64_SEV_ENABLED)) > >>>> - return; > >>>> - > >>>> /* Negotiate the GHCB protocol version. */ > >>>> if (sev_status & MSR_AMD64_SEV_ES_ENABLED) { > >>>> if (!sev_es_negotiate_protocol()) > >>>> @@ -409,6 +419,14 @@ void sev_enable(struct boot_params *bp) > >>>> if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED)) > >>>> error("SEV-SNP supported indicated by CC blob, but not SEV > >>>> status MSR."); > >>>> + /* > >>>> + * Check for the SME/SEV feature: > >>>> + * CPUID Fn8000_001F[EBX] > >>>> + * - Bits 5:0 - Pagetable bit position used to indicate encryption > >>>> + */ > >>>> + eax = 0x8000001f; > >>>> + ecx = 0; > >>>> + native_cpuid(&eax, &ebx, &ecx, &edx); > >>> > >>> This causes SEV-ES / SEV-SNP to crash. > >>> > >>> This goes back to a previous comment where calling either > >>> sev_es_negotiate_protocol() or get_hv_features() blows away the GHCB value > >>> in the GHCB MSR and as soon as the CPUID instruction is executed the boot > >>> blows up. > >>> > >>> Even if we move this up to be done earlier, we can complete this function > >>> successfully but then blow up further on. > >>> > >>> So you probably have to modify the routines in question to save and > >>> restore the GHCB MSR value. > >> > >> I should clarify that it doesn't in fact cause a problem until the final > >> patch is applied and this path is taken. > >> > > > > Could we just move the CPUID call to the start of the function? > > I tried that and it allowed sev_enable() to complete successfully, but > then it blew up after that. > > But I noticed that the patch to apply the kernel CS earlier is no longer > part of this series. When I applied the patch to move the setting of the > kernel CS directly after the call to startup_64_setup_env() in > arch/x86/kernel/head_64.S, everything worked again. > > That patch hasn't been pulled into tip, yet, and since you dropped your > version, the CS value wrong and the IRET from the #VC blows up. > > So as long as you pre-req that patch, unless Boris or Dave would prefer it > to go in with this series, moving the call to native_cpuid() up to just > before the GHCB protocol version negotiation, works. > OK, thanks for confirming. I dropped that patch because I was assuming that your fix would be picked up.
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index 014b89c890887b9a..be021e24f1ece421 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -315,6 +315,11 @@ static void enforce_vmpl0(void) */ #define SNP_FEATURES_PRESENT (0) +u64 snp_get_unsupported_features(u64 status) +{ + return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT; +} + void snp_check_features(void) { u64 unsupported; @@ -328,7 +333,7 @@ void snp_check_features(void) * 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(sev_status); if (unsupported) { if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb())) sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED); @@ -338,10 +343,38 @@ void snp_check_features(void) } } -void sev_enable(struct boot_params *bp) +u64 sev_get_status(void) { unsigned int eax, ebx, ecx, edx; struct msr m; + + /* Check for the SME/SEV support leaf */ + eax = 0x80000000; + ecx = 0; + native_cpuid(&eax, &ebx, &ecx, &edx); + if (eax < 0x8000001f) + return 0; + + /* + * Check for the SME/SEV feature: + * CPUID Fn8000_001F[EAX] + * - Bit 0 - Secure Memory Encryption support + * - Bit 1 - Secure Encrypted Virtualization support + */ + eax = 0x8000001f; + ecx = 0; + native_cpuid(&eax, &ebx, &ecx, &edx); + /* Check whether SEV is supported */ + if (!(eax & BIT(1))) + return 0; + + boot_rdmsr(MSR_AMD64_SEV, &m); + return m.q; +} + +void sev_enable(struct boot_params *bp) +{ + unsigned int eax, ebx, ecx, edx; bool snp; /* @@ -358,37 +391,14 @@ void sev_enable(struct boot_params *bp) */ snp = snp_init(bp); - /* Check for the SME/SEV support leaf */ - eax = 0x80000000; - ecx = 0; - native_cpuid(&eax, &ebx, &ecx, &edx); - if (eax < 0x8000001f) - return; - - /* - * Check for the SME/SEV feature: - * CPUID Fn8000_001F[EAX] - * - Bit 0 - Secure Memory Encryption support - * - Bit 1 - Secure Encrypted Virtualization support - * CPUID Fn8000_001F[EBX] - * - Bits 5:0 - Pagetable bit position used to indicate encryption - */ - eax = 0x8000001f; - ecx = 0; - native_cpuid(&eax, &ebx, &ecx, &edx); - /* Check whether SEV is supported */ - if (!(eax & BIT(1))) { + /* Set the SME mask if this is an SEV guest. */ + sev_status = sev_get_status(); + if (!(sev_status & MSR_AMD64_SEV_ENABLED)) { if (snp) error("SEV-SNP support indicated by CC blob, but not CPUID."); return; } - /* Set the SME mask if this is an SEV guest. */ - boot_rdmsr(MSR_AMD64_SEV, &m); - sev_status = m.q; - if (!(sev_status & MSR_AMD64_SEV_ENABLED)) - return; - /* Negotiate the GHCB protocol version. */ if (sev_status & MSR_AMD64_SEV_ES_ENABLED) { if (!sev_es_negotiate_protocol()) @@ -409,6 +419,14 @@ void sev_enable(struct boot_params *bp) if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED)) error("SEV-SNP supported indicated by CC blob, but not SEV status MSR."); + /* + * Check for the SME/SEV feature: + * CPUID Fn8000_001F[EBX] + * - Bits 5:0 - Pagetable bit position used to indicate encryption + */ + eax = 0x8000001f; + ecx = 0; + native_cpuid(&eax, &ebx, &ecx, &edx); sme_me_mask = BIT_ULL(ebx & 0x3f); } diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 13dc2a9d23c1eb25..e5aad673194698b8 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -202,6 +202,8 @@ 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(u64 status); +u64 sev_get_status(void); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } static inline void sev_es_ist_exit(void) { } @@ -225,6 +227,8 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in { return -ENOTTY; } +static inline u64 snp_get_unsupported_features(u64 status) { return 0; } +static inline u64 sev_get_status(void) { return 0; } #endif #endif diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index 2d3282d2ed6eb756..f9d203b5ee6236e8 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" #include "x86-stub.h" @@ -756,6 +757,19 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle) return EFI_SUCCESS; } +static bool have_unsupported_snp_features(void) +{ + u64 unsupported; + + unsupported = snp_get_unsupported_features(sev_get_status()); + if (unsupported) { + efi_err("Unsupported SEV-SNP features detected: 0x%llx\n", + unsupported); + return true; + } + return false; +} + static void __noreturn enter_kernel(unsigned long kernel_addr, struct boot_params *boot_params) { @@ -785,6 +799,9 @@ void __noreturn efi_stub_entry(efi_handle_t handle, if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) efi_exit(handle, EFI_INVALID_PARAMETER); + if (have_unsupported_snp_features()) + efi_exit(handle, EFI_UNSUPPORTED); + if (IS_ENABLED(CONFIG_EFI_DXE_MEM_ATTRIBUTES)) { efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID); if (efi_dxe_table &&
Before refactoring the EFI stub boot flow to avoid the legacy bare metal decompressor, duplicate the SNP feature check in the EFI stub before handing over to the kernel proper. The SNP feature check can be performed while running under the EFI boot services, which means we can fail gracefully and return an error to the bootloader if the loaded kernel does not implement support for all the features that the hypervisor enabled. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/x86/boot/compressed/sev.c | 74 ++++++++++++-------- arch/x86/include/asm/sev.h | 4 ++ drivers/firmware/efi/libstub/x86-stub.c | 17 +++++ 3 files changed, 67 insertions(+), 28 deletions(-)