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

["Huang, Kai" <kai.huang@intel.com>]

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.