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.
Reviewed-by: Tony Luck <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
drivers/platform/x86/intel/ifs/ifs.h | 2 ++
drivers/platform/x86/intel/ifs/core.c | 13 +++++++++++--
drivers/platform/x86/intel/ifs/load.c | 14 ++++----------
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 3ff1d9aaeaa9..3a051890d9e7 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -229,4 +229,6 @@ void ifs_load_firmware(struct device *dev);
int do_core_test(int cpu, struct device *dev);
const struct attribute_group **ifs_get_groups(void);
+extern bool *ifs_pkg_auth;
+
#endif
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 5fb7f655c291..4b39f2359180 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>
@@ -30,6 +31,8 @@ static struct ifs_device ifs_device = {
},
};
+bool *ifs_pkg_auth;
+
static int __init ifs_init(void)
{
const struct x86_cpu_id *m;
@@ -51,8 +54,13 @@ 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;
+ !misc_register(&ifs_device.misc)) {
+ ifs_pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
+ if (!ifs_pkg_auth)
+ return -ENOMEM;
+ else
+ return 0;
+ }
return -ENODEV;
}
@@ -60,6 +68,7 @@ static int __init ifs_init(void)
static void __exit ifs_exit(void)
{
misc_deregister(&ifs_device.misc);
+ kfree(ifs_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..c914e4d359db 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(ifs_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 (ifs_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;
+ ifs_pkg_auth[curr_pkg] = 1;
}
ret = 0;
out:
cpus_read_unlock();
- kfree(package_authenticated);
return ret;
}
--
2.25.1
Hi,
On 11/17/22 04: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.
>
> Reviewed-by: Tony Luck <[email protected]>
> Suggested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 2 ++
> drivers/platform/x86/intel/ifs/core.c | 13 +++++++++++--
> drivers/platform/x86/intel/ifs/load.c | 14 ++++----------
> 3 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 3ff1d9aaeaa9..3a051890d9e7 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -229,4 +229,6 @@ void ifs_load_firmware(struct device *dev);
> int do_core_test(int cpu, struct device *dev);
> const struct attribute_group **ifs_get_groups(void);
>
> +extern bool *ifs_pkg_auth;
> +
This is not necessary and ugly, nack for this patch as-is (sorry).
You can simply add this pointer to "struct ifs_data" and then
alloc it in ifs_init() before the misc_register call.
scan_chunks_sanity_check() already has a "struct ifs_data *ifsd",
so it can easily access ifs_pkg_auth through that when you make
ifs_pkg_auth part of "struct ifs_data".
Regards,
Hans
\
> #endif
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 5fb7f655c291..4b39f2359180 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>
>
> @@ -30,6 +31,8 @@ static struct ifs_device ifs_device = {
> },
> };
>
> +bool *ifs_pkg_auth;
> +
> static int __init ifs_init(void)
> {
> const struct x86_cpu_id *m;
> @@ -51,8 +54,13 @@ 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;
> + !misc_register(&ifs_device.misc)) {
> + ifs_pkg_auth = kmalloc_array(topology_max_packages(), sizeof(bool), GFP_KERNEL);
> + if (!ifs_pkg_auth)
> + return -ENOMEM;
> + else
> + return 0;
> + }
>
> return -ENODEV;
> }
> @@ -60,6 +68,7 @@ static int __init ifs_init(void)
> static void __exit ifs_exit(void)
> {
> misc_deregister(&ifs_device.misc);
> + kfree(ifs_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..c914e4d359db 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(ifs_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 (ifs_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;
> + ifs_pkg_auth[curr_pkg] = 1;
> }
> ret = 0;
> out:
> cpus_read_unlock();
> - kfree(package_authenticated);
>
> return ret;
> }
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.
Reviewed-by: Tony Luck <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
- Replaced global pkg_auth pointer to struct ifs_data (Hans)
- With this change there are conflicts in patches 11 and 12 (I will
post the updated 11 and 12 if this is satisfactory)
drivers/platform/x86/intel/ifs/ifs.h | 2 ++
drivers/platform/x86/intel/ifs/core.c | 12 ++++++++++--
drivers/platform/x86/intel/ifs/load.c | 14 ++++----------
3 files changed, 16 insertions(+), 12 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..6980a31e9786 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>
@@ -51,8 +52,14 @@ 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;
+ !misc_register(&ifs_device.misc)) {
+ ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(),
+ sizeof(bool), GFP_KERNEL);
+ if (!ifs_device.data.pkg_auth)
+ return -ENOMEM;
+ else
+ return 0;
+ }
return -ENODEV;
}
@@ -60,6 +67,7 @@ static int __init ifs_init(void)
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;
}
--
2.25.1
Hi Jithu,
On 11/17/22 18:29, 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.
>
> Reviewed-by: Tony Luck <[email protected]>
> Suggested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
> ---
> - Replaced global pkg_auth pointer to struct ifs_data (Hans)
> - With this change there are conflicts in patches 11 and 12 (I will
> post the updated 11 and 12 if this is satisfactory)
>
> drivers/platform/x86/intel/ifs/ifs.h | 2 ++
> drivers/platform/x86/intel/ifs/core.c | 12 ++++++++++--
> drivers/platform/x86/intel/ifs/load.c | 14 ++++----------
> 3 files changed, 16 insertions(+), 12 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..6980a31e9786 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>
>
> @@ -51,8 +52,14 @@ 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;
> + !misc_register(&ifs_device.misc)) {
> + ifs_device.data.pkg_auth = kmalloc_array(topology_max_packages(),
> + sizeof(bool), GFP_KERNEL);
Thank you for the new version, but as I mentioned in my review, this kmalloc
must be done *before* the misc_register(&ifs_device.misc), because as soon
as that is done the other code may get triggered creating a race condition.
More in general && the misc_register to gether with the integrity_cap_bit
is not really nice. If someone does not pay close attention they may
mis that the check of the if has the pretty big side-effect of
registering the actual misc device.
Generally speaking test-conditions for if-s should not have side
effects if possible.
> + if (!ifs_device.data.pkg_auth)
> + return -ENOMEM;
> + else
> + return 0;
> + }
>
> return -ENODEV;
> }
This also makes me realize that you have your -ENODEV error exit and
your normal success exit paths switched around from what is normal.
Why not just write the above as (can be done as part of this
patch since you need to touch it all anyways):
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 0;
}
That makes this all look much more like a normal probe() function
with the success 0 return at the end.
Where as your version has the success 0 return nested 2 levels
deep in the else of a kmalloc() error check...
Regards,
Hans
> @@ -60,6 +67,7 @@ static int __init ifs_init(void)
> 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;
> }
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 <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
---
- 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)
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;
}
--
2.25.1
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 <[email protected]>
> Suggested-by: Borislav Petkov <[email protected]>
> Signed-off-by: Jithu Joseph <[email protected]>
> ---
> - 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 <[email protected]>
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 <[email protected]>
>> Suggested-by: Borislav Petkov <[email protected]>
>> Signed-off-by: Jithu Joseph <[email protected]>
>> ---
>> - 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 <[email protected]>
>
Thanks for the detailed review and suggestions.
I will now resend patches 11 and 12 which will apply ontop of this revised patch4 .
Jithu
The following commit has been merged into the x86/microcode branch of tip:
Commit-ID: cb5eceee816bf05667089869d822b9cbc919465a
Gitweb: https://git.kernel.org/tip/cb5eceee816bf05667089869d822b9cbc919465a
Author: Jithu Joseph <[email protected]>
AuthorDate: Thu, 17 Nov 2022 11:59:57 -08:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 18 Nov 2022 21:43:18 +01:00
platform/x86/intel/ifs: Remove memory allocation from load path
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.
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Jithu Joseph <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/platform/x86/intel/ifs/core.c | 20 ++++++++++++++++----
drivers/platform/x86/intel/ifs/ifs.h | 2 ++
drivers/platform/x86/intel/ifs/load.c | 14 ++++----------
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 5fb7f65..943eb2a 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/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 3ff1d9a..8de1952 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/load.c b/drivers/platform/x86/intel/ifs/load.c
index 89ce265..8423c48 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 @@ done:
*/
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;
}