linux-kernel - Re: [PATCH v19 076/130] KVM: TDX: Finalize VM initialization

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, 12 Apr 2024 11:09:57 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Isaku Yamahata <isaku.yamahata@...el.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
	isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
	erdemaktas@...gle.com, Sean Christopherson <seanjc@...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,
	isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 076/130] KVM: TDX: Finalize VM initialization

On Fri, Apr 12, 2024 at 03:22:00PM +0300,
Adrian Hunter <adrian.hunter@...el.com> wrote:

> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 0d3b79b5c42a..c7ff819ccaf1 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -2757,6 +2757,12 @@ static int tdx_td_finalizemr(struct kvm *kvm)
> >  		return -EINVAL;
> >  
> >  	err = tdh_mr_finalize(kvm_tdx);
> > +	kvm_tdx->hw_error = err;
> > +
> > +	if (err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX))
> 
> There seem to be also implicit operand codes.  How sure are
> we that TDX_OPERAND_ID_RCX is the only valid busy operand?

According to the description of TDH.MR.FINALIZE, it locks exclusively,
RCX in TDR, TDCS as implicit, OP_STATE as implicit.  And the basic TDX feature
to run guest TD, TDX module locks in order of TDR => OP_STATE. We won't see
OP_STATE lock failure after gaining TDR lock.

If you worry for future, we can code it as
(err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_BUSY.  We should do it
consistently, though.

> > +		return -EAGAIN;
> > +	if (err == TDX_NO_VCPUS)
> 
> TDX_NO_VCPUS is not one of the completion status codes for
> TDH.MR.FINALIZE

It depends on the document version.  Need to check TDX_OP_STATE_INCORRECT
to be defensive.


> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index 98f5d7c5891a..dc150b8bdd5f 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -18,6 +18,9 @@ struct kvm_tdx {
> >  	u64 xfam;
> >  	int hkid;
> >  
> > +	/* For KVM_TDX ioctl to return SEAMCALL status code. */
> > +	u64 hw_error;
> 
> For this case, it seems weird to have a struct member
> to pass back a return status code,  why not make it a parameter
> of tdx_td_finalizemr() or pass &tdx_cmd?

I created the patch too quick. Given KVM_TDX_CAPABILITIES and KVM_TDX_INIT_VM
take tdx_cmd already, it's consistent to make tdx_td_finalize() take it.
-- 
Isaku Yamahata <isaku.yamahata@...el.com>