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

[Chao Gao <chao.gao@intel.com>]

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

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

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