Re: [PATCH v8 08/21] x86/virt/seamldr: Allocate and populate a module update request

Dave Hansen
2026-04-30T00:45:48Z

On 4/27/26 08:28, Chao Gao wrote:
> P-SEAMLDR uses the SEAMLDR_PARAMS structure to describe TDX module
> update requests. This structure contains physical addresses pointing to
> the module binary and its signature file (or sigstruct), along with an
> update scenario field.
>
> TDX modules are distributed in the tdx_blob format defined in
> blob_structure.txt from the "Intel TDX module Binaries Repository". A
> tdx_blob contains a header, sigstruct, and module binary. This is also the
> format supplied by the userspace to the kernel.
>
> Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure. The
> header is consumed solely by the kernel to extract the sigstruct and
> module, so validate it before processing to protect the kernel ABI. The
> sigstruct and module are passed to and validated by P-SEAMLDR, so don't
> duplicate any validation in the kernel.
>
> Note: the sigstruct_pa field in SEAMLDR_PARAMS has been extended to
> a 4-element array. The updated "SEAM Loader (SEAMLDR) Interface
> Specification" will be published separately.

These changelogs have all the right info, but I find them really hard to
parse. For instance, if you're going to have a 'struct seamldr_params',
then just stick with that name. Don't use the "SEAMLDR_PARAMS" name too.

Start with the data structures:

There are two important ABIs here:

'struct tdx_blob' - 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_blob'

> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index 650c0f097aac..f70be8e2a07b 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -7,6 +7,7 @@
> #define pr_fmt(fmt) "seamldr: " fmt
>
> #include <linux/mm.h>
> +#include <linux/slab.h>
> #include <linux/spinlock.h>
>
> #include <asm/seamldr.h>
> @@ -16,6 +17,33 @@
> /* P-SEAMLDR SEAMCALL leaf function */
> #define P_SEAMLDR_INFO 0x8000000000000000
>
> +#define SEAMLDR_MAX_NR_MODULE_PAGES 496
> +#define SEAMLDR_MAX_NR_SIG_PAGES 4

Gah. All this complexity for the variable-length sigstruct to save a
maximum of 4 pages. Wow.

This whole thing could have been:

struct tdx_image {
u16 version; // This ABI is always 0x100
u16 checksum;
u8 signature[8];
u32 length;
u8 reserved[4076];
u8 sigstruct[SIGSTRUCT_SIZE];
u8 module[];
}

One variable array. No module offset calculations or munging.

Why do we do this to ourselves for 3 measly pages? ;)

> +/*
> + * 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.
> + */
> +struct seamldr_params {
> + u32 version;
> + u32 scenario;
> + u64 sigstruct_pa[SEAMLDR_MAX_NR_SIG_PAGES];
> + u8 reserved[80];
> + u64 num_module_pages;
> + u64 mod_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,128 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
> }
> EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
> +/*
> + * Intel TDX module blob. Its format is defined at:
> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt

Heh, so URLs are not OK in changelogs because they go stale, but they're
fine in the code?

> + * Note this structure differs from the reference above: the two variable-length
> + * fields "@sigstruct" and "@module" are represented as a single "@data" field
> + * here and split programmatically using the offset_of_module value.

This is good info. But, it's copied and pasted between the changelog and
here. I'd choose one, honestly.

> + * Note @offset_of_module is relative to the start of struct tdx_blob, not
> + * @data, and @length is the total length of the blob, not the length of
> + * @data.
> + */

Out of line comments aren't great. Do these in the data structure if at
all possible. Or, in the code. For instance:

> +struct tdx_blob {
> + u16 version; // This ABI is always 0x100
> + u16 checksum;
> + u32 offset_of_module; // from start of tdx_blob
> + u8 signature[8];
> + u32 length;
> + u32 reserved0;
> + u64 reserved1[509];
> + u8 data[]; // contains sigstruct[] and module[]
> +} __packed;

That's probably _better_ than the two duplicated comments that are there
now.

Also, why bother having two reserved arrays instead of:

u8 reserved[4076];

?

> +/* Supported versions of the tdx_blob */
> +#define TDX_BLOB_VERSION_1 0x100

The comment here doesn't help much.

> +/*
> + * Blob fields are processed by the kernel and the payloads
> + * are passed to the TDX module. Do normal user input type
> + * check for any fields that don't get passed to the TDX module.
> + */

I made it this far, but I rather despise the 'blob' terminology. It's
just bad naming. We should really just call it 'tdx_update_image' or
'tdx_image' everywhere and stop saying 'blob'. Blob is one of those
names that people throw at things when they give up on naming.

> +static const struct tdx_blob *get_and_check_blob(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> +
> + /*
> + * Ensure the size is valid otherwise reading any field from the
> + * blob may overflow.
> + */
> + if (size <= sizeof(struct tdx_blob))
> + return ERR_PTR(-EINVAL);

Couple of things here:

First, using sizeof() on a type with a variable-length array is a big
warning sign. It needs commenting. It's especially subtle because this
will go on and parse patently invalid 'data' images that don't even have
room for sigstruct[] or module[].

This is *specifically* about the pre-data[] fields that are going to be
read below.

> + /*
> + * Don't care about user passing the wrong file, but protect
> + * kernel ABI by preventing accepting garbage.
> + */
> + if (memcmp(blob->signature, "TDX-BLOB", 8))
> + return ERR_PTR(-EINVAL);

Is there really no helper in the kernel anywhere that can safely do the
8-byte compare against two known-to-the-compiler 8-byte-wide fields
without hard-coding the 8?

> + /*
> + * Ensure the offset of the module is within valid bounds and
> + * page-aligned.
> + */
> + if (blob->offset_of_module >= size || blob->offset_of_module <= sizeof(struct tdx_blob))
> + return ERR_PTR(-EINVAL);

Again, the sizeof(struct tdx_blob) is wonky. Why does this disallow
pointing blob->offset_of_module at reserved1[508] but not sigstruct[]?

> + if (!IS_ALIGNED(blob->offset_of_module, PAGE_SIZE))
> + return ERR_PTR(-EINVAL);

Wait a sec. Unless blob->offset_of_module==0, how could this check pass
and "blob->offset_of_module <= sizeof(struct tdx_blob)" fail?

> + if (blob->version != TDX_BLOB_VERSION_1)
> + return ERR_PTR(-EINVAL);

This should be the first check, IMNHO. If this doesn't pass then the
rest of the fields are invalid. No?

> + if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
> + return ERR_PTR(-EINVAL);

There should not be two reserved, must-be-0 fields. There should be 1.
There must be 1.

Also I don't like the proposed data structure. It would make a lot more
sense to me if it were:

struct tdx_image_header {
u16 version; // This ABI is always 0x100
u16 checksum;
u32 offset_of_module; // from start of the header
u8 signature[8];
u32 length;
u8 reserved[4076];
}

struct p {
u8[PAGE_SIZE];
};

struct tdx_image {
struct tdx_image_header h;
struct p pages[];
};

Then you can do things like check if sizeof(struct tdx_image_header) ==
PAGE_SIZE. Or whether offset_of_module points past the header.

That stuff only makes sense if you separate out the header structure
from the payloads which are the page-aligned sigstruct and module image
itself.

But exposing the double-variable-length arrays seems really wonky to me.

> + return blob;
> +}
> +
> +static struct seamldr_params *alloc_seamldr_params(const struct tdx_blob *blob, unsigned int blob_size)
> +{

This does far more than "alloc" something.

> + struct seamldr_params *params;
> + int module_pg_cnt, sig_pg_cnt;
> + const u8 *sig, *module;
> + int i;
> +
> + params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> + if (!params)
> + return ERR_PTR(-ENOMEM);

kzmalloc(PAGE_SIZE, GFP_KERNEL) will save you a cast.

> + /*
> + * Split the blob into a sigstruct and a module. Assume all
> + * size/offsets are within bounds of blob_size due to prior checks.
> + */
> + sig = blob->data;
> + sig_pg_cnt = (blob->offset_of_module - sizeof(struct tdx_blob)) >> PAGE_SHIFT;

Of course, the size of the first thing is defined by the offset of the
second thing.

This really should just be called ->end_of_sig.

> + module = (const u8 *)blob + blob->offset_of_module;
> + module_pg_cnt = (blob_size - blob->offset_of_module) >> PAGE_SHIFT;

This looks halfway sane:

/* adjust for size of the header: */
sig_size = blob->end_of_sig - PAGE_SIZE;
module_size = module_image_size - blob->end_of_sig;

Then, page-adjust it later. One bit of magic at a time, please.

> + /*
> + * Only use version 1 when required (sigstruct > 4KB) for backward
> + * compatibility with P-SEAMLDR that lacks version 1 support.
> + */
> + params->version = sig_pg_cnt > 1;

Ewwww.

But what do we do if we're on an old P-SEAMLDR but get a big sigstruct?
It'll just fail?

How many old P-SEAMLDRs are there in the wild? Do we even care about this?

> + params->scenario = SEAMLDR_SCENARIO_UPDATE;
> +
> + for (i = 0; i < MIN(sig_pg_cnt, SEAMLDR_MAX_NR_SIG_PAGES); i++) {

Same for the MIN(). Do all the calculations separate from the loop.

> + params->sigstruct_pa[i] = vmalloc_to_pfn(sig) << PAGE_SHIFT;
> + sig += PAGE_SIZE;
> + }
> +
> + params->num_module_pages = MIN(module_pg_cnt, SEAMLDR_MAX_NR_MODULE_PAGES);
> + for (i = 0; i < params->num_module_pages; i++) {
> + params->mod_pages_pa_list[i] = vmalloc_to_pfn(module) << PAGE_SHIFT;
> + module += PAGE_SIZE;
> + }

Really what you want here is a helper. Have it take the module or
sigstruct pointer, a pointer to the pa_list[] and a maximum size.

Then call the helper twice.

> + return params;
> +}
> +
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob;
> +
> + blob = get_and_check_blob(data, size);
> + if (IS_ERR(blob))
> + return ERR_CAST(blob);
> +
> + return alloc_seamldr_params(blob, size);
> +}
> +
> +DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> + if (!IS_ERR_OR_NULL(_T)) free_page((unsigned long)_T))

Is this really worth it?

> /**
> * seamldr_install_module - Install a new TDX module.
> * @data: Pointer to the TDX module update blob.
> @@ -52,6 +202,11 @@ EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
> */
> int seamldr_install_module(const u8 *data, u32 size)
> {
> + struct seamldr_params *params __free(free_seamldr_params) =
> + init_seamldr_params(data, size);
> + if (IS_ERR(params))
> + return PTR_ERR(params);
> +
> /* TODO: Update TDX module here */
> return 0;
> }

IMNHO, this patch has way too much going on. It took well over an hour
to go through it. That's problematic.