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

[Binbin Wu <binbin.wu@linux.intel.com>]

On 4/9/2025 9:49 PM, 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).
>
> 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;
GetQuote is the first TDX specific KVM exit reason, previous TDVMCALLs that
exit to userspace are converted to exist KVM exit ABIs, e.g., TDVMCALL_MAP_GPA
is converted to KVM_EXIT_HYPERCALL with KVM_HC_MAP_GPA_RANGE, so the GPA passed
to userspace must have the shared bit dropped.

>
> 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.
It makes sense to make the GPA format consistent to userspace.
Thanks for the suggestion!