Re: [PATCH v2 2/2] virt: tdx-guest: Allocate Quote buffer dynamically

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

On Mon, Jul 20, 2026 at 06:35:29AM -0700, Dave Hansen wrote:
> On 7/17/26 14:43, Peter Fang wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > The TDX attestation driver currently uses a fixed 128 KB Quote buffer
> > shared with the host VMM. This may be too small for Quotes using schemes
> > such as post-quantum cryptography (PQC), where larger certificate chains
> > can increase the Quote size significantly.
> > 
> > Allocate the Quote buffer based on the size reported by the TDX module
> > instead of always reserving a fixed-size buffer. This avoids wasting
> > memory on platforms that do not require larger Quotes. Older platforms
> > fall back to the default 128 KB buffer.
> > 
> > Because the Quote buffer must be physically contiguous, its size is
> > bound by the buddy allocator's maximum page order (4 MB), which should
> > be sufficient for current attestation needs.
> 
> This is all talking about post-quantum-crypto and all that fancy stuff.
> 
> Isn't the important part here that the old TDX module ABI had static
> quote sizes and now they're dynamic? Now, the reason it changed is all
> the fancy stuff.
> 
> But the ABI changed. Right?

Ah yes... I should call out that the size of
/sys/kernel/config/tsm/report/$name/outblob will no longer be fixed.
I'll update this. Thanks.

> 
> > -static void *alloc_quote_buf(void)
> > +static size_t get_quote_buf_size(void)
> >  {
> > -	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> > -	unsigned int count = len >> PAGE_SHIFT;
> > +	size_t buf_size = GET_QUOTE_DEFAULT_BUF_SIZE;
> > +	u32 quote_size;
> > +
> > +	quote_size = tdx_get_max_quote_size();
> > +
> > +	if (quote_size)
> > +		/* Reported size does not include GetQuote header */
> > +		buf_size = TDX_QUOTE_BUF_LEN(quote_size);
> > +
> > +	return PAGE_ALIGN(buf_size);
> > +}
> 
> This code is almost nonsensical on the surface.
> 
> It _really_ needs some commenting. Things like:
> 
> 	/* Start with the default quote buffer size: */
> 
> ...
> 
> 	/* Override the default when ... */
> 
> 
> You could even comment the function to say what it is trying to do overall.

Got it. I'll improve this pattern and add more comments.

> 
> > +static void *alloc_quote_buf(size_t *buflen)
> > +{
> > +	unsigned int count;
> > +	size_t len;
> >  	void *addr;
> >  
> > -	addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> > +	len = get_quote_buf_size();
> > +
> > +	/*
> > +	 * This fails if the requested size exceeds the buddy allocator's
> > +	 * maximum order. Use __GFP_NOWARN since the size comes from the host
> > +	 * and should fail quietly rather than warn.
> > +	 */
> > +	addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN);
> 
> Bad Sashiko. Bad.
> 
> The host may be untrusted, but it's also a critical part of the system.
> Are we sure we want to be completely quiet?
> 
> I used to see little dmesg warnings about TCP window shenanigans from
> random systems on the Internet. Maybe that's not how we do things today,
> but if a random dude on the Internet can spew one line to dmesg, is it
> that crazy that a bad VMM be able to spew a warning?

That makes sense... I actually argued with AI about this but it kept
saying this is kind of like DoSing the guest. But thinking about it
more, tainting the guest is probably the right thing to do... At least
the guest sees a big splat about why attestation is failing. I'll remove
the __GFP_NOWARN. Thanks.

> 
> >  	if (!addr)
> >  		return NULL;
> >  
> > +	count = len >> PAGE_SHIFT;
> > +
> >  	if (set_memory_decrypted((unsigned long)addr, count))
> >  		return NULL;
> >  
> > +	*buflen = len;
> > +
> >  	return addr;
> >  }
> 
> This feels weird to me.
> 
> If the upper-layer function needs to know the size, why not have it just
> call get_quote_buf_size()? Then there's no pass-by-address.

Got it. I'll fix this.

> 

[ ... ]

> 
> So, yeah, this patch isn't gigantic. But it's also fundamentally not
> doing a _nice_ refactoring the way we expect them to be done.
> 
> 1. Refactor old code to make it nice for adding features
> 2. Add the feature
> 
> If this was doing it the nice way, we would *actually* have something
> that's really close to s/GET_QUOTE_BUF_SIZE/quote_data_len/. But,
> instead, this chose to cram the mechanical changes and the new feature
> together.
> 
> Can we do it the right way, please? If for nothing else, for practice.

Yes I'll separate out the refactoring so that the dynamic buffer thing
is on its own. Thanks for the feedback.