Skip to content

x86: MMU-based stack overflow protection #810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,9 @@ include $(srctree)/arch/x86/Makefile.idt
ifeq ($(CONFIG_X86_MMU),y)
include $(srctree)/arch/x86/Makefile.mmu
endif
ifeq ($(CONFIG_GDT_DYNAMIC),y)
include $(srctree)/arch/x86/Makefile.gdt
endif
endif

ifeq ($(CONFIG_GEN_ISR_TABLES),y)
Expand Down Expand Up @@ -1274,10 +1277,8 @@ $(help-board-dirs): help-%:
host-tools:
$(Q)$(MAKE) $(build)=scripts/basic
$(Q)$(MAKE) $(build)=scripts/kconfig standalone
$(Q)$(MAKE) $(build)=scripts/gen_idt
@mkdir -p ${ZEPHYR_BASE}/bin
@cp scripts/basic/fixdep scripts/gen_idt/gen_idt scripts/kconfig/conf \
${ZEPHYR_BASE}/bin
@cp scripts/basic/fixdep scripts/kconfig/conf ${ZEPHYR_BASE}/bin


# Documentation targets
Expand Down
12 changes: 12 additions & 0 deletions arch/x86/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ config X86_MMU
This options enables the memory management unit present in x86. Enabling
this will create boot time page table structure.

config X86_STACK_PROTECTION
bool
default n
depends on X86_MMU
select SET_GDT
select GDT_DYNAMIC
prompt "MMU-based stack overflow protection"
help
This option leverages the MMU to cause a system fatal error if the
bounds of the current process stack are overflowed. This is done
by preceding all stack areas with a 4K guard page.

menu "Floating Point Options"
depends on CPU_HAS_FPU

Expand Down
26 changes: 26 additions & 0 deletions arch/x86/Makefile.gdt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
ifeq ($(KBUILD_VERBOSE),1)
GENGDT_EXTRA_ARGS := --verbose
else
GENGDT_EXTRA_ARGS :=
endif

GENGDT := $(srctree)/scripts/gen_gdt.py

OUTPUT_FORMAT ?= elf32-i386
OUTPUT_ARCH ?= i386

quiet_cmd_gen_gdt = GDT $@
cmd_gen_gdt = \
( \
$(GENGDT) --kernel $(PREBUILT_KERNEL) \
--output-gdt gdt.bin \
$(GENGDT_EXTRA_ARGS) && \
$(OBJCOPY) -I binary -B $(OUTPUT_ARCH) -O $(OUTPUT_FORMAT) \
--rename-section .data=gdt_ram_data gdt.bin $@ \
)

gdt.o: $(PREBUILT_KERNEL) $(GENGDT)
$(call cmd,gen_gdt)

GENERATED_KERNEL_OBJECT_FILES += gdt.o

21 changes: 7 additions & 14 deletions arch/x86/Makefile.idt
Original file line number Diff line number Diff line change
@@ -1,36 +1,29 @@
ifeq ($(KBUILD_VERBOSE),1)
GENIDT_EXTRA_ARGS := -d
GENIDT_EXTRA_ARGS := --verbose
else
GENIDT_EXTRA_ARGS :=
endif

ifeq ($(PREBUILT_HOST_TOOLS),)
GENIDT := scripts/gen_idt/gen_idt
else
GENIDT := $(PREBUILT_HOST_TOOLS)/gen_idt
endif
GENIDT := $(srctree)/scripts/gen_idt.py

OUTPUT_FORMAT ?= elf32-i386
OUTPUT_ARCH ?= i386

quiet_cmd_gen_idt = SIDT $@
cmd_gen_idt = \
( \
$(OBJCOPY) -I $(OUTPUT_FORMAT) -O binary -j intList $< isrList.bin && \
$(GENIDT) -i isrList.bin -n $(CONFIG_IDT_NUM_VECTORS) -o staticIdt.bin \
-m irq_int_vector_map.bin \
-l $(CONFIG_MAX_IRQ_LINES) $(GENIDT_EXTRA_ARGS) && \
$(GENIDT) --kernel $(PREBUILT_KERNEL) \
--output-idt staticIdt.bin \
--vector-map irq_int_vector_map.bin \
$(GENIDT_EXTRA_ARGS) && \
$(OBJCOPY) -I binary -B $(OUTPUT_ARCH) -O $(OUTPUT_FORMAT) \
--rename-section .data=staticIdt staticIdt.bin staticIdt.o && \
$(OBJCOPY) -I binary -B $(OUTPUT_ARCH) -O $(OUTPUT_FORMAT) \
--rename-section .data=irq_int_vector_map irq_int_vector_map.bin \
irq_int_vector_map.o && \
rm staticIdt.bin irq_int_vector_map.bin isrList.bin \
rm staticIdt.bin irq_int_vector_map.bin \
)

$(GENIDT):
$(Q)$(MAKE) $(build)=scripts/gen_idt

staticIdt.o: $(PREBUILT_KERNEL) $(GENIDT)
$(call cmd,gen_idt)

Expand Down
1 change: 0 additions & 1 deletion arch/x86/core/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ obj-y += cpuhalt.o \

obj-$(CONFIG_IRQ_OFFLOAD) += irq_offload.o
obj-$(CONFIG_FP_SHARING) += float.o
obj-$(CONFIG_GDT_DYNAMIC) += gdt.o
obj-$(CONFIG_REBOOT_RST_CNT) += reboot_rst_cnt.o
obj-$(CONFIG_X86_MMU) += x86_mmu.o

Expand Down
9 changes: 6 additions & 3 deletions arch/x86/core/crt0.S
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,14 @@ SECTION_FUNC(TEXT_START, __start)
#ifdef CONFIG_SET_GDT
/* If we set our own GDT, update the segment registers as well.
*/
movw $0x10, %ax /* data segment selector (entry = 3) */
movw $DATA_SEG, %ax /* data segment selector (entry = 3) */
movw %ax, %ds /* set DS */
movw %ax, %es /* set ES */
movw %ax, %fs /* set FS */
movw %ax, %gs /* set GS */
movw %ax, %ss /* set SS */

ljmp $0x08, $__csSet /* set CS = 0x08 */
ljmp $CODE_SEG, $__csSet /* set CS = 0x08 */

__csSet:
#endif /* CONFIG_SET_GDT */
Expand Down Expand Up @@ -262,7 +262,10 @@ __csSet:

#endif /* CONFIG_X86_MMU */


#ifdef CONFIG_X86_STACK_PROTECTION
mov $MAIN_TSS, %ax
ltr %ax
#endif
/* Jump to C portion of kernel initialization and never return */

jmp _Cstart
Expand Down
48 changes: 45 additions & 3 deletions arch/x86/core/fatal.c
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ extern void (*_kernel_oops_handler)(void);
NANO_CPU_INT_REGISTER(_kernel_oops_handler, NANO_SOFT_IRQ,
CONFIG_X86_KERNEL_OOPS_VECTOR / 16,
CONFIG_X86_KERNEL_OOPS_VECTOR, 0);
#else
#endif

/*
* Define a default ESF for use with _NanoFatalErrorHandler() in the event
* the caller does not have a NANO_ESF to pass
Expand All @@ -149,7 +150,6 @@ const NANO_ESF _default_esf = {
0xdeaddead, /* CS */
0xdeaddead, /* EFLAGS */
};
#endif /* CONFIG_X86_KERNEL_OOPS */

#if CONFIG_EXCEPTION_DEBUG

Expand Down Expand Up @@ -192,7 +192,9 @@ EXC_FUNC_NOCODE(IV_OVERFLOW);
EXC_FUNC_NOCODE(IV_BOUND_RANGE);
EXC_FUNC_NOCODE(IV_INVALID_OPCODE);
EXC_FUNC_NOCODE(IV_DEVICE_NOT_AVAILABLE);
EXC_FUNC_CODE(IV_DOUBLE_FAULT);
#ifndef CONFIG_X86_STACK_PROTECTION
EXC_FUNC_NOCODE(IV_DOUBLE_FAULT);
#endif
EXC_FUNC_CODE(IV_INVALID_TSS);
EXC_FUNC_CODE(IV_SEGMENT_NOT_PRESENT);
EXC_FUNC_CODE(IV_STACK_FAULT);
Expand Down Expand Up @@ -230,3 +232,43 @@ FUNC_NORETURN void page_fault_handler(const NANO_ESF *pEsf)
_EXCEPTION_CONNECT_CODE(page_fault_handler, IV_PAGE_FAULT);
#endif /* CONFIG_EXCEPTION_DEBUG */

#ifdef CONFIG_X86_STACK_PROTECTION
void df_handler(void)
{
printk("DOUBLE FAULT! Likely kernel stack overflow\n");

/* TODO: do forensic analysis to figure out the faulting context
* since this exception type pushes undefined CS:EIP information
*/

/* Double faults are unrecoverable, time to freak out */
_NanoFatalErrorHandler(_NANO_ERR_KERNEL_PANIC, &_default_esf);
}

_GENERIC_SECTION(.tss)
struct task_state_segment _main_tss = {
.ss0 = DATA_SEG
};

char __noinit df_stack[512];

/* Special TSS for handling double-faults with a known good stack */
_GENERIC_SECTION(.tss)
struct task_state_segment _df_tss = {
.esp = (u32_t)(df_stack + sizeof(df_stack)),
.cs = CODE_SEG,
.ds = DATA_SEG,
.es = DATA_SEG,
.fs = DATA_SEG,
.gs = DATA_SEG,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use E, F and G segments, and in fact other OSes tend to use these for things like thread-local storage (or process IDs, etc...). May not be a great idea to set a precedent that these can be used for legitimate memory accesses. Though I guess this is only ever touched for fatal error handlers here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we set these values the same way when we initially boot up in crt0.S and I would rather have these as fixed values matching the initial boot state than uninitialized values which might not be a valid segment selector at all

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since designated initializers are being used here, omitted fields will be initialized to 0, so there's no risk of having uninitialized values in this struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm just not seeing a problem here, and this is the same configuration as is done in crt0.S, really don't want to change this unless we change both, which is outside the scope of this patch

.ss = DATA_SEG,
.eip = (u32_t)df_handler,
.cr3 = (u32_t)&__mmu_tables_start
};

/* Configure a task gate descriptor in the IDT for the double fault
* exception
*/
_X86_IDT_TSS_REGISTER(DF_TSS, -1, -1, IV_DOUBLE_FAULT, 0);

#endif /* CONFIG_X86_STACK_PROTECTION */
40 changes: 0 additions & 40 deletions arch/x86/core/gdt.c

This file was deleted.

8 changes: 8 additions & 0 deletions arch/x86/core/thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <linker/sections.h>
#include <kernel_structs.h>
#include <wait_q.h>
#include <mmustructs.h>

/* forward declaration */

Expand Down Expand Up @@ -204,6 +205,13 @@ void _new_thread(struct k_thread *thread, char *pStackMem, size_t stackSize,

unsigned long *pInitialThread;

#if CONFIG_X86_STACK_PROTECTION
_x86_mmu_set_flags(pStackMem, MMU_PAGE_SIZE, MMU_ENTRY_NOT_PRESENT,
MMU_PTE_P_MASK);
#endif
#if _STACK_GUARD_SIZE
pStackMem += _STACK_GUARD_SIZE;
#endif
_new_thread_init(thread, pStackMem, stackSize, priority, options);

/* carve the thread entry struct from the "base" of the stack */
Expand Down
49 changes: 43 additions & 6 deletions arch/x86/core/x86_mmu.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
#include<kernel.h>
#include<mmustructs.h>

/* Linker variable. It needed to access the start of the Page directory */
extern u32_t __mmu_tables_start;

#define X86_MMU_PD ((struct x86_mmu_page_directory *)\
(void *)&__mmu_tables_start)

/* Ref to _x86_mmu_buffer_validate documentation for details */
#define USER_PERM_BIT_POS ((u32_t)0x1)
#define GET_RW_PERM(flags) (flags & BUFF_WRITEABLE)
Expand Down Expand Up @@ -165,3 +159,46 @@ int _x86_mmu_buffer_validate(void *addr, size_t size, int flags)
}
return status;
}


static inline void tlb_flush_page(void *addr)
{
/* Invalidate TLB entries corresponding to the page containing the
* specified address
*/
char *page = (char *)addr;
__asm__ ("invlpg %0" :: "m" (*page));
}


void _x86_mmu_set_flags(void *ptr, size_t size, u32_t flags, u32_t mask)
{
int pde_index, pte_index;
union x86_mmu_pde *pde;
union x86_mmu_pte *pte;
struct x86_mmu_page_table *pt;

u32_t addr = (u32_t)ptr;

__ASSERT(!(addr & MMU_PAGE_MASK), "unaligned address provided");
__ASSERT(!(size & MMU_PAGE_MASK), "unaligned size provided");

while (size) {
pde_index = MMU_PDE_NUM(addr);
pde = &X86_MMU_PD->entry[pde_index];

/* TODO we're not generating 4MB entries at the moment */
__ASSERT(pde->fourmb.ps != 1, "4MB PDE found");

pt = (struct x86_mmu_page_table *)(pde->pt.page_table << 12);

pte_index = MMU_PAGE_NUM(addr);
pte = &pt->entry[pte_index];

pte->value = (pte->value & ~mask) | flags;
tlb_flush_page((void *)addr);

size -= MMU_PAGE_SIZE;
addr += MMU_PAGE_SIZE;
}
}
1 change: 1 addition & 0 deletions arch/x86/include/exception.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
".long -1\n\t" /* ISR_LIST.priority */ \
".long " STRINGIFY(vector) "\n\t" /* ISR_LIST.vec */ \
".long 0\n\t" /* ISR_LIST.dpl */ \
".long 0\n\t" /* ISR_LIST.tss */ \
".popsection\n\t" \

/* Extra preprocessor indirection to ensure arguments get expanded before
Expand Down
7 changes: 7 additions & 0 deletions arch/x86/include/kernel_arch_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@
#include <misc/dlist.h>
#endif


/* GDT layout */
#define CODE_SEG 0x08
#define DATA_SEG 0x10
#define MAIN_TSS 0x18
#define DF_TSS 0x20

/* increase to 16 bytes (or more?) to support SSE/SSE2 instructions? */

#define STACK_ALIGN_SIZE 4
Expand Down
7 changes: 4 additions & 3 deletions arch/x86/include/kernel_arch_func.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ extern "C" {
#define STACK_ROUND_UP(x) ROUND_UP(x, STACK_ALIGN_SIZE)
#define STACK_ROUND_DOWN(x) ROUND_DOWN(x, STACK_ALIGN_SIZE)

extern K_THREAD_STACK_DEFINE(_interrupt_stack, CONFIG_ISR_STACK_SIZE);

/**
*
* @brief Performs architecture-specific initialization
Expand All @@ -32,10 +34,9 @@ extern "C" {
*/
static inline void kernel_arch_init(void)
{
extern char _interrupt_stack[CONFIG_ISR_STACK_SIZE];

_kernel.nested = 0;
_kernel.irq_stack = _interrupt_stack + CONFIG_ISR_STACK_SIZE;
_kernel.irq_stack = K_THREAD_STACK_BUFFER(_interrupt_stack) +
CONFIG_ISR_STACK_SIZE;
}

/**
Expand Down
1 change: 1 addition & 0 deletions boards/x86/qemu_x86/qemu_x86_defconfig
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=25000000
CONFIG_TEST_RANDOM_GENERATOR=y
CONFIG_XIP=y
CONFIG_X86_MMU=y
CONFIG_X86_STACK_PROTECTION=y
1 change: 1 addition & 0 deletions boards/x86/qemu_x86/qemu_x86_iamcu_defconfig
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ CONFIG_TOOLCHAIN_VARIANT="iamcu"
CONFIG_X86_IAMCU=y
CONFIG_XIP=y
CONFIG_X86_MMU=y
CONFIG_X86_STACK_PROTECTION=y
Loading