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

Chao Gao
2026-04-14T09:44:15Z

On Sat, Apr 11, 2026 at 09:14:36AM +0800, Edgecombe, Rick P wrote:
>On Tue, 2026-03-31 at 05:41 -0700, 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
>> accordingly. This structure will be passed to P-SEAMLDR to initiate the
>> update.
>
>The thing that confused me about this earlier was the exact reason why we are
>checking all the fields. We discussed that we need to check the fields that
>kernel processes, but we don't need to double check data that the TDX module
>processes.
>
>Should we explain it?

Yes. how about adding the following in the changelog:

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. The sigstruct and module are
passed to and validated by P-SEAMLDR, so don't duplicate any validation
in the kernel.

>And how it explains the checks below?

>> +static void free_seamldr_params(struct seamldr_params *params)
>> +{
>> + free_page((unsigned long)params);
>> +}
>
>Do we really need this helper? This doesn't work?

No. This works. Will do.

>
>DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> if (!IS_ERR_OR_NULL(_T)) free_page((unsigned long)_T))
>
>
>> +
>> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
>> + const void *sig, unsigned int sig_size)
>> +{
>> + struct seamldr_params *params;
>> + const u8 *ptr;
>> + int i;
>> +
>> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K)
>> + return ERR_PTR(-EINVAL);
>
>I don't know if it's worth that much, but we could do a MIN thing here to
>protect the loop, and lose the conditionals. If userspace passes a blob that is
>out of spec they can deal with the module error, no?

I think we all agree: we need to do something to protect the loop. So, the
question is just how.

looks like you prefer:

module_size = MIN(module_size, SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K);
sig_size = MIN(sig_size, SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K);

But I think the MIN approach actually requires more justification than a plain
bounds check.

With MIN, the reasoning chain is: we don't want conditionals -> userspace
can deal with the module error -> and that error won't be fatal to the
system (or even if it is, it's admin-initiated). That's a lot of assumptions
to validate.

With a bounds check and early return, the reasoning is simply: the input is out
of range -> reject it as invalid.

>
>> +
>> + /*
>> + * Check that input buffers satisfy P-SEAMLDR's size and alignment
>> + * constraints so they can be passed directly to P-SEAMLDR without
>> + * relocation or copy.
>> + */
>> + if (!IS_ALIGNED(module_size, SZ_4K) || !IS_ALIGNED(sig_size, SZ_4K) ||
>> + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
>> + !IS_ALIGNED((unsigned long)sig, SZ_4K))
>> + return ERR_PTR(-EINVAL);
>
>I thought you are going to reduce this checking to just to reject invalid input
>that the kernel processes.
>
>What happens if we don't check this?

If the blob->offset_of_module or blob->size is misaligned, the loops
silently drop the unaligned portion. P-SEAMLDR will then reject the update
after module shutdown and TDs will be killed. Since this is admin-initiated
with an intentionally invalid blob, the consequence is acceptable.

>The vmallocs are all going to be page
>aligned anyway. But even still, does it mess up the below loops somehow in a way
>that hurts anything?

I agree addresses/sizes are page-aligned for valid blobs, but they are not
guaranteed (e.g., if offset_of_module is misaligned) for all inputs. I
removed the check once in v6 and Sashiko questioned it, so I thought the
assumption isn't obviously correct to everyone.

Without the check, we'd need something like:

/*
* Assume sig/module base addresses and sizes are page-aligned.
* If violated, P-SEAMLDR will reject the update.
*/

It's a few lines of straightforward validation vs. a comment that requires
readers to verify the assumption chain (vmalloc internals, userspace
contract, P-SEAMLDR behavior). Not sure the trade is worth it.

If you think the comment is sufficient, I'm fine dropping the checks.

>
>I might be confused, but it seems different then we discussed.
>
>> +
>> + params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
>> + if (!params)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /*
>> + * Only use version 1 when required (sigstruct > 4KB) for backward
>> + * compatibility with P-SEAMLDR that lacks version 1 support.
>> + */
>> + if (sig_size > SZ_4K)
>> + params->version = 1;
>> + else
>> + params->version = 0;
>
>I'm a bit confused by this part. What does it mean to support old P-SEAMLDRs?

All current P-SEAMLDRs only support version 0. Version 1 is a planned
extension but not yet implemented by any P-SEAMLDR. Always requiring
version 1 just saves us a conditional but disables updates for all existing
P-SEAMLDRs for no reason. I think it is not worthwhile.

>But also could it be:
>
>params->version = sig_size > SZ_4K;

ok with me. I just wanted to make it match with "version 1" in the
comment right above.

>> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
>> +{
>> + const struct tdx_blob *blob = (const void *)data;
>> + int module_size, sig_size;
>> + const void *sig, *module;
>> +
>> + /*
>> + * Ensure the size is valid otherwise reading any field from the
>> + * blob may overflow.
>> + */
>> + if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (blob->version != TDX_BLOB_VERSION_1)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* Split the blob into a sigstruct and a module. */
>> + sig = blob->data;
>> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
>> + module = data + blob->offset_of_module;
>> + module_size = size - blob->offset_of_module;
>
>Did you consider just passing the tdx_blob into alloc_seamldr_params()?
>Basically, this function checks the blob fields, then alloc_seamldr_params()
>turns blob into struct seamldr_params without checks. The way it is, the work
>seems kind of spread around two functions with various checks.

Fine with merging them. How about:

static struct seamldr_params *alloc_seamldr_params(const u8 *data, u32 size)
{
const struct tdx_blob *blob = (const void *)data;
struct seamldr_params *params;
int module_size, sig_size;
const void *sig, *module;
const u8 *ptr;
int i;

/*
* Ensure the size is valid otherwise reading any field from the
* blob may overflow.
*/
if (size <= sizeof(struct tdx_blob))
return ERR_PTR(-EINVAL);

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

if (blob->length != size)
return ERR_PTR(-EINVAL);

if (memcmp(blob->signature, "TDX-BLOB", 8))
return ERR_PTR(-EINVAL);

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

/* Split the blob into a sigstruct and a module. */
sig = blob->data;
sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
module = data + blob->offset_of_module;
module_size = size - blob->offset_of_module;

if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K || module_size <= 0)
return ERR_PTR(-EINVAL);

if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K || sig_size <= 0)
return ERR_PTR(-EINVAL);

params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
if (!params)
return ERR_PTR(-ENOMEM);

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

ptr = sig;
/*
* Assume sig/module base addresses and sizes are page-aligned.
* If violated, P-SEAMLDR will reject the update.
*/
for (i = 0; i < sig_size / SZ_4K; i++) {
params->sigstruct_pa[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
ptr += SZ_4K;
}

params->num_module_pages = module_size / SZ_4K;

ptr = module;
for (i = 0; i < params->num_module_pages; i++) {
params->mod_pages_pa_list[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
ptr += SZ_4K;
}

return params;
}