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