linux-kernel - Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX 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: Fri, 15 Mar 2024 12:23:46 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
	Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>,
	chen.bo@...el.com, hang.yuan@...el.com, tina.zhang@...el.com,
	Sean Christopherson <sean.j.christopherson@...el.com>,
	Binbin Wu <binbin.wu@...ux.intel.com>,
	Yuan Yao <yuan.yao@...el.com>, isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for
 SEAMCALLs to the TDX module

On Fri, Mar 15, 2024 at 10:41:28AM -0700,
Sean Christopherson <seanjc@...gle.com> wrote:

> On Mon, Feb 26, 2024, isaku.yamahata@...el.com wrote:
> > +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
> > +			       struct tdx_module_args *out)
> > +{
> > +	u64 ret;
> > +
> > +	if (out) {
> > +		*out = *in;
> > +		ret = seamcall_ret(op, out);
> > +	} else
> > +		ret = seamcall(op, in);
> > +
> > +	if (unlikely(ret == TDX_SEAMCALL_UD)) {
> > +		/*
> > +		 * SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
> > +		 * This can happen when the host gets rebooted or live
> > +		 * updated. In this case, the instruction execution is ignored
> > +		 * as KVM is shut down, so the error code is suppressed. Other
> > +		 * than this, the error is unexpected and the execution can't
> > +		 * continue as the TDX features reply on VMX to be on.
> > +		 */
> > +		kvm_spurious_fault();
> > +		return 0;
> 
> This is nonsensical.  The reason KVM liberally uses BUG_ON(!kvm_rebooting) is
> because it *greatly* simpifies the overall code by obviating the need for KVM to
> check for errors that should never happen in practice.  On, and 
> 
> But KVM quite obviously needs to check the return code for all SEAMCALLs, and
> the SEAMCALLs are (a) wrapped in functions and (b) preserve host state, i.e. we
> don't need to worry about KVM consuming garbage or running with unknown hardware
> state because something like INVVPID or INVEPT faulted.
> 
> Oh, and the other critical aspect of all of this is that unlike VMREAD, VMWRITE,
> etc., SEAMCALLs almost always require a TDR or TDVPR, i.e. need a VM or vCPU.
> Now that we've abandoned the macro shenanigans that allowed things like
> tdh_mem_page_add() to be pure translators to their respective SEAMCALL, I don't
> see any reason to take the physical addresses of the TDR/TDVPR in the helpers.
> 
> I.e.  if we do:
> 
> 	u64 tdh_mng_addcx(struct kvm *kvm, hpa_t addr)
> 
> then the intermediate wrapper to the SEAMCALL assembly has the vCPU or VM and
> thus can precisely terminate the one problematic VM.
> 
> So unless I'm missing something, I think that kvm_spurious_fault() should be
> persona non grata for TDX, and that KVM should instead use KVM_BUG_ON().

Thank you for the feedback.  As I don't see any issues to do so, I'll convert
those wrappers to take struct kvm_tdx or struct vcpu_tdx, and eliminate
kvm_spurious_fault() in favor of KVM_BUG_ON().
-- 
Isaku Yamahata <isaku.yamahata@...el.com>