| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <75da80a5-f727-2ef2-f0f5-939cce9b2b87@redhat.com>
Date: Thu, 17 Nov 2022 22:13:56 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Jithu Joseph <jithu.joseph@...el.com>, markgross@...nel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
gregkh@...uxfoundation.org, ashok.raj@...el.com,
tony.luck@...el.com, linux-kernel@...r.kernel.org,
platform-driver-x86@...r.kernel.org, patches@...ts.linux.dev,
ravi.v.shankar@...el.com, thiago.macieira@...el.com,
athenas.jimenez.gonzalez@...el.com, sohil.mehta@...el.com
Subject: Re: [PATCH v3 04/16] platform/x86/intel/ifs: Remove memory allocation
from load path
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@...el.com>
> Suggested-by: Borislav Petkov <bp@...en8.de>
> Signed-off-by: Jithu Joseph <jithu.joseph@...el.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@...hat.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;
> }
Powered by blists - more mailing lists