Re: [PATCH v9 11/23] x86/virt/seamldr: Allocate and populate a module update request

[Dave Hansen <dave.hansen@intel.com>]

On 5/13/26 08:09, Chao Gao wrote:
> There are two important ABIs here:
> 
> 'struct tdx_image'	- The on-disk and in-memory format for a TDX
> 			  module update image.
> 'struct seamldr_params'	- The in-memory ABI passed to the TDX module
> 			  loader. Points to a single 'struct tdx_image'

	... broken up into 4k pages

> Userspace supplies the update image in struct tdx_image format. The image
> consists of a header followed by a sigstruct and the module binary.
> 
> P-SEAMLDR, however, consumes struct seamldr_params rather than the image
> directly. Parse the struct tdx_image provided by userspace and populate a
> matching struct seamldr_params.

This is doing it again. It's taking the key imperative and burying it.
It should be:

=>
Userspace supplies the update image in struct tdx_image format. The
image consists of a header followed by a sigstruct and the module
binary. P-SEAMLDR, however, consumes struct seamldr_params rather than
the image directly.

Parse the struct tdx_image provided by userspace and populate a matching
struct seamldr_params.
<=

> Validate the struct tdx_image header before using it, because the header is
> consumed solely by the kernel to locate the sigstruct and module within
> the image. Do not validate the payload itself. The sigstruct and module
> pages are passed through to P-SEAMLDR, which validates them as part of the
> update flow.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> sigstruct_pages_pa_list currently has only one entry, but it will grow to
> four pages in the future. Keep it as an array for symmetry with
> module_pages_pa_list and for extensibility.

That's a good note. It can go above the ---.

> + * The seamldr_params "scenario" field specifies the operation mode:
> + * 0: Install TDX module from scratch (not used by kernel)
> + * 1: Update existing TDX module to a compatible version
> + */
> +#define SEAMLDR_SCENARIO_UPDATE		1
> +
> +/*
> + * This is called the "SEAMLDR_PARAMS" data structure and is defined
> + * in "SEAM Loader (SEAMLDR) Interface Specification".
> + *
> + * It describes the TDX module that will be installed.

Yeah, but that's not super useful.

	This is the in-memory ABI that the kernel passes to the P-
	SEAMLDR to update the TDX module. It breaks the TDX module image
	up in page-size pieces.

Better?

> + */
> +struct seamldr_params {
> +	u32	version;
> +	u32	scenario;
> +	u64	sigstruct_pages_pa_list[SEAMLDR_MAX_NR_SIG_PAGES];
> +	u8	reserved[104];
> +	u64	module_nr_pages;
> +	u64	module_pages_pa_list[SEAMLDR_MAX_NR_MODULE_PAGES];
> +} __packed;
> +
> +static_assert(sizeof(struct seamldr_params) == 4096);
> +
>  /*
>   * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
>   * interact with P-SEAMLDR simultaneously. Use raw version as the calls can
> @@ -43,6 +71,89 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
>  }
>  EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>  
> +#define TDX_IMAGE_VERSION_2		0x200
> +
> +struct tdx_image_header {
> +	u16	version; // This ABI is always 0x200

That comment reads strangely in here. Did I ask you to write that?

> +	u16	checksum;
> +	u8	signature[8];
> +	u32	sigstruct_nr_pages;
> +	u32	module_nr_pages;
> +	u8	reserved[4076];
> +} __packed;
> +
> +#define HEADER_SIZE sizeof(struct tdx_image_header)
> +static_assert(HEADER_SIZE == 4096);
> +
> +/* Intel TDX module update ABI structure. aka. "TDX module blob". */
> +struct tdx_image {
> +	struct tdx_image_header header;
> +	u8 payload[]; // Contains sigstruct pages followed by module pages
> +};
> +
> +static void populate_pa_list(u64 *pa_list, u32 max_entries, const u8 *start, u32 nr_pages)

The naming in there is painful. How about:

populate_pa_list(u64 *pa_list, u32 pa_list_len,
		 const u8 *vmalloc_addr, u32 vmalloc_len_pages)

> +{
> +	int i;
> +
> +	nr_pages = MIN(nr_pages, max_entries);

This seems wonky. Should it really be silently suppressing things if
either the allocation or source is too small? I get not wanting to
overflow, but this seems strange.

> +	for (i = 0; i < nr_pages; i++) {
> +		pa_list[i] = vmalloc_to_pfn(start) << PAGE_SHIFT;
> +		start += PAGE_SIZE;
> +	}

At the point that you modify 'start', it's not 'start' any more. Use
another variable. This would do, for instance:

	for (i = 0; i < nr_pages; i++) {
		unsigned long offset = i * PAGE_SIZE;

		pa_list[i] = vmalloc_to_pfn(&start[offset]);
	}


> +static void populate_seamldr_params(struct seamldr_params *params,
> +				    const u8 *sig, u32 sig_nr_pages,
> +				    const u8 *mod, u32 mod_nr_pages)
> +{
> +	params->version			= 0;
> +	params->scenario		= SEAMLDR_SCENARIO_UPDATE;
> +	params->module_nr_pages		= mod_nr_pages;
> +
> +	populate_pa_list(params->sigstruct_pages_pa_list, SEAMLDR_MAX_NR_SIG_PAGES,
> +			 sig, sig_nr_pages);
> +	populate_pa_list(params->module_pages_pa_list, SEAMLDR_MAX_NR_MODULE_PAGES,
> +			 mod, mod_nr_pages);
> +}

Yes, this is starting to look OK. Nit: vertically align the "*_PAGES" args:


	populate_pa_list(params->sigstruct_pages_pa_list, SEAMLDR_...,
			 sig, sig_nr_pages);
	populate_pa_list(params->module_pages_pa_list,    SEAMLDR_...,
			 mod, mod_nr_pages);


> +static int init_seamldr_params(struct seamldr_params *params, const u8 *data, u32 size)
> +{
> +	const struct tdx_image *image		= (const void *)data;
> +	const struct tdx_image_header *header	= &image->header;
> +
> +	u32 sigstruct_len	= header->sigstruct_nr_pages * PAGE_SIZE;
> +	u32 module_len		= header->module_nr_pages * PAGE_SIZE;
> +
> +	u8 *header_start	= (u8 *)header;
> +	u8 *header_end		= header_start + HEADER_SIZE;
> +
> +	u8 *sigstruct_start	= header_end;
> +	u8 *sigstruct_end	= sigstruct_start + sigstruct_len;
> +
> +	u8 *module_start	= sigstruct_end;
> +
> +	/* Check the calculated payload size against the data size. */
> +	if (HEADER_SIZE + sigstruct_len + module_len != size)
> +		return -EINVAL;
> +
> +	/*
> +	 * Don't care about user passing the wrong file, but protect
> +	 * kernel ABI by preventing accepting garbage.
> +	 */

How does this "protect kernel ABI"?

> +	if (header->version != TDX_IMAGE_VERSION_2)
> +		return -EINVAL;
> +
> +	if (memcmp(header->signature, "TDX-BLOB", sizeof(header->signature)))
> +		return -EINVAL;
> +
> +	if (memchr_inv(header->reserved, 0, sizeof(header->reserved)))
> +		return -EINVAL;
> +
> +	populate_seamldr_params(params, sigstruct_start, header->sigstruct_nr_pages,
> +				module_start, header->module_nr_pages);

Please work on the vertical alignment if there's a pattern. For
instance, it makes things pop if the two "header->"'s are vertically
aligned.