From d7c371619a287a3a74e23fc3fcff4793a12deba6 Mon Sep 17 00:00:00 2001 From: Arthur Heymans Date: Thu, 7 Apr 2022 21:41:26 +0200 Subject: cpu/x86/smm_module_load: Rewrite setup_stub This code was hard to read as it did too much and had a lot of state to keep track of. It also looks like the staggered entry points were first copied and only later the parameters of the first stub were filled in. This means that only the BSP stub is actually jumping to the permanent smihandler. On the APs the stub would jump to wherever c_handler happens to point to, which is likely 0. This effectively means that on APs it's likely easy to have arbitrary code execution in SMM which is a security problem. Change-Id: I42ef9d6a30f3039f25e2cde975086a1365ca4182 Signed-off-by: Arthur Heymans Reviewed-on: https://review.coreboot.org/c/coreboot/+/63478 Tested-by: build bot (Jenkins) Reviewed-by: Martin L Roth --- src/cpu/x86/smm/smm_module_loader.c | 92 +++++++++---------------------------- 1 file changed, 21 insertions(+), 71 deletions(-) (limited to 'src/cpu') diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index f97f747132..ea006e3335 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -288,101 +288,51 @@ static int smm_module_setup_stub(const uintptr_t smbase, const size_t smm_size, struct smm_loader_params *params, void *const fxsave_area) { - size_t total_save_state_size; - size_t smm_stub_size; - uintptr_t smm_stub_loc; - size_t size; - uintptr_t base; - size_t i; - struct smm_stub_params *stub_params; struct rmodule smm_stub; - base = smbase; - size = smm_size; - - /* The number of concurrent stacks cannot exceed CONFIG_MAX_CPUS. */ - if (params->num_cpus > CONFIG_MAX_CPUS) { - printk(BIOS_ERR, "%s: not enough stacks\n", __func__); - return -1; - } - - /* Fail if can't parse the smm stub rmodule. */ if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) { printk(BIOS_ERR, "%s: unable to parse smm stub\n", __func__); return -1; } + const size_t stub_size = rmodule_memory_size(&smm_stub); - /* Adjust remaining size to account for save state. */ - total_save_state_size = params->per_cpu_save_state_size * - params->num_concurrent_save_states; - if (total_save_state_size > size) { - printk(BIOS_ERR, - "%s: more state save space needed:need -> %zx:available->%zx\n", - __func__, total_save_state_size, size); - return -1; - } - - size -= total_save_state_size; - - /* The save state size encroached over the first SMM entry point. */ - if (size <= SMM_ENTRY_OFFSET) { - printk(BIOS_ERR, "%s: encroachment over SMM entry point\n", __func__); - printk(BIOS_ERR, "%s: state save size: %zx : smm_entry_offset -> %zx\n", - __func__, size, (size_t)SMM_ENTRY_OFFSET); - return -1; - } - - smm_stub_size = rmodule_memory_size(&smm_stub); - - /* Put the stub at the main entry point */ - smm_stub_loc = base + SMM_ENTRY_OFFSET; - - /* Stub is too big to fit. */ - if (smm_stub_size > (size - SMM_ENTRY_OFFSET)) { - printk(BIOS_ERR, "%s: stub is too big to fit\n", __func__); + /* Some sanity check */ + if (stub_size >= SMM_ENTRY_OFFSET) { + printk(BIOS_ERR, "%s: Stub too large\n", __func__); return -1; } - if (stack_top == 0) { - printk(BIOS_ERR, "%s: error assigning stacks\n", __func__); - return -1; - } - /* Load the stub. */ + const uintptr_t smm_stub_loc = smbase + SMM_ENTRY_OFFSET; if (rmodule_load((void *)smm_stub_loc, &smm_stub)) { printk(BIOS_ERR, "%s: load module failed\n", __func__); return -1; } - if (!smm_stub_place_staggered_entry_points((void *)base, params, &smm_stub)) { - printk(BIOS_ERR, "%s: staggered entry points failed\n", __func__); - return -1; - } - - /* Setup the parameters for the stub code. */ - stub_params = rmodule_parameters(&smm_stub); + struct smm_stub_params *stub_params = rmodule_parameters(&smm_stub); stub_params->stack_top = stack_top; stub_params->stack_size = g_stack_size; stub_params->c_handler = (uintptr_t)params->handler; stub_params->fxsave_area = (uintptr_t)fxsave_area; stub_params->fxsave_area_size = FXSAVE_SIZE; - printk(BIOS_DEBUG, - "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); - printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n", - __func__, stub_params->stack_size); - printk(BIOS_DEBUG, "%s: runtime.start32_offset = 0x%x\n", __func__, - stub_params->start32_offset); - printk(BIOS_DEBUG, "%s: runtime.smm_size = 0x%zx\n", - __func__, smm_size); - /* Initialize the APIC id to CPU number table to be 1:1 */ - for (i = 0; i < params->num_cpus; i++) + for (int i = 0; i < params->num_cpus; i++) stub_params->apic_id_to_cpu[i] = i; - /* Allow the initiator to manipulate SMM stub parameters. */ - params->stub_params = stub_params; - printk(BIOS_DEBUG, "SMM Module: stub loaded at %lx. Will call %p\n", - smm_stub_loc, params->handler); + printk(BIOS_DEBUG, "%s: stack_top = 0x%x\n", __func__, stub_params->stack_top); + printk(BIOS_DEBUG, "%s: per cpu stack_size = 0x%x\n", __func__, + stub_params->stack_size); + printk(BIOS_DEBUG, "%s: runtime.start32_offset = 0x%x\n", __func__, + stub_params->start32_offset); + printk(BIOS_DEBUG, "%s: runtime.smm_size = 0x%zx\n", __func__, smm_size); + + if (!smm_stub_place_staggered_entry_points((void *)smbase, params, &smm_stub)) { + printk(BIOS_ERR, "%s: staggered entry points failed\n", __func__); + return -1; + } + + printk(BIOS_DEBUG, "SMM Module: stub loaded at %lx. Will call %p\n", smm_stub_loc, + params->handler); return 0; } -- cgit v1.2.3