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



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!