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



On 4/2/2025 8:53 AM, Huang, Kai wrote:

[...]
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index b61371f45e78..90aa7a328dc8 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7162,6 +7162,25 @@ The valid value for 'flags' is:
>     - KVM_NOTIFY_CONTEXT_INVALID -- the VM context is corrupted and not valid
>       in VMCS. It would run into unknown result if resume the target VM.
>   
> +::
> +
> +		/* KVM_EXIT_TDX_GET_QUOTE */
> +		struct tdx_get_quote {
> +			__u64 ret;
> +			__u64 gpa;
> +			__u64 size;
> +		};
> +
> +If the exit reason is KVM_EXIT_TDX_GET_QUOTE, then it indicates that a TDX
> +guest has requested to generate a TD-Quote signed by a service hosting
> +TD-Quoting Enclave operating on the host. The 'gpa' field and 'size' specify
> +the guest physical address and size of a shared-memory buffer, in which the
> +TDX guest passes a TD report.
>
> "TD report" -> "TD Report"?  The changelog uses the latter.
>
>> When completed, the generated quote is returned
> "quote" -> "Quote"?
>
>> +via the same buffer. The 'ret' field represents the return value.
>>
> return value of the GetQuote TDVMCALL?
Yes, thereturn code of the GetQuote TDVMCALL.
>
>> The userspace
>> +should update the return value before resuming the vCPU according to TDX GHCI
>> +spec.
>>
> I don't quite follow.  Why userspace should "update" the return value?
Because only userspace knows whether the request has been queued successfully.

According to GHCI, TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple
requests. This is implementation specific as to how many concurrent requests
are allowed.  The TD should be able to handle TDG.VP.VMCALL_RETRY if it chooses
to issue multiple requests simultaneously.
So the userspace may set the return code as TDG.VP.VMCALL_RETRY.


>
>> It's an asynchronous request. After the TDVMCALL is returned and back to
>> +TDX guest, TDX guest can poll the status field of the shared-memory area.
>> +
>>   ::
>>   
>>   		/* Fix the size of the union. */
>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>> index b952bc673271..535200446c21 100644
>> --- a/arch/x86/kvm/vmx/tdx.c
>> +++ b/arch/x86/kvm/vmx/tdx.c
>> @@ -1463,6 +1463,39 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu)
>>   	return 1;
>>   }
>>   
>> +static int tdx_complete_get_quote(struct kvm_vcpu *vcpu)
>> +{
>> +	tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret);
>> +	return 1;
>> +}
>> +
>> +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.

Right.

Another discussion is whether KVM should skip the sanity checks for GetQuote
and let the userspace take the job.
Considering checking the buffer is shared memory or not, KVM seems to be a
better place.

>
> As a result, the comment
>
>    /* The buffer must be shared memory. */
>
> should also be updated to something like:
>
>    /*
>     * The buffer must be shared. GetQuote requires the GPA to have
>     * shared bit set.
>     */