From b6f86689d5b740f2cc3ac3a1032c7374b24381cc Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 19 Oct 2022 18:13:06 +0200 Subject: [PATCH 01/24] x86/microcode: Rip out the subsys interface gunk This is a left-over from the old days when CPU hotplug wasn't as robust as it is now. Currently, microcode gets loaded early on the CPU init path and there's no need to attempt to load it again, which that subsys interface callback is doing. The only other thing that the subsys interface init path was doing is adding the /sys/devices/system/cpu/cpu*/microcode/ hierarchy. So add a function which gets called on each CPU after all the necessary driver setup has happened. Use schedule_on_each_cpu() which can block because the sysfs creating code does kmem_cache_zalloc() which can block too and the initial version of this where it did that setup in an IPI handler of on_each_cpu() can cause a deadlock of the sort: lock(fs_reclaim); lock(fs_reclaim); as the IPI handler runs in IRQ context. Signed-off-by: Borislav Petkov Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221028142638.28498-2-bp@alien8.de --- arch/x86/kernel/cpu/microcode/core.c | 78 +++++++--------------------- 1 file changed, 20 insertions(+), 58 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 6a41cee242f6..4c222e667567 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -601,8 +601,8 @@ static enum ucode_state microcode_resume_cpu(int cpu) static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw) { - enum ucode_state ustate; struct ucode_cpu_info *uci = ucode_cpu_info + cpu; + enum ucode_state ustate; if (uci->valid) return UCODE_OK; @@ -636,44 +636,6 @@ static enum ucode_state microcode_update_cpu(int cpu) return microcode_init_cpu(cpu, false); } -static int mc_device_add(struct device *dev, struct subsys_interface *sif) -{ - int err, cpu = dev->id; - - if (!cpu_online(cpu)) - return 0; - - pr_debug("CPU%d added\n", cpu); - - err = sysfs_create_group(&dev->kobj, &mc_attr_group); - if (err) - return err; - - if (microcode_init_cpu(cpu, true) == UCODE_ERROR) - return -EINVAL; - - return err; -} - -static void mc_device_remove(struct device *dev, struct subsys_interface *sif) -{ - int cpu = dev->id; - - if (!cpu_online(cpu)) - return; - - pr_debug("CPU%d removed\n", cpu); - microcode_fini_cpu(cpu); - sysfs_remove_group(&dev->kobj, &mc_attr_group); -} - -static struct subsys_interface mc_cpu_interface = { - .name = "microcode", - .subsys = &cpu_subsys, - .add_dev = mc_device_add, - .remove_dev = mc_device_remove, -}; - /** * microcode_bsp_resume - Update boot CPU microcode during resume. */ @@ -713,6 +675,9 @@ static int mc_cpu_down_prep(unsigned int cpu) struct device *dev; dev = get_cpu_device(cpu); + + microcode_fini_cpu(cpu); + /* Suspend is in progress, only remove the interface */ sysfs_remove_group(&dev->kobj, &mc_attr_group); pr_debug("CPU%d removed\n", cpu); @@ -720,6 +685,18 @@ static int mc_cpu_down_prep(unsigned int cpu) return 0; } +static void setup_online_cpu(struct work_struct *work) +{ + int cpu = smp_processor_id(); + struct ucode_cpu_info *uci = ucode_cpu_info + cpu; + + memset(uci, 0, sizeof(*uci)); + + microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig); + + mc_cpu_online(cpu); +} + static struct attribute *cpu_root_microcode_attrs[] = { #ifdef CONFIG_MICROCODE_LATE_LOADING &dev_attr_reload.attr, @@ -755,23 +732,17 @@ static int __init microcode_init(void) if (IS_ERR(microcode_pdev)) return PTR_ERR(microcode_pdev); - cpus_read_lock(); - mutex_lock(µcode_mutex); - error = subsys_interface_register(&mc_cpu_interface); - mutex_unlock(µcode_mutex); - cpus_read_unlock(); - - if (error) - goto out_pdev; - error = sysfs_create_group(&cpu_subsys.dev_root->kobj, &cpu_root_microcode_group); if (error) { pr_err("Error creating microcode group!\n"); - goto out_driver; + goto out_pdev; } + /* Do per-CPU setup */ + schedule_on_each_cpu(setup_online_cpu); + register_syscore_ops(&mc_syscore_ops); cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting", mc_cpu_starting, NULL); @@ -782,15 +753,6 @@ static int __init microcode_init(void) return 0; - out_driver: - cpus_read_lock(); - mutex_lock(µcode_mutex); - - subsys_interface_unregister(&mc_cpu_interface); - - mutex_unlock(µcode_mutex); - cpus_read_unlock(); - out_pdev: platform_device_unregister(microcode_pdev); return error; From 2071c0aeda228107bf1b9e870b6187c90fbeef1d Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 19 Oct 2022 19:07:30 +0200 Subject: [PATCH 02/24] x86/microcode: Simplify init path even more Get rid of all the IPI-sending functions and their wrappers and use those which are supposed to be called on each CPU. Thus: - microcode_init_cpu() gets called on each CPU on init, applying any new microcode that the driver might've found on the filesystem. - mc_cpu_starting() simply tries to apply cached microcode as this is the cpuhp starting callback which gets called on CPU resume too. Even if the driver init function is a late initcall, there is no filesystem by then (not even a hdd driver has been loaded yet) so a new firmware load attempt cannot simply be done. It is pointless anyway - for that there's late loading if one really needs it. Signed-off-by: Borislav Petkov Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221028142638.28498-3-bp@alien8.de --- arch/x86/kernel/cpu/microcode/core.c | 120 ++++----------------------- 1 file changed, 16 insertions(+), 104 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 4c222e667567..63f7678743be 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -319,60 +319,6 @@ void reload_early_microcode(void) } } -static void collect_cpu_info_local(void *arg) -{ - struct cpu_info_ctx *ctx = arg; - - ctx->err = microcode_ops->collect_cpu_info(smp_processor_id(), - ctx->cpu_sig); -} - -static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig) -{ - struct cpu_info_ctx ctx = { .cpu_sig = cpu_sig, .err = 0 }; - int ret; - - ret = smp_call_function_single(cpu, collect_cpu_info_local, &ctx, 1); - if (!ret) - ret = ctx.err; - - return ret; -} - -static int collect_cpu_info(int cpu) -{ - struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - int ret; - - memset(uci, 0, sizeof(*uci)); - - ret = collect_cpu_info_on_target(cpu, &uci->cpu_sig); - if (!ret) - uci->valid = 1; - - return ret; -} - -static void apply_microcode_local(void *arg) -{ - enum ucode_state *err = arg; - - *err = microcode_ops->apply_microcode(smp_processor_id()); -} - -static int apply_microcode_on_target(int cpu) -{ - enum ucode_state err; - int ret; - - ret = smp_call_function_single(cpu, apply_microcode_local, &err, 1); - if (!ret) { - if (err == UCODE_ERROR) - ret = 1; - } - return ret; -} - /* fake device for request_firmware */ static struct platform_device *microcode_pdev; @@ -458,7 +404,7 @@ static int __reload_late(void *info) * below. */ if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu) - apply_microcode_local(&err); + err = microcode_ops->apply_microcode(cpu); else goto wait_for_siblings; @@ -480,7 +426,7 @@ static int __reload_late(void *info) * revision. */ if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu) - apply_microcode_local(&err); + err = microcode_ops->apply_microcode(cpu); return ret; } @@ -589,51 +535,15 @@ static void microcode_fini_cpu(int cpu) microcode_ops->microcode_fini_cpu(cpu); } -static enum ucode_state microcode_resume_cpu(int cpu) -{ - if (apply_microcode_on_target(cpu)) - return UCODE_ERROR; - - pr_debug("CPU%d updated upon resume\n", cpu); - - return UCODE_OK; -} - -static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw) -{ - struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - enum ucode_state ustate; - - if (uci->valid) - return UCODE_OK; - - if (collect_cpu_info(cpu)) - return UCODE_ERROR; - - /* --dimm. Trigger a delayed update? */ - if (system_state != SYSTEM_RUNNING) - return UCODE_NFOUND; - - ustate = microcode_ops->request_microcode_fw(cpu, µcode_pdev->dev, refresh_fw); - if (ustate == UCODE_NEW) { - pr_debug("CPU%d updated upon init\n", cpu); - apply_microcode_on_target(cpu); - } - - return ustate; -} - -static enum ucode_state microcode_update_cpu(int cpu) +static enum ucode_state microcode_init_cpu(int cpu) { struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - /* Refresh CPU microcode revision after resume. */ - collect_cpu_info(cpu); + memset(uci, 0, sizeof(*uci)); - if (uci->valid) - return microcode_resume_cpu(cpu); + microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig); - return microcode_init_cpu(cpu, false); + return microcode_ops->apply_microcode(cpu); } /** @@ -651,14 +561,14 @@ void microcode_bsp_resume(void) } static struct syscore_ops mc_syscore_ops = { - .resume = microcode_bsp_resume, + .resume = microcode_bsp_resume, }; static int mc_cpu_starting(unsigned int cpu) { - microcode_update_cpu(cpu); - pr_debug("CPU%d added\n", cpu); - return 0; + enum ucode_state err = microcode_ops->apply_microcode(cpu); + + return err == UCODE_ERROR; } static int mc_cpu_online(unsigned int cpu) @@ -688,11 +598,13 @@ static int mc_cpu_down_prep(unsigned int cpu) static void setup_online_cpu(struct work_struct *work) { int cpu = smp_processor_id(); - struct ucode_cpu_info *uci = ucode_cpu_info + cpu; + enum ucode_state err; - memset(uci, 0, sizeof(*uci)); - - microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig); + err = microcode_init_cpu(cpu); + if (err == UCODE_ERROR) { + pr_err("Error applying microcode on CPU%d\n", cpu); + return; + } mc_cpu_online(cpu); } From a61ac80ae52ea349416472cd52005f9988537208 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 19 Oct 2022 19:16:20 +0200 Subject: [PATCH 03/24] x86/microcode: Kill refresh_fw request_microcode_fw() can always request firmware now so drop this superfluous argument. Signed-off-by: Borislav Petkov Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221028142638.28498-4-bp@alien8.de --- arch/x86/include/asm/microcode.h | 3 +-- arch/x86/kernel/cpu/microcode/amd.c | 5 ++--- arch/x86/kernel/cpu/microcode/core.c | 2 +- arch/x86/kernel/cpu/microcode/intel.c | 3 +-- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h index 74ecc2bd6cd0..d4c36fbd1d39 100644 --- a/arch/x86/include/asm/microcode.h +++ b/arch/x86/include/asm/microcode.h @@ -33,8 +33,7 @@ enum ucode_state { }; struct microcode_ops { - enum ucode_state (*request_microcode_fw) (int cpu, struct device *, - bool refresh_fw); + enum ucode_state (*request_microcode_fw) (int cpu, struct device *); void (*microcode_fini_cpu) (int cpu); diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c index e7410e98fc1f..b103d5e5f447 100644 --- a/arch/x86/kernel/cpu/microcode/amd.c +++ b/arch/x86/kernel/cpu/microcode/amd.c @@ -891,8 +891,7 @@ load_microcode_amd(bool save, u8 family, const u8 *data, size_t size) * * These might be larger than 2K. */ -static enum ucode_state request_microcode_amd(int cpu, struct device *device, - bool refresh_fw) +static enum ucode_state request_microcode_amd(int cpu, struct device *device) { char fw_name[36] = "amd-ucode/microcode_amd.bin"; struct cpuinfo_x86 *c = &cpu_data(cpu); @@ -901,7 +900,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device, const struct firmware *fw; /* reload ucode container only on the boot cpu */ - if (!refresh_fw || !bsp) + if (!bsp) return UCODE_OK; if (c->x86 >= 0x15) diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 63f7678743be..7c41e0132fa1 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -477,7 +477,7 @@ static ssize_t reload_store(struct device *dev, if (ret) goto put; - tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev, true); + tmp_ret = microcode_ops->request_microcode_fw(bsp, µcode_pdev->dev); if (tmp_ret != UCODE_NEW) goto put; diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index 1fcbd671f1df..8c35c70029bf 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -885,8 +885,7 @@ static bool is_blacklisted(unsigned int cpu) return false; } -static enum ucode_state request_microcode_fw(int cpu, struct device *device, - bool refresh_fw) +static enum ucode_state request_microcode_fw(int cpu, struct device *device) { struct cpuinfo_x86 *c = &cpu_data(cpu); const struct firmware *firmware; From 2e6ff4052d89ff9eeaddece14ba88c40bf8b2721 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 19 Oct 2022 19:20:47 +0200 Subject: [PATCH 04/24] x86/microcode: Do some minor fixups Improve debugging printks and fixup formatting. Signed-off-by: Borislav Petkov Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221028142638.28498-5-bp@alien8.de --- arch/x86/kernel/cpu/microcode/core.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 7c41e0132fa1..ffb249c29f30 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -568,6 +568,8 @@ static int mc_cpu_starting(unsigned int cpu) { enum ucode_state err = microcode_ops->apply_microcode(cpu); + pr_debug("%s: CPU%d, err: %d\n", __func__, cpu, err); + return err == UCODE_ERROR; } @@ -590,7 +592,7 @@ static int mc_cpu_down_prep(unsigned int cpu) /* Suspend is in progress, only remove the interface */ sysfs_remove_group(&dev->kobj, &mc_attr_group); - pr_debug("CPU%d removed\n", cpu); + pr_debug("%s: CPU%d\n", __func__, cpu); return 0; } @@ -639,14 +641,11 @@ static int __init microcode_init(void) if (!microcode_ops) return -ENODEV; - microcode_pdev = platform_device_register_simple("microcode", -1, - NULL, 0); + microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0); if (IS_ERR(microcode_pdev)) return PTR_ERR(microcode_pdev); - error = sysfs_create_group(&cpu_subsys.dev_root->kobj, - &cpu_root_microcode_group); - + error = sysfs_create_group(&cpu_subsys.dev_root->kobj, &cpu_root_microcode_group); if (error) { pr_err("Error creating microcode group!\n"); goto out_pdev; From 254ed7cf4dd79a18bbc496ab53f6c82d45431c78 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Wed, 19 Oct 2022 19:25:27 +0200 Subject: [PATCH 05/24] x86/microcode: Drop struct ucode_cpu_info.valid It is not needed anymore. Signed-off-by: Borislav Petkov Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221028142638.28498-6-bp@alien8.de --- arch/x86/include/asm/microcode.h | 1 - arch/x86/kernel/cpu/intel.c | 1 - arch/x86/kernel/cpu/microcode/core.c | 4 ++-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h index d4c36fbd1d39..d5a58bde091c 100644 --- a/arch/x86/include/asm/microcode.h +++ b/arch/x86/include/asm/microcode.h @@ -49,7 +49,6 @@ struct microcode_ops { struct ucode_cpu_info { struct cpu_signature cpu_sig; - int valid; void *mc; }; extern struct ucode_cpu_info ucode_cpu_info[]; diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 2d7ea5480ec3..beb8ca596784 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -210,7 +210,6 @@ int intel_cpu_collect_info(struct ucode_cpu_info *uci) csig.rev = intel_get_microcode_revision(); uci->cpu_sig = csig; - uci->valid = 1; return 0; } diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index ffb249c29f30..712aafff96e0 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -554,9 +554,9 @@ void microcode_bsp_resume(void) int cpu = smp_processor_id(); struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - if (uci->valid && uci->mc) + if (uci->mc) microcode_ops->apply_microcode(cpu); - else if (!uci->mc) + else reload_early_microcode(); } From 10d4853e4c5cd64b9ef1e5579bb2e89bceab4175 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:20 -0800 Subject: [PATCH 06/24] platform/x86/intel/ifs: Remove unused selection CONFIG_INTEL_IFS_DEVICE is not used anywhere. The selection in Kconfig is therefore pointless. Delete it. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-2-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig index c341a27cc1a3..89152d46deee 100644 --- a/drivers/platform/x86/intel/ifs/Kconfig +++ b/drivers/platform/x86/intel/ifs/Kconfig @@ -4,7 +4,6 @@ config INTEL_IFS # Discussion on the list has shown that the sysfs API needs a bit # more work, mark this as broken for now depends on BROKEN - select INTEL_IFS_DEVICE help Enable support for the In Field Scan capability in select CPUs. The capability allows for running low level tests via From f4e209e956b5d66f0e6e34e89f19811c2c1e596e Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:21 -0800 Subject: [PATCH 07/24] platform/x86/intel/ifs: Return a more appropriate error code scan_chunks_sanity_check() returns -ENOMEM if it encounters an error while copying IFS test image from memory to Secure Memory. Return -EIO in this scenario, as it is more appropriate. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-3-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/load.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index d056617ddc85..89ce265887ea 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -157,8 +157,10 @@ static int scan_chunks_sanity_check(struct device *dev) INIT_WORK(&local_work.w, copy_hashes_authenticate_chunks); schedule_work_on(cpu, &local_work.w); wait_for_completion(&ifs_done); - if (ifsd->loading_error) + if (ifsd->loading_error) { + ret = -EIO; goto out; + } package_authenticated[curr_pkg] = 1; } ret = 0; From a4c30fa4ead5e6628e5ff5a45664ba7181acf6f1 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:22 -0800 Subject: [PATCH 08/24] platform/x86/intel/ifs: Remove image loading during init IFS test image is unnecessarily loaded during driver initialization. Drop image loading during ifs_init() and improve module load time. With this change, user has to load one when starting the tests. As a consequence, make ifs_sem static as it is only used within sysfs.c Suggested-by: Hans de Goede Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-4-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/core.c | 6 +----- drivers/platform/x86/intel/ifs/ifs.h | 2 -- drivers/platform/x86/intel/ifs/sysfs.c | 2 +- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 27204e3d674d..5fb7f655c291 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -51,12 +51,8 @@ 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)) { - down(&ifs_sem); - ifs_load_firmware(ifs_device.misc.this_device); - up(&ifs_sem); + !misc_register(&ifs_device.misc)) return 0; - } return -ENODEV; } diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 73c8e91cf144..3ff1d9aaeaa9 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -229,6 +229,4 @@ 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 struct semaphore ifs_sem; - #endif diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c index 37d8380d6fa8..65dd6fea5342 100644 --- a/drivers/platform/x86/intel/ifs/sysfs.c +++ b/drivers/platform/x86/intel/ifs/sysfs.c @@ -13,7 +13,7 @@ * Protects against simultaneous tests on multiple cores, or * reloading can file while a test is in progress */ -DEFINE_SEMAPHORE(ifs_sem); +static DEFINE_SEMAPHORE(ifs_sem); /* * The sysfs interface to check additional details of last test From cb5eceee816bf05667089869d822b9cbc919465a Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Thu, 17 Nov 2022 11:59:57 -0800 Subject: [PATCH 09/24] 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 Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117195957.28225-1-jithu.joseph@intel.com --- 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 5fb7f655c291..943eb2a17c64 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -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; - 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; } 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 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/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 #include -#include #include #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; } From 716f380275866350ee44447b3c7c999f39c3178d Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:24 -0800 Subject: [PATCH 10/24] x86/microcode/intel: Reuse find_matching_signature() IFS uses test images provided by Intel that can be regarded as firmware. An IFS test image carries microcode header with an extended signature table. Reuse find_matching_signature() for verifying if the test image header or the extended signature table indicate whether that image is fit to run on a system. No functional changes. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Ashok Raj Reviewed-by: Sohil Mehta Link: https://lore.kernel.org/r/20221117035935.4136738-6-jithu.joseph@intel.com --- arch/x86/include/asm/cpu.h | 1 + arch/x86/kernel/cpu/intel.c | 29 ++++++++++++++++++ arch/x86/kernel/cpu/microcode/intel.c | 44 +++++---------------------- 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index b472ef76826a..e853440b5c65 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -95,5 +95,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1, } extern u64 x86_read_arch_cap_msr(void); +int intel_find_matching_signature(void *mc, unsigned int csig, int cpf); #endif /* _ASM_X86_CPU_H */ diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index beb8ca596784..c7331eced21f 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -215,6 +215,35 @@ int intel_cpu_collect_info(struct ucode_cpu_info *uci) } EXPORT_SYMBOL_GPL(intel_cpu_collect_info); +/* + * Returns 1 if update has been found, 0 otherwise. + */ +int intel_find_matching_signature(void *mc, unsigned int csig, int cpf) +{ + struct microcode_header_intel *mc_hdr = mc; + struct extended_sigtable *ext_hdr; + struct extended_signature *ext_sig; + int i; + + if (intel_cpu_signatures_match(csig, cpf, mc_hdr->sig, mc_hdr->pf)) + return 1; + + /* Look for ext. headers: */ + if (get_totalsize(mc_hdr) <= get_datasize(mc_hdr) + MC_HEADER_SIZE) + return 0; + + ext_hdr = mc + get_datasize(mc_hdr) + MC_HEADER_SIZE; + ext_sig = (void *)ext_hdr + EXT_HEADER_SIZE; + + for (i = 0; i < ext_hdr->count; i++) { + if (intel_cpu_signatures_match(csig, cpf, ext_sig->sig, ext_sig->pf)) + return 1; + ext_sig++; + } + return 0; +} +EXPORT_SYMBOL_GPL(intel_find_matching_signature); + static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable; diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index 8c35c70029bf..c77ec1ba6844 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -45,34 +45,6 @@ static struct microcode_intel *intel_ucode_patch; /* last level cache size per core */ static int llc_size_per_core; -/* - * Returns 1 if update has been found, 0 otherwise. - */ -static int find_matching_signature(void *mc, unsigned int csig, int cpf) -{ - struct microcode_header_intel *mc_hdr = mc; - struct extended_sigtable *ext_hdr; - struct extended_signature *ext_sig; - int i; - - if (intel_cpu_signatures_match(csig, cpf, mc_hdr->sig, mc_hdr->pf)) - return 1; - - /* Look for ext. headers: */ - if (get_totalsize(mc_hdr) <= get_datasize(mc_hdr) + MC_HEADER_SIZE) - return 0; - - ext_hdr = mc + get_datasize(mc_hdr) + MC_HEADER_SIZE; - ext_sig = (void *)ext_hdr + EXT_HEADER_SIZE; - - for (i = 0; i < ext_hdr->count; i++) { - if (intel_cpu_signatures_match(csig, cpf, ext_sig->sig, ext_sig->pf)) - return 1; - ext_sig++; - } - return 0; -} - /* * Returns 1 if update has been found, 0 otherwise. */ @@ -83,7 +55,7 @@ static int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev if (mc_hdr->rev <= new_rev) return 0; - return find_matching_signature(mc, csig, cpf); + return intel_find_matching_signature(mc, csig, cpf); } static struct ucode_patch *memdup_patch(void *data, unsigned int size) @@ -117,7 +89,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne sig = mc_saved_hdr->sig; pf = mc_saved_hdr->pf; - if (find_matching_signature(data, sig, pf)) { + if (intel_find_matching_signature(data, sig, pf)) { prev_found = true; if (mc_hdr->rev <= mc_saved_hdr->rev) @@ -149,7 +121,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne if (!p) return; - if (!find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf)) + if (!intel_find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf)) return; /* @@ -286,8 +258,8 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save) size -= mc_size; - if (!find_matching_signature(data, uci->cpu_sig.sig, - uci->cpu_sig.pf)) { + if (!intel_find_matching_signature(data, uci->cpu_sig.sig, + uci->cpu_sig.pf)) { data += mc_size; continue; } @@ -652,9 +624,9 @@ static struct microcode_intel *find_patch(struct ucode_cpu_info *uci) if (phdr->rev <= uci->cpu_sig.rev) continue; - if (!find_matching_signature(phdr, - uci->cpu_sig.sig, - uci->cpu_sig.pf)) + if (!intel_find_matching_signature(phdr, + uci->cpu_sig.sig, + uci->cpu_sig.pf)) continue; return iter->data; From 2e13ab0158dd4a033d61a5baa8f9b5b7c9ea8431 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:25 -0800 Subject: [PATCH 11/24] x86/microcode/intel: Use appropriate type in microcode_sanity_check() The data type of the @print_err parameter used by microcode_sanity_check() is int. In preparation for exporting this function to be used by the IFS driver convert it to a more appropriate bool type for readability. No functional change intended. Suggested-by: Tony Luck Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Ashok Raj Reviewed-by: Sohil Mehta Link: https://lore.kernel.org/r/20221117035935.4136738-7-jithu.joseph@intel.com --- arch/x86/kernel/cpu/microcode/intel.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index c77ec1ba6844..e48f05ec15aa 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -135,7 +135,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne intel_ucode_patch = p->data; } -static int microcode_sanity_check(void *mc, int print_err) +static int microcode_sanity_check(void *mc, bool print_err) { unsigned long total_size, data_size, ext_table_size; struct microcode_header_intel *mc_header = mc; @@ -253,7 +253,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save) mc_size = get_totalsize(mc_header); if (!mc_size || mc_size > size || - microcode_sanity_check(data, 0) < 0) + microcode_sanity_check(data, false) < 0) break; size -= mc_size; @@ -792,7 +792,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter) memcpy(mc, &mc_header, sizeof(mc_header)); data = mc + sizeof(mc_header); if (!copy_from_iter_full(data, data_size, iter) || - microcode_sanity_check(mc, 1) < 0) { + microcode_sanity_check(mc, true) < 0) { break; } From 514ee839c6d0750c1c4456502e6fa08599e57931 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:26 -0800 Subject: [PATCH 12/24] x86/microcode/intel: Reuse microcode_sanity_check() IFS test image carries the same microcode header as regular Intel microcode blobs. Reuse microcode_sanity_check() in the IFS driver to perform sanity check of the IFS test images too. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Ashok Raj Reviewed-by: Sohil Mehta Link: https://lore.kernel.org/r/20221117035935.4136738-8-jithu.joseph@intel.com --- arch/x86/include/asm/cpu.h | 1 + arch/x86/kernel/cpu/intel.c | 99 +++++++++++++++++++++++++ arch/x86/kernel/cpu/microcode/intel.c | 102 +------------------------- 3 files changed, 102 insertions(+), 100 deletions(-) diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index e853440b5c65..9e3ac95acf2d 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -96,5 +96,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1, extern u64 x86_read_arch_cap_msr(void); int intel_find_matching_signature(void *mc, unsigned int csig, int cpf); +int intel_microcode_sanity_check(void *mc, bool print_err); #endif /* _ASM_X86_CPU_H */ diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index c7331eced21f..bef06a1fafe9 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -244,6 +244,105 @@ int intel_find_matching_signature(void *mc, unsigned int csig, int cpf) } EXPORT_SYMBOL_GPL(intel_find_matching_signature); +int intel_microcode_sanity_check(void *mc, bool print_err) +{ + unsigned long total_size, data_size, ext_table_size; + struct microcode_header_intel *mc_header = mc; + struct extended_sigtable *ext_header = NULL; + u32 sum, orig_sum, ext_sigcount = 0, i; + struct extended_signature *ext_sig; + + total_size = get_totalsize(mc_header); + data_size = get_datasize(mc_header); + + if (data_size + MC_HEADER_SIZE > total_size) { + if (print_err) + pr_err("Error: bad microcode data file size.\n"); + return -EINVAL; + } + + if (mc_header->ldrver != 1 || mc_header->hdrver != 1) { + if (print_err) + pr_err("Error: invalid/unknown microcode update format.\n"); + return -EINVAL; + } + + ext_table_size = total_size - (MC_HEADER_SIZE + data_size); + if (ext_table_size) { + u32 ext_table_sum = 0; + u32 *ext_tablep; + + if (ext_table_size < EXT_HEADER_SIZE || + ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) { + if (print_err) + pr_err("Error: truncated extended signature table.\n"); + return -EINVAL; + } + + ext_header = mc + MC_HEADER_SIZE + data_size; + if (ext_table_size != exttable_size(ext_header)) { + if (print_err) + pr_err("Error: extended signature table size mismatch.\n"); + return -EFAULT; + } + + ext_sigcount = ext_header->count; + + /* + * Check extended table checksum: the sum of all dwords that + * comprise a valid table must be 0. + */ + ext_tablep = (u32 *)ext_header; + + i = ext_table_size / sizeof(u32); + while (i--) + ext_table_sum += ext_tablep[i]; + + if (ext_table_sum) { + if (print_err) + pr_warn("Bad extended signature table checksum, aborting.\n"); + return -EINVAL; + } + } + + /* + * Calculate the checksum of update data and header. The checksum of + * valid update data and header including the extended signature table + * must be 0. + */ + orig_sum = 0; + i = (MC_HEADER_SIZE + data_size) / sizeof(u32); + while (i--) + orig_sum += ((u32 *)mc)[i]; + + if (orig_sum) { + if (print_err) + pr_err("Bad microcode data checksum, aborting.\n"); + return -EINVAL; + } + + if (!ext_table_size) + return 0; + + /* + * Check extended signature checksum: 0 => valid. + */ + for (i = 0; i < ext_sigcount; i++) { + ext_sig = (void *)ext_header + EXT_HEADER_SIZE + + EXT_SIGNATURE_SIZE * i; + + sum = (mc_header->sig + mc_header->pf + mc_header->cksum) - + (ext_sig->sig + ext_sig->pf + ext_sig->cksum); + if (sum) { + if (print_err) + pr_err("Bad extended signature checksum, aborting.\n"); + return -EINVAL; + } + } + return 0; +} +EXPORT_SYMBOL_GPL(intel_microcode_sanity_check); + static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable; diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index e48f05ec15aa..fb6ff71a5ff5 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -135,104 +135,6 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne intel_ucode_patch = p->data; } -static int microcode_sanity_check(void *mc, bool print_err) -{ - unsigned long total_size, data_size, ext_table_size; - struct microcode_header_intel *mc_header = mc; - struct extended_sigtable *ext_header = NULL; - u32 sum, orig_sum, ext_sigcount = 0, i; - struct extended_signature *ext_sig; - - total_size = get_totalsize(mc_header); - data_size = get_datasize(mc_header); - - if (data_size + MC_HEADER_SIZE > total_size) { - if (print_err) - pr_err("Error: bad microcode data file size.\n"); - return -EINVAL; - } - - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) { - if (print_err) - pr_err("Error: invalid/unknown microcode update format.\n"); - return -EINVAL; - } - - ext_table_size = total_size - (MC_HEADER_SIZE + data_size); - if (ext_table_size) { - u32 ext_table_sum = 0; - u32 *ext_tablep; - - if ((ext_table_size < EXT_HEADER_SIZE) - || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) { - if (print_err) - pr_err("Error: truncated extended signature table.\n"); - return -EINVAL; - } - - ext_header = mc + MC_HEADER_SIZE + data_size; - if (ext_table_size != exttable_size(ext_header)) { - if (print_err) - pr_err("Error: extended signature table size mismatch.\n"); - return -EFAULT; - } - - ext_sigcount = ext_header->count; - - /* - * Check extended table checksum: the sum of all dwords that - * comprise a valid table must be 0. - */ - ext_tablep = (u32 *)ext_header; - - i = ext_table_size / sizeof(u32); - while (i--) - ext_table_sum += ext_tablep[i]; - - if (ext_table_sum) { - if (print_err) - pr_warn("Bad extended signature table checksum, aborting.\n"); - return -EINVAL; - } - } - - /* - * Calculate the checksum of update data and header. The checksum of - * valid update data and header including the extended signature table - * must be 0. - */ - orig_sum = 0; - i = (MC_HEADER_SIZE + data_size) / sizeof(u32); - while (i--) - orig_sum += ((u32 *)mc)[i]; - - if (orig_sum) { - if (print_err) - pr_err("Bad microcode data checksum, aborting.\n"); - return -EINVAL; - } - - if (!ext_table_size) - return 0; - - /* - * Check extended signature checksum: 0 => valid. - */ - for (i = 0; i < ext_sigcount; i++) { - ext_sig = (void *)ext_header + EXT_HEADER_SIZE + - EXT_SIGNATURE_SIZE * i; - - sum = (mc_header->sig + mc_header->pf + mc_header->cksum) - - (ext_sig->sig + ext_sig->pf + ext_sig->cksum); - if (sum) { - if (print_err) - pr_err("Bad extended signature checksum, aborting.\n"); - return -EINVAL; - } - } - return 0; -} - /* * Get microcode matching with BSP's model. Only CPUs with the same model as * BSP can stay in the platform. @@ -253,7 +155,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save) mc_size = get_totalsize(mc_header); if (!mc_size || mc_size > size || - microcode_sanity_check(data, false) < 0) + intel_microcode_sanity_check(data, false) < 0) break; size -= mc_size; @@ -792,7 +694,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter) memcpy(mc, &mc_header, sizeof(mc_header)); data = mc + sizeof(mc_header); if (!copy_from_iter_full(data, data_size, iter) || - microcode_sanity_check(mc, true) < 0) { + intel_microcode_sanity_check(mc, true) < 0) { break; } From e0788c3281a72386e75b53a010de4bfbac7e80db Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:27 -0800 Subject: [PATCH 13/24] x86/microcode/intel: Add hdr_type to intel_microcode_sanity_check() IFS test images and microcode blobs use the same header format. Microcode blobs use header type of 1, whereas IFS test images will use header type of 2. In preparation for IFS reusing intel_microcode_sanity_check(), add header type as a parameter for sanity check. [ bp: Touchups. ] Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Ashok Raj Link: https://lore.kernel.org/r/20221117035935.4136738-9-jithu.joseph@intel.com --- arch/x86/include/asm/cpu.h | 2 +- arch/x86/include/asm/microcode_intel.h | 1 + arch/x86/kernel/cpu/intel.c | 21 ++++++++++++++++++--- arch/x86/kernel/cpu/microcode/intel.c | 4 ++-- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index 9e3ac95acf2d..78796b98a544 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -96,6 +96,6 @@ static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1, extern u64 x86_read_arch_cap_msr(void); int intel_find_matching_signature(void *mc, unsigned int csig, int cpf); -int intel_microcode_sanity_check(void *mc, bool print_err); +int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type); #endif /* _ASM_X86_CPU_H */ diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index 4c92cea7e4b5..2a999bf91ef0 100644 --- a/arch/x86/include/asm/microcode_intel.h +++ b/arch/x86/include/asm/microcode_intel.h @@ -41,6 +41,7 @@ struct extended_sigtable { #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE) #define EXT_HEADER_SIZE (sizeof(struct extended_sigtable)) #define EXT_SIGNATURE_SIZE (sizeof(struct extended_signature)) +#define MC_HEADER_TYPE_MICROCODE 1 #define get_totalsize(mc) \ (((struct microcode_intel *)mc)->hdr.datasize ? \ diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index bef06a1fafe9..b6997eb6e519 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -244,7 +244,21 @@ int intel_find_matching_signature(void *mc, unsigned int csig, int cpf) } EXPORT_SYMBOL_GPL(intel_find_matching_signature); -int intel_microcode_sanity_check(void *mc, bool print_err) +/** + * intel_microcode_sanity_check() - Sanity check microcode file. + * @mc: Pointer to the microcode file contents. + * @print_err: Display failure reason if true, silent if false. + * @hdr_type: Type of file, i.e. normal microcode file or In Field Scan file. + * Validate if the microcode header type matches with the type + * specified here. + * + * Validate certain header fields and verify if computed checksum matches + * with the one specified in the header. + * + * Return: 0 if the file passes all the checks, -EINVAL if any of the checks + * fail. + */ +int intel_microcode_sanity_check(void *mc, bool print_err, int hdr_type) { unsigned long total_size, data_size, ext_table_size; struct microcode_header_intel *mc_header = mc; @@ -261,9 +275,10 @@ int intel_microcode_sanity_check(void *mc, bool print_err) return -EINVAL; } - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) { + if (mc_header->ldrver != 1 || mc_header->hdrver != hdr_type) { if (print_err) - pr_err("Error: invalid/unknown microcode update format.\n"); + pr_err("Error: invalid/unknown microcode update format. Header type %d\n", + mc_header->hdrver); return -EINVAL; } diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index fb6ff71a5ff5..c4a00fb97f61 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -155,7 +155,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save) mc_size = get_totalsize(mc_header); if (!mc_size || mc_size > size || - intel_microcode_sanity_check(data, false) < 0) + intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0) break; size -= mc_size; @@ -694,7 +694,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter) memcpy(mc, &mc_header, sizeof(mc_header)); data = mc + sizeof(mc_header); if (!copy_from_iter_full(data, data_size, iter) || - intel_microcode_sanity_check(mc, true) < 0) { + intel_microcode_sanity_check(mc, true, MC_HEADER_TYPE_MICROCODE) < 0) { break; } From 28377e56ed22ecce60260eb8afdf0c9837b3505f Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:28 -0800 Subject: [PATCH 14/24] x86/microcode/intel: Use a reserved field for metasize Intel is using microcode file format for IFS test images too. IFS test images use one of the existing reserved fields in microcode header to indicate the size of the region in the file allocated for metadata structures. In preparation for this, rename first of the existing reserved fields in microcode header to metasize. In subsequent patches IFS specific code will make use of this field while parsing IFS images. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Ashok Raj Reviewed-by: Sohil Mehta Link: https://lore.kernel.org/r/20221117035935.4136738-10-jithu.joseph@intel.com --- arch/x86/include/asm/microcode_intel.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index 2a999bf91ef0..6af1e703cb2e 100644 --- a/arch/x86/include/asm/microcode_intel.h +++ b/arch/x86/include/asm/microcode_intel.h @@ -14,7 +14,8 @@ struct microcode_header_intel { unsigned int pf; unsigned int datasize; unsigned int totalsize; - unsigned int reserved[3]; + unsigned int metasize; + unsigned int reserved[2]; }; struct microcode_intel { From 8382fee3bb86526bde1bfb1a06834f056140e0dd Mon Sep 17 00:00:00 2001 From: Ashok Raj Date: Wed, 16 Nov 2022 19:59:29 -0800 Subject: [PATCH 15/24] platform/x86/intel/ifs: Add metadata support One of the existing reserved fields in the microcode header has been allocated to indicate the size of metadata structures. The location of metadata section within microcode header is as shown below: Microcode Blob Format +----------------------+ Base |Header Version | +----------------------+ |Update revision | +----------------------+ |Date DDMMYYYY | +----------------------+ |Sig | +----------------------+ |Checksum | +----------------------+ |Loader Version | +----------------------+ |Processor Flags | +----------------------+ |Data Size | +----------------------+ |Total Size | +----------------------+ |Meta Size | +----------------------+ |Reserved | +----------------------+ |Reserved | +----------------------+ Base+48 | | | Microcode | | Data | | | +----------------------+ Base+48+data_size- | | meta_size | Meta Data | | structure(s) | | | +----------------------+ Base+48+data_size | | | Extended Signature | | Table | | | +----------------------+ Base+total_size Add an accessor function which will return a pointer to the start of a specific meta_type being queried. [ bp: Massage commit message. ] Signed-off-by: Ashok Raj Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-11-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/load.c | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index 8423c486d11b..9228da560736 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -43,6 +43,38 @@ static const char * const scan_authentication_status[] = { [2] = "Chunk authentication error. The hash of chunk did not match expected value" }; +#define MC_HEADER_META_TYPE_END (0) + +struct metadata_header { + unsigned int type; + unsigned int blk_size; +}; + +static struct metadata_header *find_meta_data(void *ucode, unsigned int meta_type) +{ + struct metadata_header *meta_header; + unsigned long data_size, total_meta; + unsigned long meta_size = 0; + + data_size = get_datasize(ucode); + total_meta = ((struct microcode_intel *)ucode)->hdr.metasize; + if (!total_meta) + return NULL; + + meta_header = (ucode + MC_HEADER_SIZE + data_size) - total_meta; + + while (meta_header->type != MC_HEADER_META_TYPE_END && + meta_header->blk_size && + meta_size < total_meta) { + meta_size += meta_header->blk_size; + if (meta_header->type == meta_type) + return meta_header; + + meta_header = (void *)meta_header + meta_header->blk_size; + } + return NULL; +} + /* * To copy scan hashes and authenticate test chunks, the initiating cpu must point * to the EDX:EAX to the test image in linear address. From aa63e0fda85edf9a8431fc31a2b2d4f3f40592f9 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Thu, 17 Nov 2022 14:50:39 -0800 Subject: [PATCH 16/24] platform/x86/intel/ifs: Use generic microcode headers and functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 have been tweaked to become fully compatible with the microcode format. Newer IFS test image headers will use header version 2 in order to distinguish it from microcode images and older IFS test images. In light of the above, reuse struct microcode_header_intel directly in the IFS driver and reuse microcode functions for validation and sanity checking. [ bp: Massage commit message. ] Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117225039.30166-1-jithu.joseph@intel.com --- arch/x86/include/asm/microcode_intel.h | 1 + drivers/platform/x86/intel/ifs/load.c | 104 +++++-------------------- 2 files changed, 21 insertions(+), 84 deletions(-) diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index 6af1e703cb2e..f1fa979e05bf 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 MC_HEADER_TYPE_MICROCODE 1 +#define MC_HEADER_TYPE_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 9228da560736..83434160bc4c 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -7,22 +7,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); @@ -149,29 +135,14 @@ 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; struct ifs_data *ifsd = ifs_get_data(dev); struct ifs_work local_work; - char *test_ptr; + int curr_pkg, cpu, 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 */ - 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(); @@ -197,67 +168,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 image_sanity_check(struct device *dev, const struct microcode_header_intel *data) { - unsigned long total_size, data_size; - u32 sum, *mc; + struct ucode_cpu_info uci; - total_size = get_totalsize(mc_header); - data_size = get_datasize(mc_header); - - 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 != MC_HEADER_TYPE_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, MC_HEADER_TYPE_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}. @@ -278,12 +215,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 = 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); From 48c6e7dc19051c5ef725490cf8673d768cda7748 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Thu, 17 Nov 2022 15:04:08 -0800 Subject: [PATCH 17/24] platform/x86/intel/ifs: Add metadata validation The data portion of a IFS test image file contains a metadata region containing possibly multiple metadata structures in addition to test data and hashes. 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 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. [ bp: Massage commit message. ] Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117230408.30331-1-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/ifs.h | 2 + drivers/platform/x86/intel/ifs/load.c | 58 ++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 8de1952a1b7b..74c051c544f4 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -197,6 +197,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; @@ -207,6 +208,7 @@ struct ifs_data { int valid_chunks; int status; u64 scan_details; + u32 cur_batch; }; struct ifs_work { diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index 83434160bc4c..edc7baa976bf 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -7,7 +7,25 @@ #include "ifs.h" +#define IFS_CHUNK_ALIGNMENT 256 +union meta_data { + struct { + u32 meta_type; // metadata type + u32 meta_size; // size of this entire struct including hdrs. + u32 test_type; // IFS test type + u32 fusa_info; // Fusa info + u32 total_images; // Total number of images + u32 current_image; // Current Image # + u32 total_chunks; // Total number of chunks in this image + u32 starting_chunk; // Starting chunk number in this image + u32 size_per_chunk; // size of each chunk + u32 chunks_per_stride; // number of chunks in a stride + }; + u8 padding[IFS_CHUNK_ALIGNMENT]; +}; + #define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel)) +#define META_TYPE_IFS 1 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 */ @@ -128,6 +146,41 @@ 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); + union 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 = (union meta_data *)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(union 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 is not aligned on %d bytes aligned in %s\n", + IFS_CHUNK_ALIGNMENT, 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 @@ -139,8 +192,11 @@ static int scan_chunks_sanity_check(struct device *dev) struct ifs_work local_work; int curr_pkg, cpu, ret; - memset(ifsd->pkg_auth, 0, (topology_max_packages() * sizeof(bool))); + ret = validate_ifs_metadata(dev); + if (ret) + return ret; + ifsd->loading_error = false; ifsd->loaded_version = ifs_header_ptr->rev; From bf835ee852be38e9fab1fdb330eccdd9728aec34 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:32 -0800 Subject: [PATCH 18/24] platform/x86/intel/ifs: Remove reload sysfs entry Reload sysfs entry will be replaced by current_batch, drop it. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-14-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/sysfs.c | 29 -------------------------- 1 file changed, 29 deletions(-) diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c index 65dd6fea5342..e077910c5d28 100644 --- a/drivers/platform/x86/intel/ifs/sysfs.c +++ b/drivers/platform/x86/intel/ifs/sysfs.c @@ -87,34 +87,6 @@ static ssize_t run_test_store(struct device *dev, static DEVICE_ATTR_WO(run_test); -/* - * Reload the IFS image. When user wants to install new IFS image - */ -static ssize_t reload_store(struct device *dev, - struct device_attribute *attr, - const char *buf, size_t count) -{ - struct ifs_data *ifsd = ifs_get_data(dev); - bool res; - - - if (kstrtobool(buf, &res)) - return -EINVAL; - if (!res) - return count; - - if (down_interruptible(&ifs_sem)) - return -EINTR; - - ifs_load_firmware(dev); - - up(&ifs_sem); - - return ifsd->loaded ? count : -ENODEV; -} - -static DEVICE_ATTR_WO(reload); - /* * Display currently loaded IFS image version. */ @@ -136,7 +108,6 @@ static struct attribute *plat_ifs_attrs[] = { &dev_attr_details.attr, &dev_attr_status.attr, &dev_attr_run_test.attr, - &dev_attr_reload.attr, &dev_attr_image_version.attr, NULL }; From 4fb858f3dcd25cf568e35ff53ce8fa8a660fc372 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:33 -0800 Subject: [PATCH 19/24] platform/x86/intel/ifs: Add current_batch sysfs entry Initial implementation assumed a single IFS test image file with a fixed name ff-mm-ss.scan. (where ff, mm, ss refers to family, model and stepping of the core). Subsequently, it became evident that supporting more than one test image file is needed to provide more comprehensive test coverage. (Test coverage in this scenario refers to testing more transistors in the core to identify faults). The other alternative of increasing the size of a single scan test image file would not work as the upper bound is limited by the size of memory area reserved by BIOS for loading IFS test image. Introduce "current_batch" file which accepts a number. Writing a number to the current_batch file would load the test image file by name ff-mm-ss-.scan, where is the number written to the "current_batch" file in hex. Range check of the input is done to verify it not greater than 0xff. For e.g if the scan test image comprises of 6 files, they would be named: 06-8f-06-01.scan 06-8f-06-02.scan 06-8f-06-03.scan 06-8f-06-04.scan 06-8f-06-05.scan 06-8f-06-06.scan And writing 3 to current_batch would result in loading 06-8f-06-03.scan above. The file can also be read to know the currently loaded file. And testing a system looks like: for each scan file do load the IFS test image file (write to the batch file) for each core do test the core with this set of tests done done Qualify few error messages with the test image file suffix to provide better context. [ bp: Massage commit message. Add link to the discussion. ] Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221107225323.2733518-13-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/core.c | 1 + drivers/platform/x86/intel/ifs/ifs.h | 23 ++++++++++---- drivers/platform/x86/intel/ifs/load.c | 18 +++++++---- drivers/platform/x86/intel/ifs/runtest.c | 10 ++++--- drivers/platform/x86/intel/ifs/sysfs.c | 38 ++++++++++++++++++++++++ 5 files changed, 74 insertions(+), 16 deletions(-) diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 943eb2a17c64..206a617c2e02 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -23,6 +23,7 @@ MODULE_DEVICE_TABLE(x86cpu, ifs_cpu_ids); static struct ifs_device ifs_device = { .data = { .integrity_cap_bit = MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT, + .test_num = 0, }, .misc = { .name = "intel_ifs_0", diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 74c051c544f4..da1474e1d8eb 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -33,13 +33,23 @@ * The driver loads the tests into memory reserved BIOS local to each CPU * socket in a two step process using writes to MSRs to first load the * SHA hashes for the test. Then the tests themselves. Status MSRs provide - * feedback on the success/failure of these steps. When a new test file - * is installed it can be loaded by writing to the driver reload file:: + * feedback on the success/failure of these steps. * - * # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload + * The test files are kept in a fixed location: /lib/firmware/intel/ifs_0/ + * For e.g if there are 3 test files, they would be named in the following + * fashion: + * ff-mm-ss-01.scan + * ff-mm-ss-02.scan + * ff-mm-ss-03.scan + * (where ff refers to family, mm indicates model and ss indicates stepping) * - * Similar to microcode, the current version of the scan tests is stored - * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan + * A different test file can be loaded by writing the numerical portion + * (e.g 1, 2 or 3 in the above scenario) into the curent_batch file. + * To load ff-mm-ss-02.scan, the following command can be used:: + * + * # echo 2 > /sys/devices/virtual/misc/intel_ifs_0/current_batch + * + * The above file can also be read to know the currently loaded image. * * Running tests * ------------- @@ -209,6 +219,7 @@ struct ifs_data { int status; u64 scan_details; u32 cur_batch; + int test_num; }; struct ifs_work { @@ -229,7 +240,7 @@ static inline struct ifs_data *ifs_get_data(struct device *dev) return &d->data; } -void ifs_load_firmware(struct device *dev); +int ifs_load_firmware(struct device *dev); int do_core_test(int cpu, struct device *dev); const struct attribute_group **ifs_get_groups(void); diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index edc7baa976bf..c5c24e6fdc43 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -253,17 +253,18 @@ static int image_sanity_check(struct device *dev, const struct microcode_header_ /* * 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}. + * in /lib/firmware/intel/ifs_x/ and named as family-model-stepping-02x.{testname}. */ -void ifs_load_firmware(struct device *dev) +int ifs_load_firmware(struct device *dev) { struct ifs_data *ifsd = ifs_get_data(dev); const struct firmware *fw; - char scan_path[32]; - int ret; + char scan_path[64]; + int ret = -EINVAL; - snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan", - boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping); + snprintf(scan_path, sizeof(scan_path), "intel/ifs_%d/%02x-%02x-%02x-%02x.scan", + ifsd->test_num, boot_cpu_data.x86, boot_cpu_data.x86_model, + boot_cpu_data.x86_stepping, ifsd->cur_batch); ret = request_firmware_direct(&fw, scan_path, dev); if (ret) { @@ -279,8 +280,13 @@ void ifs_load_firmware(struct device *dev) ifs_hash_ptr = (u64)(ifs_header_ptr + 1); ret = scan_chunks_sanity_check(dev); + if (ret) + dev_err(dev, "Load failure for batch: %02x\n", ifsd->cur_batch); + release: release_firmware(fw); done: ifsd->loaded = (ret == 0); + + return ret; } diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index b2ca2bb4501f..0bfd8fcdd7e8 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -78,14 +78,16 @@ static void message_not_tested(struct device *dev, int cpu, union ifs_status sta static void message_fail(struct device *dev, int cpu, union ifs_status status) { + struct ifs_data *ifsd = ifs_get_data(dev); + /* * control_error is set when the microcode runs into a problem * loading the image from the reserved BIOS memory, or it has * been corrupted. Reloading the image may fix this issue. */ if (status.control_error) { - dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image\n", - cpumask_pr_args(cpu_smt_mask(cpu))); + dev_err(dev, "CPU(s) %*pbl: could not execute from loaded scan image. Batch: %02x version: 0x%x\n", + cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version); } /* @@ -96,8 +98,8 @@ static void message_fail(struct device *dev, int cpu, union ifs_status status) * the core being tested. */ if (status.signature_error) { - dev_err(dev, "CPU(s) %*pbl: test signature incorrect.\n", - cpumask_pr_args(cpu_smt_mask(cpu))); + dev_err(dev, "CPU(s) %*pbl: test signature incorrect. Batch: %02x version: 0x%x\n", + cpumask_pr_args(cpu_smt_mask(cpu)), ifsd->cur_batch, ifsd->loaded_version); } } diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c index e077910c5d28..ee636a76b083 100644 --- a/drivers/platform/x86/intel/ifs/sysfs.c +++ b/drivers/platform/x86/intel/ifs/sysfs.c @@ -87,6 +87,43 @@ static ssize_t run_test_store(struct device *dev, static DEVICE_ATTR_WO(run_test); +static ssize_t current_batch_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ifs_data *ifsd = ifs_get_data(dev); + unsigned int cur_batch; + int rc; + + rc = kstrtouint(buf, 0, &cur_batch); + if (rc < 0 || cur_batch > 0xff) + return -EINVAL; + + if (down_interruptible(&ifs_sem)) + return -EINTR; + + ifsd->cur_batch = cur_batch; + + rc = ifs_load_firmware(dev); + + up(&ifs_sem); + + return (rc == 0) ? count : rc; +} + +static ssize_t current_batch_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct ifs_data *ifsd = ifs_get_data(dev); + + if (!ifsd->loaded) + return sysfs_emit(buf, "none\n"); + else + return sysfs_emit(buf, "0x%02x\n", ifsd->cur_batch); +} + +static DEVICE_ATTR_RW(current_batch); + /* * Display currently loaded IFS image version. */ @@ -108,6 +145,7 @@ static struct attribute *plat_ifs_attrs[] = { &dev_attr_details.attr, &dev_attr_status.attr, &dev_attr_run_test.attr, + &dev_attr_current_batch.attr, &dev_attr_image_version.attr, NULL }; From 72a0f445fc091bd18873b10b9ab56573e490f00d Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:34 -0800 Subject: [PATCH 20/24] Documentation/ABI: Update IFS ABI doc Remove reload documentation and add current_batch documentation. Update the kernel version and date for all the entries. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/r/20221117035935.4136738-16-jithu.joseph@intel.com --- .../ABI/testing/sysfs-platform-intel-ifs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs index 486d6d2ff8a0..55991983d0d0 100644 --- a/Documentation/ABI/testing/sysfs-platform-intel-ifs +++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs @@ -1,39 +1,41 @@ What: /sys/devices/virtual/misc/intel_ifs_/run_test -Date: April 21 2022 -KernelVersion: 5.19 +Date: Nov 16 2022 +KernelVersion: 6.2 Contact: "Jithu Joseph" Description: Write to trigger IFS test for one online core. Note that the test is per core. The cpu# can be for any thread on the core. Running on one thread completes the test for the core containing that thread. Example: to test the core containing cpu5: echo 5 > - /sys/devices/platform/intel_ifs./run_test + /sys/devices/virtual/misc/intel_ifs_/run_test What: /sys/devices/virtual/misc/intel_ifs_/status -Date: April 21 2022 -KernelVersion: 5.19 +Date: Nov 16 2022 +KernelVersion: 6.2 Contact: "Jithu Joseph" Description: The status of the last test. It can be one of "pass", "fail" or "untested". What: /sys/devices/virtual/misc/intel_ifs_/details -Date: April 21 2022 -KernelVersion: 5.19 +Date: Nov 16 2022 +KernelVersion: 6.2 Contact: "Jithu Joseph" Description: Additional information regarding the last test. The details file reports the hex value of the SCAN_STATUS MSR. Note that the error_code field may contain driver defined software code not defined in the Intel SDM. What: /sys/devices/virtual/misc/intel_ifs_/image_version -Date: April 21 2022 -KernelVersion: 5.19 +Date: Nov 16 2022 +KernelVersion: 6.2 Contact: "Jithu Joseph" Description: Version (hexadecimal) of loaded IFS binary image. If no scan image is loaded reports "none". -What: /sys/devices/virtual/misc/intel_ifs_/reload -Date: April 21 2022 -KernelVersion: 5.19 +What: /sys/devices/virtual/misc/intel_ifs_/current_batch +Date: Nov 16 2022 +KernelVersion: 6.2 Contact: "Jithu Joseph" -Description: Write "1" (or "y" or "Y") to reload the IFS image from - /lib/firmware/intel/ifs/ff-mm-ss.scan. +Description: Write a number less than or equal to 0xff to load an IFS test image. + The number written treated as the 2 digit suffix in the following file name: + /lib/firmware/intel/ifs_/ff-mm-ss-02x.scan + Reading the file will provide the suffix of the currently loaded IFS test image. From 1a63b58082869273bfbab1b945007193f7bd3a78 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Wed, 16 Nov 2022 19:59:35 -0800 Subject: [PATCH 21/24] Revert "platform/x86/intel/ifs: Mark as BROKEN" Issues with user interface [1] to load scan test images have been addressed so remove the dependency on BROKEN. Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov Reviewed-by: Tony Luck Reviewed-by: Sohil Mehta Reviewed-by: Hans de Goede Link: https://lore.kernel.org/lkml/26102aca-a730-ddf8-d024-2e7367696757@redhat.com/ [1] Link: https://lore.kernel.org/r/20221117035935.4136738-17-jithu.joseph@intel.com --- drivers/platform/x86/intel/ifs/Kconfig | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/platform/x86/intel/ifs/Kconfig b/drivers/platform/x86/intel/ifs/Kconfig index 89152d46deee..3eded966757e 100644 --- a/drivers/platform/x86/intel/ifs/Kconfig +++ b/drivers/platform/x86/intel/ifs/Kconfig @@ -1,9 +1,6 @@ config INTEL_IFS tristate "Intel In Field Scan" depends on X86 && CPU_SUP_INTEL && 64BIT && SMP - # Discussion on the list has shown that the sysfs API needs a bit - # more work, mark this as broken for now - depends on BROKEN help Enable support for the In Field Scan capability in select CPUs. The capability allows for running low level tests via From 09265345cc8900cd0bf10c2ff98e51b495b2c5b2 Mon Sep 17 00:00:00 2001 From: Jithu Joseph Date: Fri, 2 Dec 2022 21:24:45 -0800 Subject: [PATCH 22/24] platform/x86/intel/ifs: Add missing kernel-doc entry Document the test_num member of struct ifs_data. Reported-by: Stephen Rothwell Signed-off-by: Jithu Joseph Signed-off-by: Borislav Petkov (AMD) Acked-by: Randy Dunlap Link: https://lore.kernel.org/lkml/774fd22a-aaee-758d-8195-77bac783ecbc@infradead.org/ --- drivers/platform/x86/intel/ifs/ifs.h | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index da1474e1d8eb..046e39304fd5 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -208,6 +208,7 @@ union ifs_status { * @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 + * @test_num: number indicating the test type */ struct ifs_data { int integrity_cap_bit; From 5b1586ab064ca24c6a7a6be7a9d0cb9e237ef39a Mon Sep 17 00:00:00 2001 From: Ashok Raj Date: Tue, 29 Nov 2022 13:08:26 -0800 Subject: [PATCH 23/24] x86/microcode/intel: Do not print microcode revision and processor flags collect_cpu_info() is used to collect the current microcode revision and processor flags on every CPU. It had a weird mechanism to try to mimick a "once" functionality in the sense that, that information should be issued only when it is differing from the previous CPU. However (1): the new calling sequence started doing that in parallel: microcode_init() |-> schedule_on_each_cpu(setup_online_cpu) |-> collect_cpu_info() resulting in multiple redundant prints: microcode: sig=0x50654, pf=0x80, revision=0x2006e05 microcode: sig=0x50654, pf=0x80, revision=0x2006e05 microcode: sig=0x50654, pf=0x80, revision=0x2006e05 However (2): dumping this here is not that important because the kernel does not support mixed silicon steppings microcode. Finally! Besides, there is already a pr_info() in microcode_reload_late() that shows both the old and new revisions. What is more, the CPU signature (sig=0x50654) and Processor Flags (pf=0x80) above aren't that useful to the end user, they are available via /proc/cpuinfo and they don't change anyway. Remove the redundant pr_info(). [ bp: Heavily massage. ] Fixes: b6f86689d5b7 ("x86/microcode: Rip out the subsys interface gunk") Reported-by: Tony Luck Signed-off-by: Ashok Raj Signed-off-by: Borislav Petkov (AMD) Link: https://lore.kernel.org/r/20221103175901.164783-2-ashok.raj@intel.com --- arch/x86/kernel/cpu/microcode/intel.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index c4a00fb97f61..4f93875f57b4 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -554,7 +554,6 @@ void reload_ucode_intel(void) static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) { - static struct cpu_signature prev; struct cpuinfo_x86 *c = &cpu_data(cpu_num); unsigned int val[2]; @@ -570,13 +569,6 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) csig->rev = c->microcode; - /* No extra locking on prev, races are harmless. */ - if (csig->sig != prev.sig || csig->pf != prev.pf || csig->rev != prev.rev) { - pr_info("sig=0x%x, pf=0x%x, revision=0x%x\n", - csig->sig, csig->pf, csig->rev); - prev = *csig; - } - return 0; } From be1b670f61443aa5d0d01782e9b8ea0ee825d018 Mon Sep 17 00:00:00 2001 From: Ashok Raj Date: Tue, 29 Nov 2022 13:08:27 -0800 Subject: [PATCH 24/24] x86/microcode/intel: Do not retry microcode reloading on the APs The retries in load_ucode_intel_ap() were in place to support systems with mixed steppings. Mixed steppings are no longer supported and there is only one microcode image at a time. Any retries will simply reattempt to apply the same image over and over without making progress. [ bp: Zap the circumstantial reasoning from the commit message. ] Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading") Signed-off-by: Ashok Raj Signed-off-by: Borislav Petkov (AMD) Reviewed-by: Thomas Gleixner Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/20221129210832.107850-3-ashok.raj@intel.com --- arch/x86/kernel/cpu/microcode/intel.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index 4f93875f57b4..2dba4b53e683 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -495,7 +495,6 @@ void load_ucode_intel_ap(void) else iup = &intel_ucode_patch; -reget: if (!*iup) { patch = __load_ucode_intel(&uci); if (!patch) @@ -506,12 +505,7 @@ void load_ucode_intel_ap(void) uci.mc = *iup; - if (apply_microcode_early(&uci, true)) { - /* Mixed-silicon system? Try to refetch the proper patch: */ - *iup = NULL; - - goto reget; - } + apply_microcode_early(&uci, true); } static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)