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

[Sean Christopherson]

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).

For (2), the driving motiviation for doing the checks (or not) is KVM's ABI.
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.