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: <2f56b5c7-f722-450b-9da8-1362700b77ef@intel.com>
Date: Fri, 1 Nov 2024 10:21:36 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: "Williams, Dan J" <dan.j.williams@...el.com>, "Hansen, Dave"
	<dave.hansen@...el.com>, "Lindgren, Tony" <tony.lindgren@...el.com>,
	"Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "binbin.wu@...ux.intel.com"
	<binbin.wu@...ux.intel.com>, "Chatre, Reinette" <reinette.chatre@...el.com>,
	"Li, Xiaoyao" <Xiaoyao.Li@...el.com>, "Zhao, Yan Y" <yan.y.zhao@...el.com>,
	"Hunter, Adrian" <adrian.hunter@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"Yamahata, Isaku" <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 1/11/2024 9:22 am, Sean Christopherson wrote:
> 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.

OK will change to what you suggested.  I'll need to take a deeper look 
though since later patches will add more checks.

Thanks for the comments!