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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Tue, 2026-04-14 at 17:43 +0800, Chao Gao wrote:
> 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.

Looks good.

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

Hmm, I'm convinced. I think a lot of tip code elides conditionals. I'm not sure
which they would prefer.

I guess the problem I see is that all these conditionals seem like a lot for
something that is mostly just getting passed through. So they need to come
across better somehow.

> 
> > 
> > > +
> > > +	/*
> > > +	 * 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.

Seems like we could drop it then.

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

Haha, as impressed as I am with AI code review... are we already at the point of
adding comments to help the AI not get confused?

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

The checks seem like too much. Any ideas on how to contain it more? It seems
like you are making the case for each one. But if we delete them all... does it
seem like a better tradeoff?

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

Sorry, I still don't understand. Why does the kernel need to set this bit? It's
an opt-in for a format change? How many old TDX modules are we talking?

I think, connecting to the discussion in the later patches, that we should try
to keep the initial implementation as small as possible. Everything we add makes
things take a little bit longer. And while we spin new versions the kernel has
zero support. So I see that this is another non-critical enhancement.

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

I wasn't suggesting to merge them. I was suggesting to have them each do a
dedicated thing.

> 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;
> }