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]

Date: Wed, 1 May 2024 14:56:30 +1200
From: "Huang, Kai" <kai.huang@...el.com>
To: Sean Christopherson <seanjc@...gle.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 1/05/2024 4:13 am, Sean Christopherson wrote:
> On Tue, Apr 30, 2024, Kai Huang wrote:
>> On 30/04/2024 8:06 am, Sean Christopherson wrote:
>>> My suggestion is essentially "throw in a CR4.VMXE check before
>>> TDH.SYS.LP.INIT if it's easy".  If it's not easy for some reason, then don't do
>>> it.
>>
>> I see.  The disconnection between us is I am not super clear why we should
>> treat TDH.SYS.LP.INIT as a special one that deserves a CR4.VMXE check but
>> not other SEAMCALLs.
> 
> Because TDH.SYS.LP.INIT is done on all CPUs via an IPI function call, is a one-
> time thing, and is at the intersection of core TDX and KVM module code, e.g. the
> the core TDX code has an explicit assumption that:
> 
>   * This function assumes the caller has: 1) held read lock of CPU hotplug
>   * lock to prevent any new cpu from becoming online; 2) done both VMXON
>   * and tdx_cpu_enable() on all online cpus.

Yeah but from this perspective, both tdx_cpu_enable() and tdx_enable() 
are "a one time thing" and "at the intersection of core TDX and KVM"  :-)

But from the perspective that tdx_cpu_enable() must be called in IRQ 
disabled context, and there's no possibility that other thread/code 
could potentially mess up VMX enabling after the CR4.VMXE check, so it's 
fine to add such check.

And looking again, in fact the comment of tdx_cpu_enable() doesn't 
explicitly call out it requires the caller to do VMXON first (although 
kinda implied by the comment of tdx_enable() as you quoted above).

I can add a patch to make it more clear by calling out in the comment of 
tdx_cpu_enable() that it requires caller to do VMXON and adding a 
WARN_ON_ONCE(!CR4.VMXE) check inside.  I just don't know whether it is 
worth to do at this stage given it's not something mandatory because it 
requires review time from maintainers.  I can include such patch in next 
KVM TDX patchset if you prefer so we can see how it goes.

> 
> KVM can obviously screw up and attempt SEAMCALLs without being post-VMXON, but
> that's entirely a _KVM_ bug.  And the probability of getting all the way to
> something like TDH_MEM_SEPT_ADD without being post-VMXON is comically low, e.g.
> KVM and/or the kernel would likely crash long before that point.

Yeah fully agree SEAMCALLs managed by KVM shouldn't need to do CR4.VMXE 
check.  I was talking about those involved in tdx_enable(), i.e., 
TDH.SYS.xxx.