Re: [PATCH v5 06/10] platform/x86/intel/ifs: Authenticate and copy to secured memory

[Thomas Gleixner]

On Thu, Apr 28 2022 at 08:38, Tony Luck wrote:
> The IFS image contains hashes that will be used to authenticate the ifs
> test chunks. First, use WRMSR to copy the hashes and enumerate the number
> of test chunks, chunk size and the maximum number of cores that can run
> scan test simultaneously.
>
> Next, use WRMSR to authenticate each and every scan test chunk which is
> also stored in the IFS image. The CPU will check if the test chunks match

s/also// ?

> +
> +/* MSR_CHUNKS_AUTH_STATUS bit fields */
> +union ifs_chunks_auth_status {
> +	u64	data;
> +	struct {
> +		u32	valid_chunks	:8;
> +		u32	total_chunks	:8;
> +		u32	rsvd1		:16;
> +		u32	error_code	:8;
> +		u32	rsvd2		:24;
> +	};
> +};
> +
>  /**
>   * struct ifs_data - attributes related to intel IFS driver
>   * @integrity_cap_bit - MSR_INTEGRITY_CAPS bit enumerating this test
> + * @loaded_version: stores the currently loaded ifs image version.
> + * @loaded: If a valid test binary has been loaded into the memory
> + * @loading_error: Error occurred on another CPU while loading image
> + * @valid_chunks: number of chunks which could be validated.
>   */
>  struct ifs_data {
>  	int integrity_cap_bit;
> +	int loaded_version;
> +	bool loaded;
> +	bool loading_error;
> +	int valid_chunks;

The above struct is nicely tabular. Can we have that here too please?

> +/*
> + * IFS requires scan chunks authenticated per each socket in the platform.
> + * Once the test chunk is authenticated, it is automatically copied to secured memory
> + * and proceed the authentication for the next chunk.
> + */
> +static int scan_chunks_sanity_check(struct device *dev)
> +{
> +	int metadata_size, curr_pkg, cpu, ret = -ENOMEM;
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	bool *package_authenticated;
> +	char *test_ptr;
> +
> +	package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
> +	if (!package_authenticated)
> +		return ret;
> +
> +	metadata_size = ifs_header_ptr->metadata_size;
> +
> +	/* Spec says that if the Meta Data Size = 0 then it should be treated as 2000 */
> +	if (metadata_size == 0)
> +		metadata_size = 2000;
> +
> +	/* Scan chunk start must be 256 byte aligned */
> +	if ((metadata_size + IFS_HEADER_SIZE) % 256) {
> +		dev_err(dev, "Scan pattern offset within the binary is not 256 byte aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	test_ptr = (char *)ifs_header_ptr + IFS_HEADER_SIZE + metadata_size;
> +	ifsd->loading_error = false;
> +
> +	ifs_test_image_ptr = (u64)test_ptr;
> +	ifsd->loaded_version = ifs_header_ptr->blob_revision;
> +
> +	/* copy the scan hash and authenticate per package */
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		curr_pkg = topology_physical_package_id(cpu);
> +		if (package_authenticated[curr_pkg])
> +			continue;
> +		package_authenticated[curr_pkg] = 1;

Setting the authenticated indicator _before_ actually doing the
authentication is just wrong. It does not matter in this case, but it's
still making my eyes bleed.

> +		ret = smp_call_function_single(cpu, copy_hashes_authenticate_chunks,
> +					       dev, 1);

Why has this to be a smp function call? Just because it's conveniant?
This is nothing urgent and no hotpath, so this really can use
queue_work_on().

Thanks,

        tglx