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.
