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

[Chao Gao]

>> > > +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)
>> > > +		return ERR_PTR(-EINVAL);
>> > > +
>> > > +	if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
>> > > +		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;
>> > 
>> > Did you consider just passing the tdx_blob into alloc_seamldr_params()?
>> > Basically, this function checks the blob fields, then alloc_seamldr_params()
>> > turns blob into  struct seamldr_params without checks. The way it is, the work
>> > seems kind of spread around two functions with various checks.
>> 
>> Fine with merging them. 
>> 
>
>I wasn't suggesting to merge them. I was suggesting to have them each do a
>dedicated thing.

Ok. See the code snippet below. Most checks are in init_seamldr_params(),
except the limit checks on module_size and sig_size. Moving them there would
require duplicating the module_size/sig_size calculations. So, I just keep the
checks next to the calculations.

static struct seamldr_params *alloc_seamldr_params(const struct tdx_blob *blob)
{
	struct seamldr_params *params;
	int module_size, sig_size;
	const void *sig, *module;
	const u8 *ptr;
	int i;

	/* Split the blob into a sigstruct and a module. */
	sig		= blob->data;
	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);
	module		= (u8 *)blob + blob->offset_of_module;
	module_size	= blob->length - blob->offset_of_module;

	if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
		return ERR_PTR(-EINVAL);

	if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K)
		return ERR_PTR(-EINVAL);

	params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
	if (!params)
		return ERR_PTR(-ENOMEM);

	/*
	 * Only use version 1 when required (sigstruct > 4KB) for backward
	 * compatibility with P-SEAMLDR that lacks version 1 support.
	 */
	params->version = sig_size > SZ_4K;
	params->scenario = SEAMLDR_SCENARIO_UPDATE;

	ptr = sig;
	for (i = 0; i < sig_size / SZ_4K; i++) {
		params->sigstruct_pa[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
		ptr += SZ_4K;
	}

	params->num_module_pages = module_size / SZ_4K;

	ptr = module;
	for (i = 0; i < params->num_module_pages; i++) {
		params->mod_pages_pa_list[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
		ptr += SZ_4K;
	}

	return params;
}

static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
{
	const struct tdx_blob *blob = (const void *)data;

	/*
	 * Ensure the size is valid otherwise reading any field from the
	 * blob may overflow.
	 */
	if (size <= sizeof(struct tdx_blob))
		return ERR_PTR(-EINVAL);

	if (blob->version != TDX_BLOB_VERSION_1)
		return ERR_PTR(-EINVAL);

	if (blob->length != size)
		return ERR_PTR(-EINVAL);

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

	if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
		return ERR_PTR(-EINVAL);

	/* Ensure the offset_of_module is within valid range and aligned. */
	if (blob->offset_of_module >= size ||
	    blob->offset_of_module <= sizeof(struct tdx_blob))
		return ERR_PTR(-EINVAL);

	if (!IS_ALIGNED(blob->offset_of_module, SZ_4K))
		return ERR_PTR(-EINVAL);

	return alloc_seamldr_params(blob);
}