Re: [PATCH v6 08/42] x86/sev-es: initialize sev_status/features within #VC handler

[Borislav Petkov]

On Fri, Oct 08, 2021 at 01:04:19PM -0500, Brijesh Singh wrote:
> From: Michael Roth <michael.roth@xxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> ---
>  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