[v2,10/14] platform/x86/intel/ifs: Add metadata validation

  The data portion of IFS test image file contains a metadata
region containing possibly multiple metadata structures in
addition to test data and hashes.

Introduce the layout of this meta_data structure and validate
the sanity of certain fields of the new image before loading.

Tweak references to IFS test image chunks to reflect the updated
layout of the test image.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h  |  2 +
 drivers/platform/x86/intel/ifs/load.c | 53 +++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)
  

Jithu, I replied to the prior version of the series by mistake. 
Duplicating that comment over here.

On 11/7/2022 2:53 PM, Jithu Joseph wrote:
> The data portion of IFS test image file contains a metadata
> region containing possibly multiple metadata structures in
> addition to test data and hashes.
> 
> Introduce the layout of this meta_data structure and validate
> the sanity of certain fields of the new image before loading.
> 
> Tweak references to IFS test image chunks to reflect the updated
> layout of the test image.
> 

Can you provide some more information on the updated layout of the 
metadata structure?

Some parts of validate_ifs_metadata() like the ifs_test_image_ptr 
calculation would be easier to follow with that.
  

On 11/9/2022 3:15 PM, Sohil Mehta wrote:
> Jithu, I replied to the prior version of the series by mistake. Duplicating that comment over here.
> 
> On 11/7/2022 2:53 PM, Jithu Joseph wrote:
>> The data portion of IFS test image file contains a metadata
>> region containing possibly multiple metadata structures in
>> addition to test data and hashes.
>>
>> Introduce the layout of this meta_data structure and validate
>> the sanity of certain fields of the new image before loading.
>>
>> Tweak references to IFS test image chunks to reflect the updated
>> layout of the test image.
>>
> 
> Can you provide some more information on the updated layout of the metadata structure?
> 

Here is the layout of the metadata section in an IFS test image.
Test Data (chunks) follow the struct meta_data  defined in load.c

    IFS Metadata layout
   +----------------------+  0
   |META_TYPE_IFS (=1)    |
   +----------------------+
   |meta_size             |
   +----------------------+
   |test type             |
   +----------------------+
   |fusa info             |
   +----------------------+
   |total images          |
   +----------------------+
   |current image#        |
   +----------------------+
   |total chunks          |
   +----------------------+
   |starting chunk        |
   +----------------------+
   |size per chunk        |
   +----------------------+
   |chunks per stride     |
   +----------------------+
   |Reserved[54]          |
   +----------------------+ 256
   |                      |
   |                      |
   |                      |
   |                      |
   |Test Data/Chunks      |
   |                      |
   |                      |
   |                      |
   |                      |
   +----------------------+  meta_size
   |   META_TYPE_END (=0) |
   +----------------------+  meta_size + 4
   |   size of end (=8)   |
   |                      |
   +----------------------+  meta_size + 8


Jithu
  

On 11/9/2022 5:22 PM, Joseph, Jithu wrote:

> Here is the layout of the metadata section in an IFS test image.
> Test Data (chunks) follow the struct meta_data  defined in load.c
> 

Thanks, this helps clarify things around the test data placement in this 
patch. I think it would be useful to include the below information in 
the commit message as well.

Given that,

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

>      IFS Metadata layout
>     +----------------------+  0
>     |META_TYPE_IFS (=1)    |
>     +----------------------+
>     |meta_size             |
>     +----------------------+
>     |test type             |
>     +----------------------+
>     |fusa info             |
>     +----------------------+
>     |total images          |
>     +----------------------+
>     |current image#        |
>     +----------------------+
>     |total chunks          |
>     +----------------------+
>     |starting chunk        |
>     +----------------------+
>     |size per chunk        |
>     +----------------------+
>     |chunks per stride     |
>     +----------------------+
>     |Reserved[54]          |
>     +----------------------+ 256
>     |                      |
>     |                      |
>     |                      |
>     |                      |
>     |Test Data/Chunks      |
>     |                      |
>     |                      |
>     |                      |
>     |                      |
>     +----------------------+  meta_size
>     |   META_TYPE_END (=0) |
>     +----------------------+  meta_size + 4
>     |   size of end (=8)   |
>     |                      |
>     +----------------------+  meta_size + 8
> 
> 
> Jithu
  

Hi,

On 11/7/22 23:53, Jithu Joseph wrote:
> The data portion of IFS test image file contains a metadata
> region containing possibly multiple metadata structures in
> addition to test data and hashes.
> 
> Introduce the layout of this meta_data structure and validate
> the sanity of certain fields of the new image before loading.
> 
> Tweak references to IFS test image chunks to reflect the updated
> layout of the test image.
> 
> 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



> ---
>  drivers/platform/x86/intel/ifs/ifs.h  |  2 +
>  drivers/platform/x86/intel/ifs/load.c | 53 +++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 3ff1d9aaeaa9..98ca91bdd5ca 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -196,6 +196,7 @@ union ifs_status {
>   * @valid_chunks: number of chunks which could be validated.
>   * @status: it holds simple status pass/fail/untested
>   * @scan_details: opaque scan status code from h/w
> + * @cur_batch: number indicating the currently loaded test file
>   */
>  struct ifs_data {
>  	int	integrity_cap_bit;
> @@ -205,6 +206,7 @@ struct ifs_data {
>  	int	valid_chunks;
>  	int	status;
>  	u64	scan_details;
> +	int	cur_batch;
>  };
>  
>  struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 7c0d8602817b..f361fd42a320 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -8,7 +8,23 @@
>  
>  #include "ifs.h"
>  
> +struct meta_data {
> +	unsigned int meta_type;		// metadata type
> +	unsigned int meta_size;		// size of this entire struct including hdrs.
> +	unsigned int test_type;		// IFS test type
> +	unsigned int fusa_info;		// Fusa info
> +	unsigned int total_images;	// Total number of images
> +	unsigned int current_image;	// Current Image #
> +	unsigned int total_chunks;	// Total number of chunks in this image
> +	unsigned int starting_chunk;	// Starting chunk number in this image
> +	unsigned int size_per_chunk;	// size of each chunk
> +	unsigned int chunks_per_stride;	// number of chunks in a stride
> +	unsigned int reserved[54];	// Align to 256 bytes for chunk alignment.
> +};
> +
>  #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
> +#define META_TYPE_IFS	1
> +#define IFS_CHUNK_ALIGNMENT	256
>  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 */
> @@ -129,6 +145,40 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  	complete(&ifs_done);
>  }
>  
> +static int validate_ifs_metadata(struct device *dev)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	struct meta_data *ifs_meta;
> +	char test_file[64];
> +	int ret = -EINVAL;
> +
> +	snprintf(test_file, sizeof(test_file), "%02x-%02x-%02x-%02x.scan",
> +		 boot_cpu_data.x86, boot_cpu_data.x86_model,
> +		 boot_cpu_data.x86_stepping, ifsd->cur_batch);
> +
> +	ifs_meta = (struct meta_data *)ifs_find_meta_data(ifs_header_ptr, META_TYPE_IFS);
> +	if (!ifs_meta) {
> +		dev_err(dev, "IFS Metadata missing in file %s\n", test_file);
> +		return ret;
> +	}
> +
> +	ifs_test_image_ptr = (u64)ifs_meta + sizeof(struct meta_data);
> +
> +	/* Scan chunk start must be 256 byte aligned */
> +	if (!IS_ALIGNED(ifs_test_image_ptr, IFS_CHUNK_ALIGNMENT)) {
> +		dev_err(dev, "Scan pattern offset is not 256 byte aligned in %s\n", test_file);
> +		return ret;
> +	}
> +
> +	if (ifs_meta->current_image != ifsd->cur_batch) {
> +		dev_warn(dev, "Mismatch between filename %s and batch metadata 0x%02x\n",
> +			 test_file, ifs_meta->current_image);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * IFS requires scan chunks authenticated per each socket in the platform.
>   * Once the test chunk is authenticated, it is automatically copied to secured memory
> @@ -145,6 +195,9 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	if (!package_authenticated)
>  		return ret;
>  
> +	ret = validate_ifs_metadata(dev);
> +	if (ret)
> +		return ret;
>  
>  	ifsd->loading_error = false;
>  	ifsd->loaded_version = ifs_header_ptr->rev;
  

On Mon, Nov 07, 2022 at 02:53:19PM -0800, Jithu Joseph wrote:
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 7c0d8602817b..f361fd42a320 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -8,7 +8,23 @@
>  
>  #include "ifs.h"
>  
> +struct meta_data {
> +	unsigned int meta_type;		// metadata type
> +	unsigned int meta_size;		// size of this entire struct including hdrs.
> +	unsigned int test_type;		// IFS test type
> +	unsigned int fusa_info;		// Fusa info
> +	unsigned int total_images;	// Total number of images
> +	unsigned int current_image;	// Current Image #
> +	unsigned int total_chunks;	// Total number of chunks in this image
> +	unsigned int starting_chunk;	// Starting chunk number in this image
> +	unsigned int size_per_chunk;	// size of each chunk
> +	unsigned int chunks_per_stride;	// number of chunks in a stride
> +	unsigned int reserved[54];	// Align to 256 bytes for chunk alignment.

That looks weird.

__packed and __aligned doesn't work?

>  #define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
> +#define META_TYPE_IFS	1
> +#define IFS_CHUNK_ALIGNMENT	256
>  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 */
> @@ -129,6 +145,40 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
>  	complete(&ifs_done);
>  }
>  
> +static int validate_ifs_metadata(struct device *dev)
> +{
> +	struct ifs_data *ifsd = ifs_get_data(dev);
> +	struct meta_data *ifs_meta;
> +	char test_file[64];
> +	int ret = -EINVAL;
> +
> +	snprintf(test_file, sizeof(test_file), "%02x-%02x-%02x-%02x.scan",
> +		 boot_cpu_data.x86, boot_cpu_data.x86_model,
> +		 boot_cpu_data.x86_stepping, ifsd->cur_batch);
> +
> +	ifs_meta = (struct meta_data *)ifs_find_meta_data(ifs_header_ptr, META_TYPE_IFS);
> +	if (!ifs_meta) {
> +		dev_err(dev, "IFS Metadata missing in file %s\n", test_file);
> +		return ret;
> +	}
> +
> +	ifs_test_image_ptr = (u64)ifs_meta + sizeof(struct meta_data);
> +
> +	/* Scan chunk start must be 256 byte aligned */
> +	if (!IS_ALIGNED(ifs_test_image_ptr, IFS_CHUNK_ALIGNMENT)) {
> +		dev_err(dev, "Scan pattern offset is not 256 byte aligned in %s\n", test_file);

	" ... is not aligned on %d bytes...\n", test_file, IFS_CHUNK_ALIGNMENT);

> +		return ret;
> +	}
> +
> +	if (ifs_meta->current_image != ifsd->cur_batch) {
> +		dev_warn(dev, "Mismatch between filename %s and batch metadata 0x%02x\n",
> +			 test_file, ifs_meta->current_image);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * IFS requires scan chunks authenticated per each socket in the platform.
>   * Once the test chunk is authenticated, it is automatically copied to secured memory
> @@ -145,6 +195,9 @@ static int scan_chunks_sanity_check(struct device *dev)
>  	if (!package_authenticated)
>  		return ret;
>  
> +	ret = validate_ifs_metadata(dev);
> +	if (ret)
> +		return ret;

Right, if you return here because validate_ifs_metadata() failed for
whatever reason, you leak package_authenticated. That's why, as I said
earlier, you should just use a stack variable where you simply set a
bit. A bitmask or so.
  

On 11/11/22 10:39, Borislav Petkov wrote:
>> +struct meta_data {
>> +	unsigned int meta_type;		// metadata type
>> +	unsigned int meta_size;		// size of this entire struct including hdrs.
>> +	unsigned int test_type;		// IFS test type
>> +	unsigned int fusa_info;		// Fusa info
>> +	unsigned int total_images;	// Total number of images
>> +	unsigned int current_image;	// Current Image #
>> +	unsigned int total_chunks;	// Total number of chunks in this image
>> +	unsigned int starting_chunk;	// Starting chunk number in this image
>> +	unsigned int size_per_chunk;	// size of each chunk
>> +	unsigned int chunks_per_stride;	// number of chunks in a stride
>> +	unsigned int reserved[54];	// Align to 256 bytes for chunk alignment.
> That looks weird.
> 
> __packed and __aligned doesn't work?

... and don't we try to use fixed-size typed in hardware structures,
like u32?

There are also much nicer ways to do this:

union meta_data {
	struct {
		u32 meta_type;		// metadata type
		u32 meta_size;		// size of ...
	};
	u8 padding[IFS_CHUNK_ALIGNMENT];
}

That doesn't have any magic linkage between the magic "54" (times 4) and
IFS_CHUNK_ALIGNMENT.  It makes the compiler do the hard work for you.

Voila, you have a union that's always IFS_CHUNK_ALIGNMENT in size,  No
magic 54's necessary.
  

On 11/11/2022 10:48 AM, Dave Hansen wrote:
> On 11/11/22 10:39, Borislav Petkov wrote:
>>> +struct meta_data {
>>> +	unsigned int meta_type;		// metadata type
>>> +	unsigned int meta_size;		// size of this entire struct including hdrs.
>>> +	unsigned int test_type;		// IFS test type
>>> +	unsigned int fusa_info;		// Fusa info
>>> +	unsigned int total_images;	// Total number of images
>>> +	unsigned int current_image;	// Current Image #
>>> +	unsigned int total_chunks;	// Total number of chunks in this image
>>> +	unsigned int starting_chunk;	// Starting chunk number in this image
>>> +	unsigned int size_per_chunk;	// size of each chunk
>>> +	unsigned int chunks_per_stride;	// number of chunks in a stride
>>> +	unsigned int reserved[54];	// Align to 256 bytes for chunk alignment.
>> That looks weird.
>>
>> __packed and __aligned doesn't work?

The struct was mirroring the initial 256 bytes of the metadata section of IFS test image (replicated below). 
I will change it to union with a separate padding member as Dave suggests below.

    IFS Metadata layout
   +----------------------+  0
   |META_TYPE_IFS (=1)    |
   +----------------------+
   |meta_size             |
   +----------------------+
   |test type             |
   +----------------------+
   |fusa info             |
   +----------------------+
   |total images          |
   +----------------------+
   |current image#        |
   +----------------------+
   |total chunks          |
   +----------------------+
   |starting chunk        |
   +----------------------+
   |size per chunk        |
   +----------------------+
   |chunks per stride     |
   +----------------------+
   |Reserved[54]          |
   +----------------------+ 256
   |                      |
   |                      |
   |                      |
   |                      |
   |Test Data/Chunks      |
   |                      |
   |                      |
   |                      |
   |                      |
   +----------------------+  meta_size
   |   META_TYPE_END (=0) |
   +----------------------+  meta_size + 4
   |   size of end (=8)   |
   |                      |
   +----------------------+  meta_size + 8



> 
> ... and don't we try to use fixed-size typed in hardware structures,
> like u32?
> 
> There are also much nicer ways to do this:
> 
> union meta_data {
> 	struct {
> 		u32 meta_type;		// metadata type
> 		u32 meta_size;		// size of ...
> 	};
> 	u8 padding[IFS_CHUNK_ALIGNMENT];
> }
> 
> That doesn't have any magic linkage between the magic "54" (times 4) and
> IFS_CHUNK_ALIGNMENT.  It makes the compiler do the hard work for you.
> 
> Voila, you have a union that's always IFS_CHUNK_ALIGNMENT in size,  No
> magic 54's necessary.

Thanks, will adopt this.

Jithu
  

On Fri, Nov 11, 2022 at 10:48:49AM -0800, Dave Hansen wrote:
> On 11/11/22 10:39, Borislav Petkov wrote:
> >> +struct meta_data {
> >> +	unsigned int meta_type;		// metadata type
> >> +	unsigned int meta_size;		// size of this entire struct including hdrs.
> >> +	unsigned int test_type;		// IFS test type
> >> +	unsigned int fusa_info;		// Fusa info
> >> +	unsigned int total_images;	// Total number of images
> >> +	unsigned int current_image;	// Current Image #
> >> +	unsigned int total_chunks;	// Total number of chunks in this image
> >> +	unsigned int starting_chunk;	// Starting chunk number in this image
> >> +	unsigned int size_per_chunk;	// size of each chunk
> >> +	unsigned int chunks_per_stride;	// number of chunks in a stride
> >> +	unsigned int reserved[54];	// Align to 256 bytes for chunk alignment.
> > That looks weird.
> > 
> > __packed and __aligned doesn't work?
> 
> ... and don't we try to use fixed-size typed in hardware structures,
> like u32?

Maybe should we do a cleanup patch to move struct microcode_header_intel
also to u32 format then add meta_data to follow the same style?

When we defined struct meta_data followed the format and style that was
already in place.

The suggestion below is a lot nicer, let the compiler do te job.

> 
> There are also much nicer ways to do this:
> 
> union meta_data {
> 	struct {
> 		u32 meta_type;		// metadata type
> 		u32 meta_size;		// size of ...
> 	};
> 	u8 padding[IFS_CHUNK_ALIGNMENT];
> }
> 
> That doesn't have any magic linkage between the magic "54" (times 4) and
> IFS_CHUNK_ALIGNMENT.  It makes the compiler do the hard work for you.
> 
> Voila, you have a union that's always IFS_CHUNK_ALIGNMENT in size,  No
> magic 54's necessary.