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

[Chao Gao]

>> header is consumed solely by the kernel to extract the sigstruct and
>> module, so validate it before processing to protect the kernel ABI. The
>> sigstruct and module are passed to and validated by P-SEAMLDR, so don't
>> duplicate any validation in the kernel.
>> 
>> Note: the sigstruct_pa field in SEAMLDR_PARAMS has been extended to
>> a 4-element array. The updated "SEAM Loader (SEAMLDR) Interface
>> Specification" will be published separately.
>
>These changelogs have all the right info, but I find them really hard to
>parse. For instance, if you're going to have a 'struct seamldr_params',
>then just stick with that name. Don't use the "SEAMLDR_PARAMS" name too.
>
>Start with the data structures:
>
>There are two important ABIs here:
>
>'struct tdx_blob'       - the on-disk and in-memory format for a TDX
> 		          module update image.
>'struct seamldr_params' - The in-memory ABI passed to the TDX module
>			  loader. Points to a single 'struct tdx_blob'

Thanks for the thorough review.

Your comments all make sense to me. I just want to confirm two points
below.


>> +	/*
>> +	 * Don't care about user passing the wrong file, but protect
>> +	 * kernel ABI by preventing accepting garbage.
>> +	 */
>> +	if (memcmp(blob->signature, "TDX-BLOB", 8))
>> +		return ERR_PTR(-EINVAL);
>
>Is there really no helper in the kernel anywhere that can safely do the
>8-byte compare against two known-to-the-compiler 8-byte-wide fields
>without hard-coding the 8?

I couldn't find a helper that automatically derives the comparison
length from the operands.  'strcmp()' is not suitable here because
'blob->signature' is not NUL-terminated.

Do you mean just avoiding the hard-coded 8, e.g.

	if (memcmp(blob->signature, "TDX-BLOB", sizeof(blob->signature)))
		return ERR_PTR(-EINVAL);

or define the 'u8 signature[8]' as a u64 and compare it with a constant, like

/* Little-endian encoding of "TDX-BLOB" string */
#define TDX_IMAGE_SIGNATURE	0x424f4c422d584454ULL

	if (blob->signature != TDX_IMAGE_SIGNATURE)
		return ERR_PTR(-EINVAL);

>> +	struct seamldr_params *params;
>> +	int module_pg_cnt, sig_pg_cnt;
>> +	const u8 *sig, *module;
>> +	int i;
>> +
>> +	params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
>> +	if (!params)
>> +		return ERR_PTR(-ENOMEM);
>
>kzmalloc(PAGE_SIZE, GFP_KERNEL) will save you a cast.

I noticed that 'kzalloc_obj()' can be used here, which avoids spelling out
the size and GFP flags explicitly.  So I ended up with:

    params = kzalloc_obj(*params);

If you would prefer 'kzalloc(PAGE_SIZE, GFP_KERNEL)', I can switch to that.