Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation

[Peter Fang <peter.fang@intel.com>]

On Wed, Jul 08, 2026 at 02:37:55PM -0700, Sean Christopherson wrote:
> On Thu, Jun 18, 2026, Xu Yilun wrote:
> >  void __init tdx_early_init(void);
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index ac8323a68b16..5e4b3aee0577 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -47,6 +47,12 @@ struct kvm_tdx {
> >  	 * Set/unset is protected with kvm->mmu_lock.
> >  	 */
> >  	bool wait_for_sept_zap;
> > +
> > +	/*
> > +	 * Whether to get the quote directly in kernel, without exiting to
> > +	 * userspace.
> > +	 */
> > +	bool get_quote_in_kernel;
> 
> Why is this cached per-VM?  IIUC, tdx_quote_enabled() is constant after initial
> TDX bringup.

Ah you're right. It shouldn't change. I think I did this because there
was an older version where quoting was not enabled during core TDX
bringup. So this was an attempt to make sure a guest doesn't see
tdx_quote_enabled() flip. It shouldn't be here. Sorry about that.

> 
> And is keying off tdx_quote_enabled() and only tdx_quote_enabled() backwards
> compatible?  How do we know that cutting userspace out of the loop wont' break
> anything?

Yeah this needs explaining:

The guest-observable difference between the two paths is that it sees
quotes in different formats (the TDX module uses newer formats of
course). And an old guest must be able to continue using its
TDVMCALL<GetQuote> interface for quotes. The guest sends the quote to an
external verifier to authenticate itself. The verifier (part of the
cloud infrastructure) must know how to parse this format. So in other
words, it's the verifier that must maintain backward compatibility if it
wants to continue accepting old quotes. And it needs to be updated to
support newer formats as well.

> 
> 
> >  };
> >  
> >  /* TDX module vCPU states */
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 9f7c39e0d4b5..20558b0185b6 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1538,11 +1538,133 @@ static int tdx_get_quote_user(struct kvm_vcpu *vcpu, u64 gpa, u64 size)
> >  	return 0;
> >  }
> >  
> > +static bool write_quote_status_to_guest(struct kvm_vcpu *vcpu, u64 status,
> > +					gpa_t gpa)
> > +{
> > +	if (kvm_vcpu_write_guest(vcpu,
> > +				 gpa + offsetof(struct tdx_quote_req, status),
> > +				 &status, sizeof(status)))
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> This is a completely useless wrapper.  Actually, it's worse than useless, as it
> inverts the return value from the widely used kvm_vcpu_write_guest() API.

I'll remove this. I got feedback that tdx_get_quote_kernel() was full of
kvm_vcpu_write_guest()s and was hard to read, so I tried to break it
apart. Looks like it didn't help.

> 
> > +
> > +static bool write_quote_to_guest(struct kvm_vcpu *vcpu, void *quote_data,
> > +				 u32 quote_len, gpa_t gpa)
> > +{
> > +	if (kvm_vcpu_write_guest(vcpu,
> > +				 gpa + TDX_QUOTE_REQ_HDR_SIZE,
> 
> This is unnecessarily confusing.  I had to look at the definition of
> TDX_QUOTE_REQ_HDR_SIZE to understand that this just writing to tdx_quote_req.data.
> I might not care if this was the only write, but then the very next statement
> writes to a field that isn't obviously in the header.

I'll fix this. I can just spell out offsetof() to make this consistent.

> 
> > +				 quote_data, quote_len))
> > +		return false;
> > +
> > +	if (kvm_vcpu_write_guest(vcpu,
> > +				 gpa + offsetof(struct tdx_quote_req, out_len),
> > +				 &quote_len, sizeof(quote_len)))
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> So is this.  There's literally one caller.

Yeah breaking the kvm_vcpu_write_guest()s apart didn't help...

> 
> > +
> > +static u64 get_quote_kernel(struct kvm_vcpu *vcpu, struct tdx_quote_req *req,
> > +			    gpa_t req_gpa, size_t total_len)
> > +{
> > +	struct tdx_td *td = &to_kvm_tdx(vcpu->kvm)->td;
> > +
> > +	/* Only support version 1 as defined in the GHCI spec */
> > +	if (req->version != 1)
> > +		return TDX_QUOTE_STATUS_ERROR;
> > +
> > +	/* Header + input data must fit in the page read from guest memory */
> > +	if ((size_t)req->in_len + TDX_QUOTE_REQ_HDR_SIZE > PAGE_SIZE)
> > +		return TDX_QUOTE_STATUS_ERROR;
> > +
> > +	/* Caller owns the requested quote */
> > +	void *quote_data __free(kvfree) =
> > +		tdx_quote_generate(td, req->data, req->in_len, &req->out_len);
> 
> Am I the only person that finds the __free() stuff to be difficult to follow?
> I'm sure there are flows where it's a big net positive, but IMO this isn't one.
> If the *unnecesary* layers of single-use helpers are stripped away, I suspect
> we'll naturally end up with a goto sequence that is more obvious that using __free().

The main concern I had was that the guest could give random message
lengths and mess with the host's memory allocator. Core TDX has a better
idea because it knows the maximum possible quote size through a metadata
field, so I just let the API return the final quote from the TDX module.
It's a bit annoying that the caller now has to free it.

Several folks suggested letting KVM allocate and free the buffer, so I
think I'll do that. It should make this easier to read, and this
__free() can be replaced with a much simpler pattern.

> 
> > +
> > +	if (!quote_data)
> > +		return TDX_QUOTE_STATUS_UNAVAILABLE;
> > +
> > +	if ((size_t)req->out_len + TDX_QUOTE_REQ_HDR_SIZE > total_len)
> 
> Can we really not pre-check the size?

Yeah like the response above, KVM doesn't know what a "sane" total_len
is here. out_len comes from the TDX module so I used it to check against
total_len. If we let KVM allocate+free the quote buffer, then I'll also
let KVM get the maximum quote size from core TDX. I was trying to save
some code in KVM but looks like it's not worth it.

> 
> > +		return TDX_QUOTE_STATUS_ERROR;
> > +
> > +	if (!write_quote_to_guest(vcpu, quote_data, req->out_len, req_gpa))
> > +		return TDX_QUOTE_STATUS_ERROR;
> > +
> > +	return TDX_QUOTE_STATUS_SUCCESS;
> > +}
> > +
> > +static u64 tdx_get_quote_check_args(struct kvm_vcpu *vcpu, u64 gpa, u64 size)
> 
> Adding single use helpers does not make the code easier to read.

Yep. Will fix.

> 
> > +{
> > +	gfn_t gfn_start, gfn_end;
> > +	u64 end;
> > +
> > +	if (!size)
> > +		return TDVMCALL_STATUS_INVALID_OPERAND;
> > +
> > +	if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size))
> > +		return TDVMCALL_STATUS_ALIGN_ERROR;
> > +
> > +	if (check_add_overflow(gpa, size, &end))
> > +		return TDVMCALL_STATUS_INVALID_OPERAND;
> > +
> > +	gfn_start = gpa_to_gfn(gpa);
> > +	gfn_end = gpa_to_gfn(end);
> > +
> > +	/*
> > +	 * Reject if the guest didn't explicitly convert its quote pages to
> > +	 * shared.
> > +	 */
> > +	if (!kvm_range_has_memory_attributes(vcpu->kvm, gfn_start, gfn_end,
> > +					     KVM_MEMORY_ATTRIBUTE_PRIVATE, 0))
> 
> TOCTOU?

I struggled a bit with this actually... I can't just grab
kvm->slots_lock here, and dropping kvm->srcu seems bad because the quote
will be written to guest memory later. I settled on the idea that the
guest should make sure these pages are converted to shared first. If it
tries to convert them back to private after this check, before this
TDVMCALL returns, then it's kind of a self-inflicted race and I don't
see KVM doing any self damage in this case. Not sure if this makes sense
to you? Or maybe I should just drop this check and go straight to
kvm_vcpu_read_guest()?


Thanks,
Peter

> 
> > +		return TDVMCALL_STATUS_INVALID_OPERAND;
> > +
> > +	return TDVMCALL_STATUS_SUCCESS;
> > +}