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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZiaWMpNm30DD1A-0@google.com>
Date: Mon, 22 Apr 2024 09:54:10 -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 Mon, Apr 22, 2024, Kai Huang wrote:
> On Fri, 2024-04-19 at 10:23 -0700, Sean Christopherson wrote:
> > On Fri, Apr 19, 2024, Kai Huang wrote:
> > And tdx_enable() should also do its best to verify that the caller is post-VMXON:
> > 
> > 	if (WARN_ON_ONCE(!(__read_cr4() & X86_CR4_VMXE)))
> > 		return -EINVAL;
> 
> This won't be helpful, or at least isn't sufficient.
> 
> tdx_enable() can SEAMCALLs on all online CPUs, so checking "the caller is
> post-VMXON" isn't enough.  It needs "checking all online CPUs are in post-
> VMXON with tdx_cpu_enable() having been done".

I'm suggesting adding it in the responding code that does that actual SEAMCALL.

And the intent isn't to catch every possible problem.  As with many sanity checks,
the intent is to detect the most likely failure mode to make triaging and debugging
issues a bit easier.

> I didn't add such check because it's not mandatory, i.e., the later
> SEAMCALL will catch such violation.

Yeah, a sanity check definitely isn't manadatory, but I do think it would be
useful and worthwhile.  The code in question is relatively unique (enables VMX
at module load) and a rare operation, i.e. the cost of sanity checking CR4.VMXE
is meaningless.  If we do end up with a bug where a CPU fails to do VMXON, this
sanity check would give a decent chance of a precise report, whereas #UD on a
SEAMCALL will be less clearcut.

> Btw, I noticed there's another problem, that is currently tdx_cpu_enable()
> actually requires IRQ being disabled.  Again it was implemented based on
> it would be invoked via both on_each_cpu() and kvm_online_cpu().
> 
> It also also implemented with consideration that it could be called by
> multiple in-kernel TDX users in parallel via both SMP call and in normal
> context, so it was implemented to simply request the caller to make sure
> it is called with IRQ disabled so it can be IRQ safe  (it uses a percpu
> variable to track whether TDH.SYS.LP.INIT has been done for local cpu
> similar to the hardware_enabled percpu variable).

Is this is an actual problem, or is it just something that would need to be
updated in the TDX code to handle the change in direction?

> > Actually, duh.  There's absolutely no reason to force other architectures to
> > choose when to enable virtualization.  As evidenced by the massaging to have x86
> > keep enabling virtualization on-demand for !TDX, the cleanups don't come from
> > enabling virtualization during module load, they come from registering cpuup and
> > syscore ops when virtualization is enabled.
> > 
> > I.e. we can keep kvm_usage_count in common code, and just do exactly what I
> > proposed for kvm_x86_enable_virtualization().
> 
> If so, then looks this is basically changing "cpuhp_setup_state_nocalls()
> + on_each_cpu()" to "cpuhp_setup_state()", and moving it along with
> register_syscore_ops() to hardware_enable_all()"?

Yep.