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

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

>> +	ptr = sig;
>> +	for (i = 0; i < sig_size / SZ_4K; i++) {
>> +		/*
>> +		 * Don't assume @sig is page-aligned although it is 4KB-aligned.
>> +		 * Always add the in-page offset to get the physical address.
>> +		 */
>
>I don't follow this. If @sig is 4k aligned in VA, it is page aligned.

Dan's concern was that PAGE_SIZE is not guaranteed to be 4096.

I agree that PAGE_SIZE is 4K on x86 today. But to address that concern, I saw
two options:

  1. Add WARN_ON_ONCE(PAGE_SIZE != SZ_4K), or
  2. Handle it as in the code above.

I didn't find existing code using option 1 in x86, so I chose option 2.

>
>If you want to handle case when @sig is not 4k aligned, than this is
>broken. You need to bump ptr to the next 4k boundary, not by 4k.

@sig is 4KB aligned.

<snip>

>> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
>> +{
>> +	const struct tdx_blob *blob = (const void *)data;
>> +	int module_size, sig_size;
>> +	const void *sig, *module;
>> +
>> +	/* Ensure the size is valid otherwise reading any field from the blob may overflow. */
>> +	if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (blob->version != TDX_BLOB_VERSION_1) {
>> +		pr_err("unsupported blob version: %x\n", blob->version);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	/* Split the blob into a sigstruct and a module. */
>> +	sig		= blob->data;
>> +	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);
>> +	module		= data + blob->offset_of_module;
>> +	module_size	= size - blob->offset_of_module;
>> +
>> +	if (sig_size <= 0 || module_size <= 0 || blob->length != size)
>> +		return ERR_PTR(-EINVAL);
>
>Maybe add a comment somewhere that block->offset_of_module is relative
>to start of struct tdx_blob, not blob->data and blob->length includes
>length of struct tdx_blob.
>
>It can be either way and it is better to give a reader a hint.

Sure. Will do.