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

[Chao Gao]

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

>
>> +	u16	checksum;
>> +	u8	signature[8];
>> +	u32	sigstruct_nr_pages;
>> +	u32	module_nr_pages;
>> +	u8	reserved[4076];
>> +} __packed;
>> +
>> +#define HEADER_SIZE sizeof(struct tdx_image_header)
>> +static_assert(HEADER_SIZE == 4096);
>> +
>> +/* Intel TDX module update ABI structure. aka. "TDX module blob". */
>> +struct tdx_image {
>> +	struct tdx_image_header header;
>> +	u8 payload[]; // Contains sigstruct pages followed by module pages
>> +};
>> +
>> +static void populate_pa_list(u64 *pa_list, u32 max_entries, const u8 *start, u32 nr_pages)
>
>The naming in there is painful. How about:
>
>populate_pa_list(u64 *pa_list, u32 pa_list_len,
>		 const u8 *vmalloc_addr, u32 vmalloc_len_pages)

Sure.

>
>> +{
>> +	int i;
>> +
>> +	nr_pages = MIN(nr_pages, max_entries);
>
>This seems wonky. Should it really be silently suppressing things if
>either the allocation or source is too small? I get not wanting to
>overflow, but this seems strange.

Ok. I'll add explicit bounds checks and drop the MIN().

>
>> +	for (i = 0; i < nr_pages; i++) {
>> +		pa_list[i] = vmalloc_to_pfn(start) << PAGE_SHIFT;
>> +		start += PAGE_SIZE;
>> +	}
>
>At the point that you modify 'start', it's not 'start' any more. Use
>another variable. This would do, for instance:
>
>	for (i = 0; i < nr_pages; i++) {
>		unsigned long offset = i * PAGE_SIZE;
>
>		pa_list[i] = vmalloc_to_pfn(&start[offset]);
>	}

Good point.

>
>
>> +static void populate_seamldr_params(struct seamldr_params *params,
>> +				    const u8 *sig, u32 sig_nr_pages,
>> +				    const u8 *mod, u32 mod_nr_pages)
>> +{
>> +	params->version			= 0;
>> +	params->scenario		= SEAMLDR_SCENARIO_UPDATE;
>> +	params->module_nr_pages		= mod_nr_pages;
>> +
>> +	populate_pa_list(params->sigstruct_pages_pa_list, SEAMLDR_MAX_NR_SIG_PAGES,
>> +			 sig, sig_nr_pages);
>> +	populate_pa_list(params->module_pages_pa_list, SEAMLDR_MAX_NR_MODULE_PAGES,
>> +			 mod, mod_nr_pages);
>> +}
>
>Yes, this is starting to look OK. Nit: vertically align the "*_PAGES" args:
>
>
>	populate_pa_list(params->sigstruct_pages_pa_list, SEAMLDR_...,
>			 sig, sig_nr_pages);
>	populate_pa_list(params->module_pages_pa_list,    SEAMLDR_...,
>			 mod, mod_nr_pages);
>
>
>> +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. */