Lean Sheng Tan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/84833?usp=email )
Change subject: arch/x86: Add breakpoint to stack canary ......................................................................
arch/x86: Add breakpoint to stack canary
In order to debug stack smashing add a write breakpoint to the stack canary (at _stack or _car_stack) and print the IP when the stack canary is written.
TEST: Wrote to address _stack in ramstage and got the EIP of the code that smashed the stack canary.
Change-Id: I8adf07a8425856795a4a71da5c41bec2244b02a8 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/84833 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Shuo Liu shuo.liu@intel.com Reviewed-by: Jérémy Compostella jeremy.compostella@intel.com Reviewed-by: Subrata Banik subratabanik@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/Kconfig M src/arch/x86/Makefile.mk M src/arch/x86/exception.c A src/arch/x86/include/arch/stack_canary_breakpoint.h A src/arch/x86/stack_canary_breakpoint.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/intel/fsp2_0/notify.c M src/drivers/intel/fsp2_0/silicon_init.c M src/include/symbols.h 9 files changed, 113 insertions(+), 0 deletions(-)
Approvals: Shuo Liu: Looks good to me, but someone else must approve Subrata Banik: Looks good to me, approved build bot (Jenkins): Verified Jérémy Compostella: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/arch/x86/Kconfig b/src/arch/x86/Kconfig index e99b3af..2077405 100644 --- a/src/arch/x86/Kconfig +++ b/src/arch/x86/Kconfig @@ -368,6 +368,18 @@ default y depends on DEBUG_NULL_DEREF_BREAKPOINTS && DEBUG_HW_BREAKPOINTS_IN_ALL_STAGES
+config DEBUG_STACK_OVERFLOW_BREAKPOINTS + bool + default y + depends on DEBUG_HW_BREAKPOINTS + help + Enable support for catching stack overflows + +config DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES + bool + default y + depends on DEBUG_STACK_OVERFLOW_BREAKPOINTS && DEBUG_HW_BREAKPOINTS_IN_ALL_STAGES + # Some EC need an "EC firmware pointer" (a data structure hinting the address # of its firmware blobs) being put at a fixed position. Its space # (__section__(".ecfw_ptr")) should be reserved if it lies in the range of a diff --git a/src/arch/x86/Makefile.mk b/src/arch/x86/Makefile.mk index 43c7bd4..a455a09 100644 --- a/src/arch/x86/Makefile.mk +++ b/src/arch/x86/Makefile.mk @@ -69,6 +69,7 @@ bootblock-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c bootblock-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c bootblock-$(CONFIG_DEBUG_NULL_DEREF_BREAKPOINTS_IN_ALL_STAGES) += null_breakpoint.c +bootblock-$(CONFIG_DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES) += stack_canary_breakpoint.c bootblock-$(CONFIG_BOOTBLOCK_NORMAL) += bootblock_normal.c bootblock-y += gdt_init.S bootblock-y += id.S @@ -116,6 +117,7 @@ verstage-$(CONFIG_ARCH_VERSTAGE_X86_64) += memmove_64.S verstage-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c verstage-$(CONFIG_DEBUG_NULL_DEREF_BREAKPOINTS_IN_ALL_STAGES) += null_breakpoint.c +verstage-$(CONFIG_DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES) += stack_canary_breakpoint.c # If verstage is a separate stage it means there's no need # for a chipset-specific car_stage_entry() so use the generic one # which just calls verstage(). @@ -152,6 +154,7 @@ romstage-y += memset.c romstage-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c romstage-$(CONFIG_DEBUG_NULL_DEREF_BREAKPOINTS_IN_ALL_STAGES) += null_breakpoint.c +romstage-$(CONFIG_DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES) += stack_canary_breakpoint.c romstage-y += postcar_loader.c romstage-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c romstage-$(CONFIG_HAVE_CF9_RESET) += cf9_reset.c @@ -194,6 +197,7 @@ postcar-y += memset.c postcar-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c postcar-$(CONFIG_DEBUG_NULL_DEREF_BREAKPOINTS_IN_ALL_STAGES) += null_breakpoint.c +postcar-$(CONFIG_DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES) += stack_canary_breakpoint.c postcar-y += postcar.c postcar-$(CONFIG_COLLECT_TIMESTAMPS_TSC) += timestamp.c postcar-$(CONFIG_HAVE_CF9_RESET) += cf9_reset.c @@ -238,6 +242,7 @@ ramstage-$(CONFIG_X86_TOP4G_BOOTMEDIA_MAP) += mmap_boot.c ramstage-$(CONFIG_GENERATE_MP_TABLE) += mpspec.c ramstage-$(CONFIG_DEBUG_NULL_DEREF_BREAKPOINTS) += null_breakpoint.c +ramstage-$(CONFIG_DEBUG_STACK_OVERFLOW_BREAKPOINTS) += stack_canary_breakpoint.c ramstage-$(CONFIG_GENERATE_PIRQ_TABLE) += pirq_routing.c ramstage-y += rdrand.c ramstage-$(CONFIG_GENERATE_SMBIOS_TABLES) += smbios.c diff --git a/src/arch/x86/exception.c b/src/arch/x86/exception.c index aecce7b..224f0e1 100644 --- a/src/arch/x86/exception.c +++ b/src/arch/x86/exception.c @@ -2,6 +2,7 @@ #include <arch/cpu.h> #include <arch/breakpoint.h> #include <arch/null_breakpoint.h> +#include <arch/stack_canary_breakpoint.h> #include <arch/exception.h> #include <arch/registers.h> #include <commonlib/helpers.h> @@ -665,4 +666,5 @@ load_idt(idt, sizeof(idt));
null_breakpoint_init(); + stack_canary_breakpoint_init(); } diff --git a/src/arch/x86/include/arch/stack_canary_breakpoint.h b/src/arch/x86/include/arch/stack_canary_breakpoint.h new file mode 100644 index 0000000..98c7922 --- /dev/null +++ b/src/arch/x86/include/arch/stack_canary_breakpoint.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef _STACK_CANARY_BREAKPOINT_H_ +#define _STACK_CANARY_BREAKPOINT_H_ + +#if CONFIG(DEBUG_STACK_OVERFLOW_BREAKPOINTS) && \ + (CONFIG(DEBUG_STACK_OVERFLOW_BREAKPOINTS_IN_ALL_STAGES) || ENV_RAMSTAGE) + +/* Places a data breakpoint at stack canary. */ +void stack_canary_breakpoint_init(void); +void stack_canary_breakpoint_disable(void); +#else +static inline void stack_canary_breakpoint_init(void) +{ + /* Not implemented */ +} +static inline void stack_canary_breakpoint_disable(void) +{ + /* Not implemented */ +} +#endif +#endif /* _STACK_CANARY_BREAKPOINT_H_ */ diff --git a/src/arch/x86/stack_canary_breakpoint.c b/src/arch/x86/stack_canary_breakpoint.c new file mode 100644 index 0000000..6db8d63 --- /dev/null +++ b/src/arch/x86/stack_canary_breakpoint.c @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/breakpoint.h> +#include <arch/stack_canary_breakpoint.h> +#include <arch/symbols.h> +#include <bootstate.h> +#include <console/console.h> +#include <types.h> +#include <symbols.h> + +static struct breakpoint_handle stack_canary_bp; + +static int handle_stack_canary_written(struct breakpoint_handle handle, struct eregs *regs) +{ +#if ENV_X86_64 + printk(BIOS_ERR, "Stack corruption detected at rip: 0x%llx\n", regs->rip); +#else + printk(BIOS_ERR, "Stack corruption detected at eip: 0x%x\n", regs->eip); +#endif + return 0; +} + +static void create_stack_canary_breakpoint(uintptr_t *addr) +{ + enum breakpoint_result res = + breakpoint_create_data(&stack_canary_bp, addr, sizeof(uintptr_t), true); + + if (res != BREAKPOINT_RES_OK) { + printk(BIOS_ERR, "Failed to create stack canary breakpoint\n"); + return; + } + + breakpoint_set_handler(stack_canary_bp, &handle_stack_canary_written); + breakpoint_enable(stack_canary_bp, true); +} + +void stack_canary_breakpoint_init(void) +{ + uintptr_t *addr; + + if (CONFIG(RESET_VECTOR_IN_RAM)) { + addr = (uintptr_t *)_earlyram_stack; + } else if (ENV_CACHE_AS_RAM) { + addr = (uintptr_t *)_car_stack; + } else { + addr = (uintptr_t *)_stack; + } + + create_stack_canary_breakpoint(addr); +} + +void stack_canary_breakpoint_disable(void) +{ + breakpoint_remove(stack_canary_bp); +} + +static void stack_canary_breakpoint_disable_hook(void *unused) +{ + stack_canary_breakpoint_disable(); +} + +BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, stack_canary_breakpoint_disable_hook, NULL); +BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_BOOT, BS_ON_ENTRY, stack_canary_breakpoint_disable_hook, NULL); diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 7e9676c..39d7ce9 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/null_breakpoint.h> +#include <arch/stack_canary_breakpoint.h> #include <arch/symbols.h> #include <assert.h> #include <cbfs.h> @@ -412,6 +413,7 @@
/* FSP disables the interrupt handler so remove debug exceptions temporarily */ null_breakpoint_disable(); + stack_canary_breakpoint_disable(); post_code(POSTCODE_FSP_MEMORY_INIT); timestamp_add_now(TS_FSP_MEMORY_INIT_START); if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) @@ -421,6 +423,7 @@ else status = fsp_raminit(&fspm_upd, fsp_get_hob_list_ptr()); null_breakpoint_init(); + stack_canary_breakpoint_init();
post_code(POSTCODE_FSP_MEMORY_EXIT); timestamp_add_now(TS_FSP_MEMORY_INIT_END); diff --git a/src/drivers/intel/fsp2_0/notify.c b/src/drivers/intel/fsp2_0/notify.c index 22bbf53..3ec78d5 100644 --- a/src/drivers/intel/fsp2_0/notify.c +++ b/src/drivers/intel/fsp2_0/notify.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/null_breakpoint.h> +#include <arch/stack_canary_breakpoint.h> #include <bootstate.h> #include <console/console.h> #include <cpu/x86/mtrr.h> @@ -78,11 +79,13 @@
/* FSP disables the interrupt handler so remove debug exceptions temporarily */ null_breakpoint_disable(); + stack_canary_breakpoint_disable(); if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) ret = protected_mode_call_1arg(fspnotify, (uintptr_t)¬ify_params); else ret = fspnotify(¬ify_params); null_breakpoint_init(); + stack_canary_breakpoint_init();
timestamp_add_now(data->timestamp_after); post_code(data->post_code_after); diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c index cea894c..1e1ff99 100644 --- a/src/drivers/intel/fsp2_0/silicon_init.c +++ b/src/drivers/intel/fsp2_0/silicon_init.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */
#include <arch/null_breakpoint.h> +#include <arch/stack_canary_breakpoint.h> #include <bootsplash.h> #include <bootstate.h> #include <cbfs.h> @@ -132,11 +133,13 @@
/* FSP disables the interrupt handler so remove debug exceptions temporarily */ null_breakpoint_disable(); + stack_canary_breakpoint_disable(); if (ENV_X86_64 && CONFIG(PLATFORM_USES_FSP2_X86_32)) status = protected_mode_call_1arg(silicon_init, (uintptr_t)upd); else status = silicon_init(upd); null_breakpoint_init(); + stack_canary_breakpoint_init();
fsp_printk(status, BIOS_INFO, "FSPS");
diff --git a/src/include/symbols.h b/src/include/symbols.h index ef23814..36e4c10 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -82,6 +82,7 @@ DECLARE_OPTIONAL_REGION(bl31) DECLARE_REGION(transfer_buffer) DECLARE_OPTIONAL_REGION(watchdog_tombstone) +DECLARE_OPTIONAL_REGION(earlyram_stack)
/* Returns true when pre-RAM symbols are known to the linker. * (Does not necessarily mean that the memory is accessible.) */