linux-kernel - Re: [PATCH 3/3] KVM: VMX: Initialize TDX during KVM module load

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: <ZyPnC3K9hjjKAWCM@google.com>
Date: Thu, 31 Oct 2024 13:22:35 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Kai Huang <kai.huang@...el.com>
Cc: Dan J Williams <dan.j.williams@...el.com>, Dave Hansen <dave.hansen@...el.com>, 
	Tony Lindgren <tony.lindgren@...el.com>, Rick P Edgecombe <rick.p.edgecombe@...el.com>, 
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>, Reinette Chatre <reinette.chatre@...el.com>, 
	Xiaoyao Li <xiaoyao.li@...el.com>, Yan Y Zhao <yan.y.zhao@...el.com>, 
	Adrian Hunter <adrian.hunter@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"pbonzini@...hat.com" <pbonzini@...hat.com>, Isaku Yamahata <isaku.yamahata@...el.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"kristen@...ux.intel.com" <kristen@...ux.intel.com>
Subject: Re: [PATCH 3/3] KVM: VMX: Initialize TDX during KVM module load

On Thu, Oct 31, 2024, Kai Huang wrote:
> On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > +void __init tdx_bringup(void)
> > > +{
> > > +	enable_tdx = enable_tdx && !__tdx_bringup();
> > 
> > Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> > configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
> 
> Thanks for the comments.
> 
> I see your point.  However for "enabling virtualization failure" kvm_init() will
> also try to do (default behaviour), so if it fails it will result in module
> loading failure eventually.  So while I guess it would be slightly better to
> make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> issue to me.
> 
> I think "enabling virtualization failure" is the only "unexpected issue" that
> should result in module loading failure.  For any other TDX-specific
> initialization failure (e.g., any memory allocation in future patches) it's
> better to only disable TDX.

I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
disable TDX because of a transient, unrelated failure.

If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
disable, then that's different.  And that's the general pattern throughout KVM.
If a requested feature isn't supported, then KVM continues on updates the module
param accordingly.  But if something outright fails during setup, KVM aborts the
entire sequence.

> So I can change to "make loading KVM-the-module fail if enabling virtualization
> fails in TDX", but I want to confirm this is what you want?

I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
would normally succeed, fails.