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

[Michael Roth <michael.roth@amd.com>]

On Mon, Oct 18, 2021 at 04:29:07PM +0200, Borislav Petkov wrote:
> On Fri, Oct 08, 2021 at 01:04:19PM -0500, Brijesh Singh wrote:
> > From: Michael Roth <michael.roth@amd.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@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.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?

This was the result of my proposal in v5:

  > More specifically, the general protocol to determine SNP is enabled
  > seems
  > to be:
  > 
  >  1) check cpuid 0x8000001f to determine if SEV bit is enabled and SEV
  >     MSR is available
  >  2) check the SEV MSR to see if SEV-SNP bit is set
  > 
  > but the conundrum here is the CPUID page is only valid if SNP is
  > enabled, otherwise it can be garbage. So the code to set up the page
  > skips those checks initially, and relies on the expectation that UEFI,
  > or whatever the initial guest blob was, will only provide a CC_BLOB if
  > it already determined SNP is enabled.
  > 
  > It's still possible something goes awry and the kernel gets handed a
  > bogus CC_BLOB even though SNP isn't actually enabled. In this case the
  > cpuid values could be bogus as well, but the guest will fail
  > attestation then and no secrets should be exposed.
  > 
  > There is one thing that could tighten up the check a bit though. Some
  > bits of SEV-ES code will use the generation of a #VC as an indicator
  > of SEV-ES support, which implies SEV MSR is available without relying
  > on hypervisor-provided CPUID bits. I could add a one-time check in
  > the cpuid #VC to check SEV MSR for SNP bit, but it would likely
  > involve another static __ro_after_init variable store state. If that
  > seems worthwhile I can look into that more as well.
  
  Yes, the skipping of checks above sounds weird: why don't you simply
  keep the checks order: SEV, -ES, -SNP and then parse CPUID. It'll fail
  at attestation eventually, but you'll have the usual flow like with the
  rest of the SEV- feature picking apart.

https://lore.kernel.org/lkml/YS3+saDefHwkYwny@zn.tnic/

I'd thought you didn't like the previous approach of having snp_cpuid_init()
defer the CPUID/MSR checks until sme_enable() sets up sev_status later on,
then failing the boot retroactively if SNP bit isn't set but CPUID table
was advertised. So I added those checks in snp_cpuid_init(), along with the
additional #VC-based indicator of SEV-ES/SEV-SNP support as an additional
sanity check of what EFI firmware was providing, since I thought that was
the key concern here.

Now I'm realizing that perhaps your suggestion was to actually defer the
entire CPUID page setup until after sme_enable(). Is that correct?

> 
> 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?

If CPUID has lied, that would result in a #GP, rather than a controlled
termination in the various checkers/callers. The latter is easier to
debug.

Additionally, #VC is arguably a better indicator of SEV MSR availability
for SEV-ES/SEV-SNP guests, since it is only generated by ES/SNP hardware
and doesn't rely directly on hypervisor/EFI-provided CPUID values. It
doesn't work for SEV guests, but I don't think it's a bad idea to allow
SEV-ES/SEV-SNP guests to initialize sev_status in #VC handler to make
use of the added assurance.

Is it just the way it's currently implemented as something
cpuid-table-specific that's at issue, or are you opposed to doing so in
general?

Thanks,

Mike

> 
> And not just slap it somewhere just because it works?
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7C462c7481ae414f7706a808d99243a615%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637701641625364120%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=6MViA5KCFEgSA2fijEx3Dg05btIEAjw55bFYRKL0P6o%3D&amp;reserved=0