| Message ID | 20221117195957.28225-1-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | None | expand |
Hi, On 11/17/22 20:59, Jithu Joseph wrote: > IFS requires tests to be authenticated once for each CPU socket > on a system. > > scan_chunks_sanity_check() was dynamically allocating memory > to store the state of whether tests have been authenticated on > each socket for every load operation. > > Move the memory allocation to init path and store the pointer > in ifs_data struct. > > Also rearrange the adjacent error checking in init for a > more simplified and natural flow. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Suggested-by: Borislav Petkov <bp@alien8.de> > Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> > --- > - Replaced global pkg_auth pointer to struct ifs_data (Hans) > - Rearrange the adjacent error checking flow in ifs_init (Hans) > - With this change there are conflicts in patches 11 and 12 (I will > post the updated 11 and 12 if this is satisfactory) Thanks, this patch looks good to me now: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > drivers/platform/x86/intel/ifs/ifs.h | 2 ++ > drivers/platform/x86/intel/ifs/core.c | 20 ++++++++++++++++---- > drivers/platform/x86/intel/ifs/load.c | 14 ++++---------- > 3 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h > index 3ff1d9aaeaa9..8de1952a1b7b 100644 > --- a/drivers/platform/x86/intel/ifs/ifs.h > +++ b/drivers/platform/x86/intel/ifs/ifs.h > @@ -191,6 +191,7 @@ union ifs_status { > * struct ifs_data - attributes related to intel IFS driver > * @integrity_cap_bit: MSR_INTEGRITY_CAPS bit enumerating this test > * @loaded_version: stores the currently loaded ifs image version. > + * @pkg_auth: array of bool storing per package auth status > * @loaded: If a valid test binary has been loaded into the memory > * @loading_error: Error occurred on another CPU while loading image > * @valid_chunks: number of chunks which could be validated. > @@ -199,6 +200,7 @@ union ifs_status { > */ > struct ifs_data { > int integrity_cap_bit; > + bool *pkg_auth; > int loaded_version; > bool loaded; > bool loading_error; > diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c > index 5fb7f655c291..943eb2a17c64 100644 > --- a/drivers/platform/x86/intel/ifs/core.c > +++ b/drivers/platform/x86/intel/ifs/core.c > @@ -4,6 +4,7 @@ > #include <linux/module.h> > #include <linux/kdev_t.h> > #include <linux/semaphore.h> > +#include <linux/slab.h> > > #include <asm/cpu_device_id.h> > > @@ -34,6 +35,7 @@ static int __init ifs_init(void) > { > const struct x86_cpu_id *m; > u64 msrval; > + int ret; > > m = x86_match_cpu(ifs_cpu_ids); > if (!m) > @@ -50,16 +52,26 @@ static int __init ifs_init(void) > > ifs_device.misc.groups = ifs_get_groups(); > > - if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) && > - !misc_register(&ifs_device.misc)) > - return 0; > + if (!(msrval & BIT(ifs_device.data.integrity_cap_bit))) > + return -ENODEV; > + > + ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL); > + if (!ifs_device.data.pkg_auth) > + return -ENOMEM; > + > + ret = misc_register(&ifs_device.misc); > + if (ret) { > + kfree(ifs_device.data.pkg_auth); > + return ret; > + } > > - return -ENODEV; > + return 0; > } > > static void __exit ifs_exit(void) > { > misc_deregister(&ifs_device.misc); > + kfree(ifs_device.data.pkg_auth); > } > > module_init(ifs_init); > diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c > index 89ce265887ea..8423c486d11b 100644 > --- a/drivers/platform/x86/intel/ifs/load.c > +++ b/drivers/platform/x86/intel/ifs/load.c > @@ -3,7 +3,6 @@ > > #include <linux/firmware.h> > #include <asm/cpu.h> > -#include <linux/slab.h> > #include <asm/microcode_intel.h> > > #include "ifs.h" > @@ -118,16 +117,12 @@ 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; > + int metadata_size, curr_pkg, cpu, ret; > struct ifs_data *ifsd = ifs_get_data(dev); > - 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; > - > + memset(ifsd->pkg_auth, 0, (topology_max_packages() * sizeof(bool))); > metadata_size = ifs_header_ptr->metadata_size; > > /* Spec says that if the Meta Data Size = 0 then it should be treated as 2000 */ > @@ -150,7 +145,7 @@ static int scan_chunks_sanity_check(struct device *dev) > cpus_read_lock(); > for_each_online_cpu(cpu) { > curr_pkg = topology_physical_package_id(cpu); > - if (package_authenticated[curr_pkg]) > + if (ifsd->pkg_auth[curr_pkg]) > continue; > reinit_completion(&ifs_done); > local_work.dev = dev; > @@ -161,12 +156,11 @@ static int scan_chunks_sanity_check(struct device *dev) > ret = -EIO; > goto out; > } > - package_authenticated[curr_pkg] = 1; > + ifsd->pkg_auth[curr_pkg] = 1; > } > ret = 0; > out: > cpus_read_unlock(); > - kfree(package_authenticated); > > return ret; > }
On 11/17/2022 1:13 PM, Hans de Goede wrote: > Hi, > > On 11/17/22 20:59, Jithu Joseph wrote: >> IFS requires tests to be authenticated once for each CPU socket >> on a system. >> >> scan_chunks_sanity_check() was dynamically allocating memory >> to store the state of whether tests have been authenticated on >> each socket for every load operation. >> >> Move the memory allocation to init path and store the pointer >> in ifs_data struct. >> >> Also rearrange the adjacent error checking in init for a >> more simplified and natural flow. >> >> Reviewed-by: Tony Luck <tony.luck@intel.com> >> Suggested-by: Borislav Petkov <bp@alien8.de> >> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com> >> --- >> - Replaced global pkg_auth pointer to struct ifs_data (Hans) >> - Rearrange the adjacent error checking flow in ifs_init (Hans) >> - With this change there are conflicts in patches 11 and 12 (I will >> post the updated 11 and 12 if this is satisfactory) > > Thanks, this patch looks good to me now: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Thanks for the detailed review and suggestions. I will now resend patches 11 and 12 which will apply ontop of this revised patch4 . Jithu
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 3ff1d9aaeaa9..8de1952a1b7b 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -191,6 +191,7 @@ union ifs_status { * struct ifs_data - attributes related to intel IFS driver * @integrity_cap_bit: MSR_INTEGRITY_CAPS bit enumerating this test * @loaded_version: stores the currently loaded ifs image version. + * @pkg_auth: array of bool storing per package auth status * @loaded: If a valid test binary has been loaded into the memory * @loading_error: Error occurred on another CPU while loading image * @valid_chunks: number of chunks which could be validated. @@ -199,6 +200,7 @@ union ifs_status { */ struct ifs_data { int integrity_cap_bit; + bool *pkg_auth; int loaded_version; bool loaded; bool loading_error; diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 5fb7f655c291..943eb2a17c64 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/kdev_t.h> #include <linux/semaphore.h> +#include <linux/slab.h> #include <asm/cpu_device_id.h> @@ -34,6 +35,7 @@ static int __init ifs_init(void) { const struct x86_cpu_id *m; u64 msrval; + int ret; m = x86_match_cpu(ifs_cpu_ids); if (!m) @@ -50,16 +52,26 @@ static int __init ifs_init(void) ifs_device.misc.groups = ifs_get_groups(); - if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) && - !misc_register(&ifs_device.misc)) - return 0; + if (!(msrval & BIT(ifs_device.data.integrity_cap_bit))) + return -ENODEV; + + ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL); + if (!ifs_device.data.pkg_auth) + return -ENOMEM; + + ret = misc_register(&ifs_device.misc); + if (ret) { + kfree(ifs_device.data.pkg_auth); + return ret; + } - return -ENODEV; + return 0; } static void __exit ifs_exit(void) { misc_deregister(&ifs_device.misc); + kfree(ifs_device.data.pkg_auth); } module_init(ifs_init); diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index 89ce265887ea..8423c486d11b 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -3,7 +3,6 @@ #include <linux/firmware.h> #include <asm/cpu.h> -#include <linux/slab.h> #include <asm/microcode_intel.h> #include "ifs.h" @@ -118,16 +117,12 @@ 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; + int metadata_size, curr_pkg, cpu, ret; struct ifs_data *ifsd = ifs_get_data(dev); - 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; - + memset(ifsd->pkg_auth, 0, (topology_max_packages() * sizeof(bool))); metadata_size = ifs_header_ptr->metadata_size; /* Spec says that if the Meta Data Size = 0 then it should be treated as 2000 */ @@ -150,7 +145,7 @@ static int scan_chunks_sanity_check(struct device *dev) cpus_read_lock(); for_each_online_cpu(cpu) { curr_pkg = topology_physical_package_id(cpu); - if (package_authenticated[curr_pkg]) + if (ifsd->pkg_auth[curr_pkg]) continue; reinit_completion(&ifs_done); local_work.dev = dev; @@ -161,12 +156,11 @@ static int scan_chunks_sanity_check(struct device *dev) ret = -EIO; goto out; } - package_authenticated[curr_pkg] = 1; + ifsd->pkg_auth[curr_pkg] = 1; } ret = 0; out: cpus_read_unlock(); - kfree(package_authenticated); return ret; }