Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for kexec kernel

Tom Lendacky
2023-08-02T13:40:46Z

On 8/2/23 04:39, Borislav Petkov wrote:
> On Wed, Aug 02, 2023 at 04:22:54PM +0800, Tao Liu wrote:
>> Thanks for the patch! I have tested it on the lenovo machine in the
>> past few days, no issue found, so the patch tests OK.
>
> Thanks for testing!
>
> Mike, Tom, the below ok this way?

Short of figuring out how to map page accesses earlier through the
boot_page_fault IDT routine, this seems reasonable.

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

>
> ---
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> Date: Sun, 16 Jul 2023 20:22:20 +0200
> Subject: [PATCH] x86/sev: Do not try to parse for the CC blob on non-AMD
> hardware
>
> Tao Liu reported a boot hang on an Intel Atom machine due to an unmapped
> EFI config table. The reason being that the CC blob which contains the
> CPUID page for AMD SNP guests is parsed for before even checking
> whether the machine runs on AMD hardware.
>
> Usually that's not a problem on !AMD hw - it simply won't find the CC
> blob's GUID and return. However, if any parts of the config table
> pointers array is not mapped, the kernel will #PF very early in the
> decompressor stage without any opportunity to recover.
>
> Therefore, do a superficial CPUID check before poking for the CC blob.
> This will fix the current issue on real hardware. It would also work as
> a guest on a non-lying hypervisor.
>
> For the lying hypervisor, the check is done again, *after* parsing the
> CC blob as the real CPUID page will be present then.
>
> Clear the #VC handler in case SEV-{ES,SNP} hasn't been detected, as
> a precaution.
>
> Fixes: c01fce9cef84 ("x86/compressed: Add SEV-SNP feature detection/setup")
> Reported-by: Tao Liu <ltao@redhat.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Tested-by: Tao Liu <ltao@redhat.com>
> Cc: <stable@kernel.org>
> Link: https://lore.kernel.org/r/20230601072043.24439-1-ltao@redhat.com
> ---
> arch/x86/boot/compressed/idt_64.c | 9 +++++++-
> arch/x86/boot/compressed/sev.c | 37 +++++++++++++++++++++++++++++--
> 2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/idt_64.c b/arch/x86/boot/compressed/idt_64.c
> index 6debb816e83d..3cdf94b41456 100644
> --- a/arch/x86/boot/compressed/idt_64.c
> +++ b/arch/x86/boot/compressed/idt_64.c
> @@ -63,7 +63,14 @@ void load_stage2_idt(void)
> set_idt_entry(X86_TRAP_PF, boot_page_fault);
>
> #ifdef CONFIG_AMD_MEM_ENCRYPT
> - set_idt_entry(X86_TRAP_VC, boot_stage2_vc);
> + /*
> + * Clear the second stage #VC handler in case guest types
> + * needing #VC have not been detected.
> + */
> + if (sev_status & BIT(1))
> + set_idt_entry(X86_TRAP_VC, boot_stage2_vc);
> + else
> + set_idt_entry(X86_TRAP_VC, NULL);
> #endif
>
> load_boot_idt(&boot_idt_desc);
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 09dc8c187b3c..c3e343bd4760 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -404,13 +404,46 @@ void sev_enable(struct boot_params *bp)
> if (bp)
> bp->cc_blob_address = 0;
>
> + /*
> + * Do an initial SEV capability check before snp_init() which
> + * loads the CPUID page and the same checks afterwards are done
> + * without the hypervisor and are trustworthy.
> + *
> + * If the HV fakes SEV support, the guest will crash'n'burn
> + * which is good enough.
> + */
> +
> + /* 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)))
> + return;
> +
> /*
> * Setup/preliminary detection of SNP. This will be sanity-checked
> * against CPUID/MSR values later.
> */
> snp = snp_init(bp);
>
> - /* Check for the SME/SEV support leaf */
> + /* Now repeat the checks with the SNP CPUID table. */
> +
> + /* Recheck the SME/SEV support leaf */
> eax = 0x80000000;
> ecx = 0;
> native_cpuid(&eax, &ebx, &ecx, &edx);
> @@ -418,7 +451,7 @@ void sev_enable(struct boot_params *bp)
> return;
>
> /*
> - * Check for the SME/SEV feature:
> + * Recheck for the SME/SEV feature:
> * CPUID Fn8000_001F[EAX]
> * - Bit 0 - Secure Memory Encryption support
> * - Bit 1 - Secure Encrypted Virtualization support