linux-kernel - Re: [PATCH 7/7] KVM: TDX: Add TSX_CTRL msr into uret

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]

Message-ID: <Z05SK2OxASuznmPq@google.com>
Date: Mon, 2 Dec 2024 16:34:51 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Rick P Edgecombe <rick.p.edgecombe@...el.com>
Cc: Adrian Hunter <adrian.hunter@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	Xiaoyao Li <xiaoyao.li@...el.com>, Kai Huang <kai.huang@...el.com>, 
	Yan Y Zhao <yan.y.zhao@...el.com>, 
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "dmatlack@...gle.com" <dmatlack@...gle.com>, 
	Weijiang Yang <weijiang.yang@...el.com>, Reinette Chatre <reinette.chatre@...el.com>, 
	"pbonzini@...hat.com" <pbonzini@...hat.com>, 
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>, Isaku Yamahata <isaku.yamahata@...el.com>, 
	"nik.borisov@...e.com" <nik.borisov@...e.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Chao Gao <chao.gao@...el.com>, 
	"tony.lindgren@...ux.intel.com" <tony.lindgren@...ux.intel.com>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH 7/7] KVM: TDX: Add TSX_CTRL msr into uret_msrs list

On Mon, Dec 02, 2024, Rick P Edgecombe wrote:
> On Mon, 2024-12-02 at 11:07 -0800, Sean Christopherson wrote:
> > > guest_can_use() is per-vcpu whereas we are currently using the
> > > CPUID from TD_PARAMS (as per spec) before there are any VCPU's.
> > > It is a bit of a disconnect so let's keep tsx_supported for now.
> > 
> > No, as was agreed upon[*], KVM needs to ensure consistency between what KVM
> > sees

Rick, fix your MUA to not wrap already :-)

> > as guest CPUID and what is actually enabled/exposed to the guest.  If there
> > are no vCPUs, then there's zero reason to snapshot the value in kvm_tdx. 
> > And if there are vCPUs, then their CPUID info needs to be consistent with
> > respect to TDPARAMS.
> 
> Small point - the last conversation[0] we had on this was to let *userspace*
> ensure consistency between KVM's CPUID (i.e. KVM_SET_CPUID2) and the TDX
> Module's view.

I'm all for that, right up until KVM needs to protect itself again userspace and
flawed TDX architecture.  A relevant comment I made in that thread:

 : If the upgrade breaks a setup because it confuses _KVM_, then I'll care

As it applies here, letting vCPU CPUID and actual guest functionality diverge for
features that KVM cares about _will_ cause problems.

This will be less ugly to handle once kvm_vcpu_arch.cpu_caps is a thing.  KVM
can simply force set/clear bits to match the actual guest functionality that's
hardcoded by the TDX Module or defined by TDPARAMS.

> So the configuration goes:
> 1. Userspace configures per-VM CPU features
> 2. Userspace gets TDX Module's final per-vCPU version of CPUID configuration via
> KVM API
> 3. Userspace calls KVM_SET_CPUID2 with the merge of TDX Module's version, and
> userspace's desired values for KVM "owned" CPUID leads (pv features, etc)
> 
> But KVM's knowledge of CPUID bits still remains per-vcpu for TDX in any case.
> 
> > 
> >  - Don't hardcode fixed/required CPUID values in KVM, use available metadata
> >    from TDX Module to reject "bad" guest CPUID (or let the TDX module reject?).
> >    I.e. don't let a guest silently run with a CPUID that diverges from what
> >    userspace provided.
> 
> The latest QEMU patches have this fixed bit data hardcoded in QEMU. Then the
> long term solution is to make the TDX module return this data. Xiaoyao will post
> a proposal on how the TDX module should expose this soon.

Punting the "merge" to userspace is fine, but KVM still needs to ensure it doesn't
have holes where userspace can attack the kernel by lying about what features the
guest has access to.  And that means forcing bits in kvm_vcpu_arch.cpu_caps;
anything else is just asking for problems.

> > [*] https://lore.kernel.org/all/20240405165844.1018872-1-seanjc@google.com
> 
> 
> [0]https://lore.kernel.org/kvm/CABgObfaobJ=G18JO9Jx6-K2mhZ2saVyLY-tHOgab1cJupOe-0Q@mail.gmail.com/