| Message ID | 20221107225323.2733518-11-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | IFS multi test image support and misc changes | expand |
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.
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;