Date:   Tue, 18 Jan 2022 12:49:30 -0600
From:   Michael Roth <michael.roth@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Brijesh Singh <brijesh.singh@....com>, <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>,
        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 v8 29/40] x86/compressed/64: add support for SEV-SNP
 CPUID table in #VC handlers

On Tue, Jan 18, 2022 at 06:41:16PM +0100, Borislav Petkov wrote:
> On Tue, Jan 18, 2022 at 11:20:43AM -0600, Michael Roth wrote:
> > The HV fills out the initial contents of the CPUID page, which includes
> > the count. SNP/PSP firmware will validate the contents the HV tries to put
> > in the initial page, but does not currently enforce that the 'count' field
> > is non-zero.
> 
> So if the HV sets count to 0, then the PSP can validate all it wants but
> you basically don't have a CPUID page. And that's a pretty easy way to
> defeat it, if you ask me.
> 
> So, if it is too late to change this, I guess the only way out of here
> is to terminate the guest on count == 0.

Right, which is already enforced as part of snp_cpuid_info_create(). So
snp_cpuid_info->count will always be non-zero for SNP guests...

Er... so I suppose we *could* use snp_cpuid_info->count==0 as an indicator
that the cpuid page isn't enabled afterall...since if this was an SNP guest
then count==0 would've caused it to terminate...

Sorry I missed that, early versions of the code used count==0 as
indicator to bypass CPUID page before we realized that wasn't safe, and
I'd avoided relying on count==0 for anything since then. But in the
current code it should work, since count==0 causes SNP guests to
terminate, so a running guest with count==0 is clearly non-SNP.

> 
> And regardless, what if the HV fakes the count - how do you figure out
> what the proper count is? You go and read the whole CPUID page and try
> to make sense of what's there, even beyond the "last" function leaf.

The current code trusts the count value, as long as it is within the
bounds of the CPUID page. If the hypervisor provides a count that is
higher or lower than the number of entries added to the table, the PSP
will fail the guest launch.

count==0 is sort of a special case because of the reasons above, and
since it is never a valid CPUID configuration, so makes sense to
guard against.


> 
> > So we can't rely on the 'count' field as an indicator of whether or
> > not the CPUID page is active, we need to rely on the presence of the
> > ccblob as the true indicator, then treat a non-zero 'count' field as
> > an invalid state.
> 
> treat a non-zero count field as invalid?
> 
> You mean, "a zero count" maybe...

Yes, sorry for the confusion.

> 
> But see above, how do you check whether the HV hasn't "hidden" some
> entries by modifying the count field?
> 
> Either I'm missing something or this sounds really weird...

Yes, that's my fault. count must match the actual number of entries in
the table in all cases. If count==0 then there must also be no entries
in the table. count==0 is only special in that code might erroneously
decide to treat it as an indicator that cpuid table isn't enabled at
all, but since that case causes termination it should actually be ok.

Though I wonder if we should do something like this to still keep
callers from relying on checking count==0 directly:

  static const struct snp_cpuid_info *
  snp_cpuid_info_get_ptr(void)
  {
          const struct snp_cpuid_info *cpuid_info;
          void *ptr;
  
          /*
           * This may be called early while still running on the initial identity
           * mapping. Use RIP-relative addressing to obtain the correct address
           * in both for identity mapping and after switch-over to kernel virtual
           * addresses.
           */
          asm ("lea cpuid_info_copy(%%rip), %0"
               : "=r" (ptr)
               : "p" (&cpuid_info_copy));
  
          cpuid_info = ptr;
          if (cpuid_info->count == 0)
                  return NULL
  
          return cpuid_info;
  }

Because then it's impossible for a caller to accidentally misconstrue
what count==0 means (0 entries? or cpuid table not present?), since the
table then simply becomes inaccessible for anything other than an SNP
guest, and callers just need a NULL check (and will get a free hint
(crash) if they don't).

> 
> -- 
> 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%7C56a8943d73484fda82ce08d9daa9bc0c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637781244848502186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=2QO85fJBiZt2opWRWX%2FGb5LPt4How5cuAt4UJzAiQsg%3D&amp;reserved=0

Powered by blists - more mailing lists