Re: [RFC PATCH v1 1/3] x86/virt/seamldr: Allocate 'struct seamldr_params' after image validation

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

On Tue, 2026-11-03 at 14:30 +0800, Chao Gao wrote:
> On Tue, Nov 03, 2026 at 07:58:06AM +0800, Edgecombe, Rick P wrote:
> > On Fri, 2026-10-30 at 01:52 -0700, Chao Gao wrote:
> > > 'struct seamldr_params' is the in-memory ABI the kernel passes to P-
> > > SEAMLDR
> > > to update the TDX module. It has a fixed size and is allocated before the
> > > update image is validated.
> > > 
> > > To support larger TDX modules, the ABI was extended to let
> > > 'struct seamldr_params' link to extra pages. The number of extra pages
> > > depends on the module page count, which can be trusted only after the
> > > image
> > > header is validated.
> > > 
> > > Move the allocation after the image validation in init_seamldr_params(),
> > > so a later change can allocate 'struct seamldr_params' and the extra
> > > pages as one buffer. Rename the function to build_seamldr_params() since
> > > it now allocates and returns a buffer rather than filling in one the
> > > caller provides.
> > > 
> > > No functional change intended.
> > 
> > So we want to have the allocation happen after header->module_nr_pages is
> > verified so we don't allocate some ridiculous size. And the verification
> > currently happens all in one place. So move the allocation.
> > 
> > Did you look at just moving the one header field check earlier? Wondering if
> > it
> > requires less churn.
> 
> I didn't. It works too, and we can drop this patch by doing the check in
> alloc_seamldr_params() (added in patch 2) instead:
> 
> static struct seamldr_params *alloc_seamldr_params(u32 nr_module_pages)
> {
> 	struct seamldr_params *params;
> 	u32 nr_chain_pages;
> 	u32 nr_direct;
> 	u64 *link;
> 
> +	if (nr_module_pages > SEAMLDR_MAX_NR_MODULE_PAGES)
> +		return ERR_PTR(-EINVAL);
> +
> 	/*
> 	 * Number of module pages that fit directly in
> 	 * module_pages_pa_list[]. Minus one as the last entry is
> 	 * reserved to point to the first chain page.
> 	 */
> 	nr_direct = SEAMLDR_NR_MODULE_PA_ENTRIES - 1;
> 
> My concern is that this spreads the header checks across two functions.

Yea.

> The churn is a one-time cost, while keeping all the checks in one place
> helps readability for the long term. That said, I am fine with going
> with less churn if you prefer.
> 

My concern is not just the churn, but the resulting code. What used to be a
clearer separation of responsibilities of the functions is now a bit muddled. I
think it could take some playing around to convince that this is all needed,
especially when this series has been pre-sold as "a quick fix".

> > 
> > Hmm, also, this is an admin only thing right? What goes wrong if the admin
> > user
> > passes a huge value?
> 
> Right, it is admin only, and nothing too bad happens. A u32
> module_nr_pages can ask for about 32GB of chain pages. Anything above
> KMALLOC_MAX_SIZE (4MB) hits the WARN_ON_ONCE() in the page allocator,
> taints the kernel and fails the update with -ENOMEM.
> 
> So it is not a serious problem. But relying on "it does not hurt" has a
> cost of its own. Every reader has to work out why the unvalidated value
> is safe to use. Using the value only after it is validated is more
> straightforward.

Hmm. We don't want to have a warning. We could pass __GFP_NOWARN and add a
comment? It still uses the field before validation though, which looks wrong.

I think this looks too much like hammering the the code to make space to jam
something in. We need to make it more naturally correct looking. Maybe it's that
alloc_seamldr_params() returns NULL on ENONMEM, and build_seamldr_params()
returns ERR_PTR(). Or that the verification of the user input no longer seems
district from the other operations.

> 
> > 
> > In any case, I think the log could be simpler and more plain while still
> > explaining what is happening better.
> 
> If we keep this patch, how about:
> 
>   'struct seamldr_params' is the in-memory ABI passed to P-SEAMLDR to
>   update the TDX module. It has a fixed size and is allocated before
>   the update image header is validated.
> 
>   A later change will append a variable number of pages to the
>   structure, sized by header->module_nr_pages. The allocation then
>   needs to come after that field is checked, so that a bogus header
>   cannot make the kernel attempt a huge allocation.
> 
>   All header checks are in one place. Move the allocation after them
>   instead of splitting the module_nr_pages check out.
> 
>   Rename init_seamldr_params() to build_seamldr_params() since it now
>   allocates and returns the structure rather than filling in one the
>   caller provides.
> 
>   No functional change intended.