Re: [PATCH v9 11/23] x86/virt/seamldr: Allocate and populate a module update request

[Dave Hansen]

On 5/18/26 07:15, Chao Gao wrote:
>>> +#define TDX_IMAGE_VERSION_2		0x200
>>> +
>>> +struct tdx_image_header {
>>> +	u16	version; // This ABI is always 0x200
>>
>> That comment reads strangely in here. Did I ask you to write that?
> 
> I copied that from the example you suggested for this structure in v8. But
> yes, it does read awkwardly here, so I will drop it.

The point is that the code or comments needs to mention *somewhere* that
"This (header->version) ABI is always 0x200". I didn't mean for you to
literally put that ^ in there.

It's fine if it is in the code to. Just mention it somewhere.

>>> +static int init_seamldr_params(struct seamldr_params *params, const u8 *data, u32 size)
>>> +{
>>> +	const struct tdx_image *image		= (const void *)data;
>>> +	const struct tdx_image_header *header	= &image->header;
>>> +
>>> +	u32 sigstruct_len	= header->sigstruct_nr_pages * PAGE_SIZE;
>>> +	u32 module_len		= header->module_nr_pages * PAGE_SIZE;
>>> +
>>> +	u8 *header_start	= (u8 *)header;
>>> +	u8 *header_end		= header_start + HEADER_SIZE;
>>> +
>>> +	u8 *sigstruct_start	= header_end;
>>> +	u8 *sigstruct_end	= sigstruct_start + sigstruct_len;
>>> +
>>> +	u8 *module_start	= sigstruct_end;
>>> +
>>> +	/* Check the calculated payload size against the data size. */
>>> +	if (HEADER_SIZE + sigstruct_len + module_len != size)
>>> +		return -EINVAL;
>>> +
>>> +	/*
>>> +	 * Don't care about user passing the wrong file, but protect
>>> +	 * kernel ABI by preventing accepting garbage.
>>> +	 */
>>
>> How does this "protect kernel ABI"?
> 
> "Protect kernel ABI" was imprecise here. The intent is to reject
> obviously malformed headers.
> 
> If the kernel accepts garbage in header fields today, userspace can come
> to rely on that behavior. Later, the kernel may start validating those
> fields more strictly or assign meaning to fields that were previously
> reserved. Rejecting the same image then could be seen as a kernel
> regression.
> 
> I'll simplify the comment to:
> 
> 	/* Reject obviously malformed image headers. */

I'm still not following this at all.

I suspect that someone along the way said something in reviewing this
about ensuring that fields that are "reserved" are treated as "reserved
+ must be zero".

Somehow that recommendation got conflated with the version checking.

But the deeper point is that neither this patch nor its contributor is
quite able to articulate the reason for this line of code being here.

Let me try.

	The "tdx_image" ABI is versioned. However, there has only ever
	been one public versions of the structure: ->version==0x200. The
	kernel can only parse that version. Future versions of the
	module might be able to use the same ABIs (user/kernel and
	kernel/SEAMLDR) but they will not be able to use this kernel
	code.

	Reject module images without that specific version. This ensures
	that the kernel is able to understand the passed-in format.