| Message ID | 20221021203413.1220137-11-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | IFS multi test image support and misc changes | expand |
On 10/21/2022 1:34 PM, Jithu Joseph wrote: > The data portion of IFS test image file contains a meta-data Can we be consistent with meta-data/metadata usage? Multiple patches have this dual usage. The popular usage in the kernel seems to be "metadata". I would suggest: s/meta-data/metadata > structure 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. > s/new-image/new image > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index be37512535f2..bb43fd65d2d2 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: suffix indicating the currently loaded test file What does "suffix" refer to here? I feel how you derive the current batch information shouldn't really matter. > */ > 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; > }; > ... > #define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel)) > #define IFS_HEADER_VER 2 > +#define META_TYPE_IFS 1 What namespace does this meta type belong to? Is the expectation here that IFS will have different meta types? Or in the generic microcode header IFS meta can be found using this type? I am asking since microcode_intel_find_meta_data() would be eventually called from other non-ifs places as well. Can you please point me to the architecture documentation that describes this? > +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); > + There are multiple usages of ifsd->cur_batch in this patch. AFAIU, the variable is still uninitialized. Would this validation patch make more sense after patch 12? The "cur_batch" terminology is introduced there and the initialization happens there as well. > + > + if (ifs_meta->current_image != ifsd->cur_batch) { > + dev_warn(dev, "Suffix metadata is not matching with filename %s(0x%02x)\n", What does "suffix metadata" mean? How about: Currently loaded filename %s(0x%02x) doesn't match with the information in the metadata. Sohil
On 10/21/2022 1:34 PM, Jithu Joseph wrote: > The data portion of IFS test image file contains a meta-data > structure 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.
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index be37512535f2..bb43fd65d2d2 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: suffix 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 3cb13a7aa74b..d300cf3ce5bd 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -8,8 +8,24 @@ #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 IFS_HEADER_VER 2 +#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 */ @@ -98,6 +114,41 @@ 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 *)microcode_intel_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, "Suffix metadata is not matching with filename %s(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 @@ -114,6 +165,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;