linux-kernel - Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZirUN9G-Y1VUSlDB@google.com>
Date: Thu, 25 Apr 2024 15:07:51 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Kai Huang <kai.huang@...el.com>
Cc: Tina Zhang <tina.zhang@...el.com>, Hang Yuan <hang.yuan@...el.com>, 
	Bo2 Chen <chen.bo@...el.com>, "sagis@...gle.com" <sagis@...gle.com>, 
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Erdem Aktas <erdemaktas@...gle.com>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>, 
	Isaku Yamahata <isaku.yamahata@...el.com>, 
	"isaku.yamahata@...ux.intel.com" <isaku.yamahata@...ux.intel.com>
Subject: Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when
 loading the KVM intel kernel module

On Thu, Apr 25, 2024, Kai Huang wrote:
> On Thu, 2024-04-25 at 09:35 -0700, Sean Christopherson wrote:
> > On Tue, Apr 23, 2024, Kai Huang wrote:
> > > On Tue, 2024-04-23 at 08:15 -0700, Sean Christopherson wrote:
> > > > Presumably that approach relies on something blocking onlining CPUs when TDX is
> > > > active.  And if that's not the case, the proposed patches are buggy.
> > > 
> > > The current patch ([PATCH 023/130] KVM: TDX: Initialize the TDX module
> > > when loading the KVM intel kernel module) indeed is buggy, but I don't
> > > quite follow why we need to block onlining CPU  when TDX is active?
> > 
> > I was saying that based on my reading of the code, either (a) the code is buggy
> > or (b) something blocks onlining CPUs when TDX is active.  Sounds like the answer
> > is (a).
> 
> Yeah it's a).

..

> > > Being a module natually we will need to handle module init and exit.  But
> > > TDX cannot be disabled and re-enabled after initialization, so in general
> > > the vac.ko doesn't quite fit for TDX.
> > > 
> > > And I am not sure what's the fundamental difference between managing TDX
> > > module in a module vs in the core-kernel from KVM's perspective.
> > 
> > VAC isn't strictly necessary.  What I was saying is that it's not strictly
> > necessary for the core kernel to handle VMXON either.  I.e. it could be done in
> > something like VAC, or it could be done in the core kernel.
> 
> Right, but so far I cannot see any advantage of using a VAC module,
> perhaps I am missing something although.

Yeah, there's probably no big advantage.

> > The important thing is that they're handled by _one_ entity.  What we have today
> > is probably the worst setup; VMXON is handled by KVM, but TDX.SYS.LP.INIT is
> > handled by core kernel (sort of).
> 
> I cannot argue against this :-)
> 
> But from this point of view, I cannot see difference between tdx_enable()
> and tdx_cpu_enable(), because they both in core-kernel while depend on KVM
> to handle VMXON.

My comments were made under the assumption that the code was NOT buggy, i.e. if
KVM did NOT need to call tdx_cpu_enable() independent of tdx_enable().

That said, I do think it makes to have tdx_enable() call an private/inner version,
e.g. __tdx_cpu_enable(), and then have KVM call a public version.  Alternatively,
the kernel could register yet another cpuhp hook that runs after KVM's, i.e. does
TDX.SYS.LP.INIT after KVM has done VMXON (if TDX has been enabled).

> Or, do you prefer to we move VMXON to the core-kernel at this stage, i.e.,
> as a prepare work for KVM TDX? 

No, the amount of churn/work that would create is high, and TDX is already taking
on enough churn.