linux-kernel - Re: [PATCH v6 08/42] x86/sev-es: initialize sev

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YW2EsxcqBucuyoal@zn.tnic>
Date:   Mon, 18 Oct 2021 16:29:07 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        Tom Lendacky <thomas.lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Michael Roth <michael.roth@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        tony.luck@...el.com, marcorr@...gle.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com
Subject: Re: [PATCH v6 08/42] x86/sev-es: initialize sev_status/features
 within #VC handler

On Fri, Oct 08, 2021 at 01:04:19PM -0500, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@....com>
> 
> Generally access to MSR_AMD64_SEV is only safe if the 0x8000001F CPUID
> leaf indicates SEV support. With SEV-SNP, CPUID responses from the
> hypervisor are not considered trustworthy, particularly for 0x8000001F.
> SEV-SNP provides a firmware-validated CPUID table to use as an
> alternative, but prior to checking MSR_AMD64_SEV there are no
> guarantees that this is even an SEV-SNP guest.
> 
> Rather than relying on these CPUID values early on, allow SEV-ES and
> SEV-SNP guests to instead use a cpuid instruction to trigger a #VC and
> have it cache MSR_AMD64_SEV in sev_status, since it is known to be safe
> to access MSR_AMD64_SEV if a #VC has triggered.
> 
> Signed-off-by: Michael Roth <michael.roth@....com>
> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> ---
>  arch/x86/kernel/sev-shared.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
> index 8ee27d07c1cd..2796c524d174 100644
> --- a/arch/x86/kernel/sev-shared.c
> +++ b/arch/x86/kernel/sev-shared.c
> @@ -191,6 +191,20 @@ void __init do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>  	if (exit_code != SVM_EXIT_CPUID)
>  		goto fail;
>  
> +	/*
> +	 * A #VC implies that either SEV-ES or SEV-SNP are enabled, so the SEV
> +	 * MSR is also available. Go ahead and initialize sev_status here to
> +	 * allow SEV features to be checked without relying solely on the SEV
> +	 * cpuid bit to indicate whether it is safe to do so.
> +	 */
> +	if (!sev_status) {
> +		unsigned long lo, hi;
> +
> +		asm volatile("rdmsr" : "=a" (lo), "=d" (hi)
> +				     : "c" (MSR_AMD64_SEV));
> +		sev_status = (hi << 32) | lo;
> +	}
> +
>  	sev_es_wr_ghcb_msr(GHCB_CPUID_REQ(fn, GHCB_CPUID_REQ_EAX));
>  	VMGEXIT();
>  	val = sev_es_rd_ghcb_msr();
> -- 

Ok, you guys are killing me. ;-\

How is bolting some pretty much unrelated code into the early #VC
handler not a hack? Do you not see it?

So sme_enable() is reading MSR_AMD64_SEV and setting up everything
there, including sev_status. If a SNP guest does not trust CPUID, why
can't you attempt to read that MSR there, even if CPUID has lied to the
guest?

And not just slap it somewhere just because it works?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette