linux-kernel - Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Apr 2023 00:20:39 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Christopherson,, Sean" <seanjc@...gle.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "zhi.wang.linux@...il.com" <zhi.wang.linux@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> On Wed, Apr 12, 2023, Kai Huang wrote:
> > On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> > > On Wed, 5 Apr 2023 19:10:40 -0700
> > > Sean Christopherson <seanjc@...gle.com> wrote:
> > > > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> > > 
> > > Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> > > It would be nice to have a conclusion of what is "sane" and what is the
> > > proper role for KVM, as firmware/TDX module is going to validate the "sane"
> > > CPUID.
> > > 
> > > TDX/SNP requires the CPUID to be pre-configured and validated before creating
> > > a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> > > SNP_LAUNCH_UPDATE in SNP).
> > > 
> > > IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> > > for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> > > one is literally possible and correct. KVM should not care if the
> > > combination, the usage of the chosen ones is insane or not, which gives
> > > QEMU flexibility.
> > > 
> > > As the valid CPUIDs have been decided when creating a CC guest, what should be
> > > the proper behavior (basically any new checks?) of KVM for the later
> > > SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> > > at least, because some KVM code paths rely on guest CPUID configuration.
> > 
> > For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
> > manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
> > the SET_CPUID2 for TDX guest.
> 
> It's been a long while since I looked at TDX's CPUID management, but IIRC ignoring
> SET_CPUID2 is not an option becuase the TDH.MNG.INIT only allows leafs that are
> known to the TDX Module, e.g. KVM's paravirt CPUID leafs can't be communicated via
> TDH.MNG.INIT.  
> 

Oh yes.  I forgot this.

> KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> could result in previously allowed input becoming invalid.

How about only filtering out PV related CPUIDs when applying CPUIDs to
TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
anyway.

> 
> Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
more concrete result.  

> 
> Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> that userspace provide a CPUID model that's consistent with what userspace provided
> earlier.

So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
all vcpus are consistent (the same) in SET_CPUID{2}?

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
when applying CPUIDs to TDH.MNG.INIT" approach works.