linux-kernel - Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

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: <e835e48004350cb17cae70b015cc04169e55b743.camel@intel.com>
Date: Thu, 10 Apr 2025 00:15:47 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "seanjc@...gle.com" <seanjc@...gle.com>, "binbin.wu@...ux.intel.com"
	<binbin.wu@...ux.intel.com>
CC: "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>, "Edgecombe,
 Rick P" <rick.p.edgecombe@...el.com>, "Gao, Chao" <chao.gao@...el.com>, "Li,
 Xiaoyao" <xiaoyao.li@...el.com>, "Lindgren, Tony" <tony.lindgren@...el.com>,
	"Hunter, Adrian" <adrian.hunter@...el.com>, "Chatre, Reinette"
	<reinette.chatre@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "Yamahata, Isaku"
	<isaku.yamahata@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Zhao, Yan Y" <yan.y.zhao@...el.com>
Subject: Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

On Wed, 2025-04-09 at 06:49 -0700, Sean Christopherson wrote:
> On Wed, Apr 02, 2025, Binbin Wu wrote:
> > On 4/2/2025 8:53 AM, Huang, Kai wrote:
> > > > +static int tdx_get_quote(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +	struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > +
> > > > +	u64 gpa = tdx->vp_enter_args.r12;
> > > > +	u64 size = tdx->vp_enter_args.r13;
> > > > +
> > > > +	/* The buffer must be shared memory. */
> > > > +	if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) {
> > > > +		tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND);
> > > > +		return 1;
> > > > +	}
> > > It is a little bit confusing about the shared buffer check here.  There are two
> > > perspectives here:
> > > 
> > > 1) the buffer has already been converted to shared, i.e., the attributes are
> > > stored in the Xarray.
> > > 2) the GPA passed in the GetQuote must have the shared bit set.
> > > 
> > > The key is we need 1) here.  From the spec, we need the 2) as well because it
> > > *seems* that the spec requires GetQuote to provide the GPA with shared bit set,
> > > as it says "Shared GPA as input".
> > > 
> > > The above check only does 2).  I think we need to check 1) as well, because once
> > > you forward this GetQuote to userspace, userspace is able to access it freely.
> 
> (1) is inherently racy.  By the time KVM exits to userspace, the page could have
> already been converted to private in the memory attributes.  KVM doesn't control
> shared<=>private conversions, so ultimately it's userspace's responsibility to
> handle this check.  E.g. userspace needs to take its lock on conversions across
> the check+access on the buffer.  Or if userpsace unmaps its shared mappings when
> a gfn is private, userspace could blindly access the region and handle the
> resulting SIGBUS (or whatever error manifests).

Oh right it is racy. :-)

> 
> For (2), the driving motiviation for doing the checks (or not) is KVM's ABI.

Right.

> I.e. whether nor KVM should handle the check depends on what KVM does for
> similar exits to userspace.  Helping userspace is nice-to-have, but not mandatory
> (and helping userspace can also create undesirable ABI).
> 
> My preference would be that KVM doesn't bleed the SHARED bit into its exit ABI.
> And at a glance, that's exactly what KVM does for KVM_HC_MAP_GPA_RANGE.  In
> __tdx_map_gpa(), the so called "direct" bits are dropped (OMG, who's brilliant
> idea was it to add more use of "direct" in the MMU code):
> 
> 	tdx->vcpu.run->hypercall.args[0] = gpa & ~gfn_to_gpa(kvm_gfn_direct_bits(tdx->vcpu.kvm));
> 	tdx->vcpu.run->hypercall.args[1] = size / PAGE_SIZE;
> 	tdx->vcpu.run->hypercall.args[2] = vt_is_tdx_private_gpa(tdx->vcpu.kvm, gpa) ?
> 					   KVM_MAP_GPA_RANGE_ENCRYPTED :
> 					   KVM_MAP_GPA_RANGE_DECRYPTED;
> 
> So, KVM should keep the vt_is_tdx_private_gpa(), but KVM also needs to strip the
> SHARED bit from the GPA reported to userspace.

Yeah fine to me.