[v2,09/14] platform/x86/intel/ifs: Use generic microcode headers and functions

  Existing implementation (broken) of IFS used a header format (for IFS
test images) which was very similar to microcode format, but didn’t
accommodate extended signatures. This meant same IFS test image had to
be duplicated for different steppings and the validation code in the
driver was only looking at the primary header parameters. Going forward
IFS test image headers has been tweaked to become fully compatible with
microcode format.

Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
so as to distinguish it from microcode images and older IFS test images.

In light of the above, use struct microcode_header_intel directly in
IFS driver instead of duplicating into a separate ifs_header structure.
Further use existing microcode_sanity_check() and find_matching_signature()
directly instead of implementing separate ones within the driver.

More IFS specific checks will be added subsequently.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 arch/x86/include/asm/microcode_intel.h |   1 +
 drivers/platform/x86/intel/ifs/load.c  | 102 +++++--------------------
 2 files changed, 20 insertions(+), 83 deletions(-)
  

On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> Existing implementation (broken) of IFS used a header format (for IFS
> test images) which was very similar to microcode format, but didn’t
> accommodate extended signatures. This meant same IFS test image had to
> be duplicated for different steppings and the validation code in the
> driver was only looking at the primary header parameters. Going forward
> IFS test image headers has been tweaked to become fully compatible with
> microcode format.
> 
> Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
> so as to distinguish it from microcode images and older IFS test images.
> 
> In light of the above, use struct microcode_header_intel directly in
> IFS driver instead of duplicating into a separate ifs_header structure.
> Further use existing microcode_sanity_check() and find_matching_signature()
> directly instead of implementing separate ones within the driver.
> 
> More IFS specific checks will be added subsequently.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
  

Hi all,

On 11/7/22 23:53, Jithu Joseph wrote:
> Existing implementation (broken) of IFS used a header format (for IFS
> test images) which was very similar to microcode format, but didn’t
> accommodate extended signatures. This meant same IFS test image had to
> be duplicated for different steppings and the validation code in the
> driver was only looking at the primary header parameters. Going forward
> IFS test image headers has been tweaked to become fully compatible with
> microcode format.
> 
> Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
> so as to distinguish it from microcode images and older IFS test images.
> 
> In light of the above, use struct microcode_header_intel directly in
> IFS driver instead of duplicating into a separate ifs_header structure.
> Further use existing microcode_sanity_check() and find_matching_signature()
> directly instead of implementing separate ones within the driver.
> 
> More IFS specific checks will be added subsequently.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  arch/x86/include/asm/microcode_intel.h |   1 +
>  drivers/platform/x86/intel/ifs/load.c  | 102 +++++--------------------
>  2 files changed, 20 insertions(+), 83 deletions(-)
> 
> diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> index 0ff4545f72d2..f905238748fc 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -43,6 +43,7 @@ struct extended_sigtable {
>  #define EXT_HEADER_SIZE		(sizeof(struct extended_sigtable))
>  #define EXT_SIGNATURE_SIZE	(sizeof(struct extended_signature))
>  #define MICROCODE_HEADER_VER_UCODE	1
> +#define MICROCODE_HEADER_VER_IFS	2
>  
>  #define get_totalsize(mc) \
>  	(((struct microcode_intel *)mc)->hdr.datasize ? \
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 60ba5a057f91..7c0d8602817b 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -8,22 +8,8 @@
>  
>  #include "ifs.h"
>  
> -struct ifs_header {
> -	u32 header_ver;
> -	u32 blob_revision;
> -	u32 date;
> -	u32 processor_sig;
> -	u32 check_sum;
> -	u32 loader_rev;
> -	u32 processor_flags;
> -	u32 metadata_size;
> -	u32 total_size;
> -	u32 fusa_info;
> -	u64 reserved;
> -};
> -
> -#define IFS_HEADER_SIZE	(sizeof(struct ifs_header))
> -static struct ifs_header *ifs_header_ptr;	/* pointer to the ifs image header */
> +#define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
> +static  struct microcode_header_intel *ifs_header_ptr;	/* pointer to the ifs image header */
>  static u64 ifs_hash_ptr;			/* Address of ifs metadata (hash) */
>  static u64 ifs_test_image_ptr;			/* 256B aligned address of test pattern */
>  static DECLARE_COMPLETION(ifs_done);
> @@ -150,33 +136,18 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>   */
>  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);
> +	int curr_pkg, cpu, ret = -ENOMEM;
>  	bool *package_authenticated;
>  	struct ifs_work local_work;
> -	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;
> +	ifsd->loaded_version = ifs_header_ptr->rev;
>  
>  	/* copy the scan hash and authenticate per package */
>  	cpus_read_lock();
> @@ -203,67 +174,33 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	return ret;
>  }
>  
> -static int ifs_sanity_check(struct device *dev,
> -			    const struct microcode_header_intel *mc_header)
> +static int ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
>  {
> -	unsigned long total_size, data_size;
> -	u32 sum, *mc;
> -
> -	total_size = get_totalsize(mc_header);
> -	data_size = get_datasize(mc_header);
> +	struct ucode_cpu_info uci;
>  
> -	if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
> -		dev_err(dev, "bad ifs data file size.\n");
> +	/* Provide a specific error message when loading an older/unsupported image */
> +	if (data->hdrver != MICROCODE_HEADER_VER_IFS) {
> +		dev_err(dev, "Header version %d not supported\n", data->hdrver);
>  		return -EINVAL;
>  	}
>  
> -	if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> -		dev_err(dev, "invalid/unknown ifs update format.\n");
> +	if (intel_microcode_sanity_check((void *)data, true, MICROCODE_HEADER_VER_IFS)) {
> +		dev_err(dev, "sanity check failed\n");
>  		return -EINVAL;
>  	}
>  
> -	mc = (u32 *)mc_header;
> -	sum = 0;
> -	for (int i = 0; i < total_size / sizeof(u32); i++)
> -		sum += mc[i];
> +	intel_cpu_collect_info(&uci);
>  
> -	if (sum) {
> -		dev_err(dev, "bad ifs data checksum, aborting.\n");
> +	if (!intel_find_matching_signature((void *)data,
> +					   uci.cpu_sig.sig,
> +					   uci.cpu_sig.pf)) {
> +		dev_err(dev, "cpu signature, processor flags not matching\n");
>  		return -EINVAL;
>  	}
>  
>  	return 0;
>  }
>  
> -static bool find_ifs_matching_signature(struct device *dev, struct ucode_cpu_info *uci,
> -					const struct microcode_header_intel *shdr)
> -{
> -	unsigned int mc_size;
> -
> -	mc_size = get_totalsize(shdr);
> -
> -	if (!mc_size || ifs_sanity_check(dev, shdr) < 0) {
> -		dev_err(dev, "ifs sanity check failure\n");
> -		return false;
> -	}
> -
> -	if (!intel_cpu_signatures_match(uci->cpu_sig.sig, uci->cpu_sig.pf, shdr->sig, shdr->pf)) {
> -		dev_err(dev, "ifs signature, pf not matching\n");
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
> -static bool ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
> -{
> -	struct ucode_cpu_info uci;
> -
> -	intel_cpu_collect_info(&uci);
> -
> -	return find_ifs_matching_signature(dev, &uci, data);
> -}
> -
>  /*
>   * Load ifs image. Before loading ifs module, the ifs image must be located
>   * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
> @@ -284,12 +221,11 @@ void ifs_load_firmware(struct device *dev)
>  		goto done;
>  	}
>  
> -	if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
> -		dev_err(dev, "ifs header sanity check failed\n");
> +	ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
> +	if (ret)
>  		goto release;
> -	}
>  
> -	ifs_header_ptr = (struct ifs_header *)fw->data;
> +	ifs_header_ptr = (struct microcode_header_intel *)fw->data;
>  	ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
>  
>  	ret = scan_chunks_sanity_check(dev);
  

On Mon, Nov 07, 2022 at 02:53:18PM -0800, Jithu Joseph wrote:
>  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);
> +	int curr_pkg, cpu, ret = -ENOMEM;
>  	bool *package_authenticated;
>  	struct ifs_work local_work;
> -	char *test_ptr;
>  
>  	package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
>  	if (!package_authenticated)
>  		return ret;

Bah, how big is that thing so that you can't simply do a bitfield on the
stack here instead of kcalloc-ing?

> @@ -203,67 +174,33 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	return ret;
>  }
>  
> -static int ifs_sanity_check(struct device *dev,
> -			    const struct microcode_header_intel *mc_header)
> +static int ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)

Yet another static function - no need for the ifs_ prefix.

...
  

On 11/11/2022 8:23 AM, Borislav Petkov wrote:
> On Mon, Nov 07, 2022 at 02:53:18PM -0800, Jithu Joseph wrote:
>>  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);
>> +	int curr_pkg, cpu, ret = -ENOMEM;
>>  	bool *package_authenticated;
>>  	struct ifs_work local_work;
>> -	char *test_ptr;
>>  
>>  	package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
>>  	if (!package_authenticated)
>>  		return ret;
> 
> Bah, how big is that thing so that you can't simply do a bitfield on the
> stack here instead of kcalloc-ing?

Will modify it to use individual bits from a u32 to store package authentication status.

> 
>> @@ -203,67 +174,33 @@ static int scan_chunks_sanity_check(struct device *dev)
>>  	return ret;
>>  }
>>  
>> -static int ifs_sanity_check(struct device *dev,
>> -			    const struct microcode_header_intel *mc_header)
>> +static int ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
> 
> Yet another static function - no need for the ifs_ prefix.

Noted ... will change


Jithu
  

On Fri, Nov 11, 2022 at 05:23:48PM +0100, Borislav Petkov wrote:
> On Mon, Nov 07, 2022 at 02:53:18PM -0800, Jithu Joseph wrote:
> >  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);
> > +	int curr_pkg, cpu, ret = -ENOMEM;
> >  	bool *package_authenticated;
> >  	struct ifs_work local_work;
> > -	char *test_ptr;
> >  
> >  	package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
> >  	if (!package_authenticated)
> >  		return ret;
> 
> Bah, how big is that thing so that you can't simply do a bitfield on the
> stack here instead of kcalloc-ing?

Kernel is built with gcc options that prevent a variable sized local
array. So:

	DECLARE_BITMAP(auth, topology_max_packages());

grumbles:

drivers/platform/x86/intel/ifs/load.c:196:2: warning: ISO C90 forbids variable length array ‘ifs_auth2’ [-Wvla]

We could pick a likely big enough static number:

#define MAX_SUPPORTED_PACKAGES	128

	DECLARE_BITMAP(auth, MAX_SUPPORTED_PACKAGES);

and error out of this code if SGI or someone build a monster machine:

	if (topology_max_packages() > MAX_SUPPORTED_PACKAGES) {
		pr_error_once("IFS driver needs update to support this machine\n");
		return -E2BIG;
	}

That avoids the kcalloc() and making sure to kfree() in all error paths.

But seems a bit hacky.  Other ideas?

-Tony
  

On Wed, Nov 16, 2022 at 09:26:19AM -0800, Tony Luck wrote:
> But seems a bit hacky.  Other ideas?

Yeah, that looks not too optimal.

How about you still allocate but at driver init time, ifs_init() or so?

And deallocate it at driver removal so that its always present during
the driver's lifetime and can be reused each time the sanity check is
called.

And you call ifs_load_firmware() under the ifs_sem so you already got
sync too.

Hmmm.
  

> How about you still allocate but at driver init time, ifs_init() or so?
>
> And deallocate it at driver removal so that its always present during
> the driver's lifetime and can be reused each time the sanity check is
> called.

That works. Now we have multiple files we know that we'll take this
path every time user switches to a new file.

-Tony