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@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?
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@xxxxxxx/
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&data=04%7C01%7Cmichael.roth%40amd.com%7C462c7481ae414f7706a808d99243a615%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637701641625364120%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6MViA5KCFEgSA2fijEx3Dg05btIEAjw55bFYRKL0P6o%3D&reserved=0
