linux-kernel - Re: [PATCH v4 20/21] x86/efistub: Perform SNP feature test while running in the firmware

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]

Message-ID: <849a65c8-a320-a8a8-8784-0ee3737eee9e@amd.com>
Date:   Fri, 2 Jun 2023 15:39:51 -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>,
        Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH v4 20/21] x86/efistub: Perform SNP feature test while
 running in the firmware

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

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>       sme_me_mask = BIT_ULL(ebx & 0x3f);
>>   }