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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 2 Aug 2023 08:40:36 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Borislav Petkov <bp@...en8.de>, Tao Liu <ltao@...hat.com>,
        Michael Roth <michael.roth@....com>
Cc:     tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
        x86@...nel.org, hpa@...or.com, ardb@...nel.org,
        linux-kernel@...r.kernel.org, bhe@...hat.com, dyoung@...hat.com,
        kexec@...ts.infradead.org, linux-efi@...r.kernel.org
Subject: Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for
 kexec kernel

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@....com>

> 
> ---
> From: "Borislav Petkov (AMD)" <bp@...en8.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@...hat.com>
> Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>
> Tested-by: Tao Liu <ltao@...hat.com>
> Cc: <stable@...nel.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