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

[Dave Hansen]

On 7/17/26 14:43, Peter Fang wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> 
> 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?

> -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.

> +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?

>  	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.

> @@ -285,7 +310,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>  	if (desc->inblob_len != TDX_REPORTDATA_LEN)
>  		return -EINVAL;
>  
> -	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> +	memset(quote_data, 0, quote_data_len);
>  
>  	/* Update Quote buffer header */
>  	quote_buf->version = GET_QUOTE_CMD_VER;
> @@ -296,7 +321,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>  	if (ret)
>  		return ret;
>  
> -	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> +	err = tdx_hcall_get_quote(quote_data, quote_data_len);
>  	if (err) {
>  		pr_err("GetQuote hypercall failed, status:%llx\n", err);
>  		return -EIO;
> @@ -315,7 +340,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>  
>  	out_len = READ_ONCE(quote_buf->out_len);
>  
> -	if (out_len > TDX_QUOTE_MAX_LEN)
> +	if (TDX_QUOTE_BUF_LEN(out_len) > quote_data_len)
>  		return -EFBIG;
>  
>  	buf = kvmemdup(quote_buf->data, out_len, GFP_KERNEL);
> @@ -417,7 +442,7 @@ static int __init tdx_guest_init(void)
>  	if (ret)
>  		goto deinit_mr;
>  
> -	quote_data = alloc_quote_buf();
> +	quote_data = alloc_quote_buf(&quote_data_len);
>  	if (!quote_data) {
>  		pr_err("Failed to allocate Quote buffer\n");
>  		ret = -ENOMEM;
> @@ -431,7 +456,7 @@ static int __init tdx_guest_init(void)
>  	return 0;
>  
>  free_quote:
> -	free_quote_buf(quote_data);
> +	free_quote_buf(quote_data, quote_data_len);
>  free_misc:
>  	misc_deregister(&tdx_misc_dev);
>  deinit_mr:
> @@ -444,7 +469,7 @@ module_init(tdx_guest_init);
>  static void __exit tdx_guest_exit(void)
>  {
>  	tsm_report_unregister(&tdx_tsm_ops);
> -	free_quote_buf(quote_data);
> +	free_quote_buf(quote_data, quote_data_len);
>  	misc_deregister(&tdx_misc_dev);
>  	tdx_mr_deinit(tdx_attr_groups[0]);
>  }

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.