lkml.org 
[lkml]   [2021]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 08/42] x86/sev-es: initialize sev_status/features within #VC handler
    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

    \
     
     \ /
      Last update: 2021-10-18 20:40    [W:2.554 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site