x86: MMU-based stack overflow protection #810

andrewboie · 2017-07-15T19:10:09Z

We configure a guard page immediately before the thread stack area which is non-present, triggering a CPU exception.

x86 does not do an automatic stack switch when handling exceptions at the same privilege level, and the initial page fault will cause a double fault since there is nowhere for the exception stack data to be pushed, so we introduce an alternate IA hardware task with a known good stack. the double fault exception now will do task gate instead of an interrupt gate.

we also convert gen_idt to Python, and introduce a new script for creating GDT since the GDT data structure is too complex to be specified in C code at build time (certain pointer fields need to be split up, and this can't be done with symbols, only literal values)

andrewboie · 2017-07-15T19:55:20Z

Need to fix a few checkpatch nits.
Most tests are failing because we need python elftools installed on the build slaves.

lpereira

I can't comment on the x86 intricacies, but there are a few nits.

lpereira · 2017-07-17T23:36:46Z

scripts/gen_idt.py

+    sys.stderr.write(os.path.basename(sys.argv[0]) + ": " + text + "\n")
+    sys.exit(1)
+
+irq_gate_desc_format = "<HHBBH"


Minor, but it would be nice to have a C-struct-as-comment here like it's done elsewhere in the code.

it's a horrendously complex data structure but I can put a pointer to where it is described in the Intel CPU manuals, like I did in segmentation.h

Style: Similar comment, and a repeat of earlier one: this string isn't a "constant", it's code to be interpreted at runtime. It should go where it's used as a literal, not here.

lpereira · 2017-07-17T23:37:42Z

scripts/gen_idt.py

+
+    symdict = {}
+    for sym in symbols.iter_symbols():
+        symdict[sym.name] = sym.entry.st_value


Minor, but you can use a dict comprehension: return {sym.name: sym.entry.st_value for sym in symbols.iter_symbols()}

lpereira · 2017-07-17T23:47:17Z

include/arch/x86/arch.h

+ * @param vec Interrupt vector
+ * @param dpl Descriptor privilege level
+ */
+#define NANO_CPU_TSS_REGISTER(tss, irq, priority, vec, dpl) \


For how long we need to keep NANO_* identifiers around?

lpereira · 2017-07-17T23:51:12Z

scripts/gen_gdt.py

+gdt_ent_fmt = "<HHBBBB"
+
+def create_code_data_entry(base, limit, gran, ex, dpl, rw, dc):
+    present = 1;


Stray semicolon

lpereira · 2017-07-18T00:08:07Z

scripts/gen_gdt.py

+            limit)
+
+    # Cast all these to 1 or 0
+    if rw:


Instead of many boolean values, consider a bitmask. It's more readable at the call site (reducing possibility of swapping values by mistake):

create_code_data_entry(base, limit, FLAGS_EX | FLAGS_RW)

Otherwise, these statements are a no-op, as only 0 or 1 are passed from main().

andrewboie · 2017-07-18T02:09:17Z

Fixed checkpatch issues.
Addressed @lpereira review comments

andrewboie · 2017-07-20T18:59:42Z

Rebased to fix some conflicts introduced by Adithya's MMU patch that went in.

andrewboie · 2017-07-24T18:24:23Z

Rebased to fix a merge conflict.
Added a TLB flush to the function which modifies page tables.
No other changes.

andyross

Nitpickery

andyross · 2017-07-24T18:03:28Z

arch/x86/core/thread.c

@@ -204,6 +204,9 @@ void _new_thread(struct k_thread *thread, char *pStackMem, size_t stackSize,

 	unsigned long *pInitialThread;

+#if _STACK_GUARD_SIZE
+	pStackMem += _STACK_GUARD_SIZE;


You don't need the #if. If _STACK_GUARD_SIZE is zero, then the += will optimize out.

andyross · 2017-07-24T18:10:28Z

tests/kernel/fatal/src/main.c

@@ -162,7 +162,8 @@ void main(void)
 	}

 	TC_PRINT("test alt thread 3: initiate kernel panic\n");
-	k_thread_create(&alt_thread, alt_stack, sizeof(alt_stack),
+	k_thread_create(&alt_thread, alt_stack,


Digression: Seems like this is going to be a persistent error with the new API. If the stack_size argument to k_thread_create() must always be the result of a K_THREAD_STACK_SIZEOF() macro, it probably shouldn't be a size_t named "stack_size" and documented as "stack size in bytes".

A little work could allow the compiler to typecheck this by having a "k_static_stack" type or somesuch.

this is outside the scope of this patch but I'll keep it in mind

andyross · 2017-07-24T18:17:17Z

arch/x86/core/fatal.c

+	.ds = DATA_SEG,
+	.es = DATA_SEG,
+	.fs = DATA_SEG,
+	.gs = DATA_SEG,


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...

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

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.

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

andyross · 2017-07-24T18:21:34Z

scripts/gen_gdt.py

+    return (base_lo, base_mid, base_hi, limit_lo, limit_hi)
+
+
+gdt_ent_fmt = "<HHBBBB"


Style: seems like putting this here is actually less readable than writing the literal into the call to struct.pack() below. Sure, it's a string literal, but semantically this is "code", not a constant you want to separate for maintainability.

i'd rather not, for one thing I use this formatter in two different functions

andyross · 2017-07-24T18:26:50Z

scripts/gen_idt.py

+    sys.stderr.write(os.path.basename(sys.argv[0]) + ": " + text + "\n")
+    sys.exit(1)
+
+irq_gate_desc_format = "<HHBBH"


Style: Similar comment, and a repeat of earlier one: this string isn't a "constant", it's code to be interpreted at runtime. It should go where it's used as a literal, not here.

lpereira

LGTM for the most part. No need to go around fixing the style issues, but the routine to find the symbol table might not work everywhere (given issues to gen_offset.py).

lpereira · 2017-07-24T20:58:38Z

include/arch/x86/arch.h

@@ -106,8 +113,25 @@ typedef struct s_isrList {
 #define NANO_CPU_INT_REGISTER(r, n, p, v, d) \
 	 static ISR_LIST __attribute__((section(".intList"))) \
 			 __attribute__((used)) MK_ISR_NAME(r) = \
-			{&r, n, p, v, d}
+			{&r, n, p, v, d, 0}


Nitpick: With more than 3 items in a struct, I prefer to use designated initializers. Also, while you're at it, please add parenthesis around the macro arguments.

lpereira · 2017-07-24T21:02:11Z

scripts/gen_gdt.py

+
+
+def get_symbols(elf):
+    symbols = elf.get_section_by_name(".symtab")


Will this work with all combinations of Python/pyelftools? Maybe iterating over the section table looking for some SymbolTableSection instance is going to be more robust. The gen_offsets.py script has a function you can copy. Since it raises an exception if the symbol table is absent from the ELF image, the next if can be junked as well.

i'll make this change, but there is no guarantee that other uses of elftools in this or other scripts I am working on will work properlty if people are not using the 0.24 or later version of the library. all of our scripts are run under Python 3, there is no need for worrying about Python 2 suppport

lpereira · 2017-07-24T21:04:11Z

scripts/gen_gdt.py

+    for sym in symbols.iter_symbols():
+        symdict[sym.name] = sym.entry.st_value
+
+    return symdict


Nitpick, but creating symdict and returning it looks nicer with a dict comprehension: return {sym.name: sym.entry.st_value for sym in symbols.iter_symbols()}.

lpereira · 2017-07-24T21:06:36Z

scripts/gen_idt.py

+            if not handler and not tss:
+                error("entry does not specify either handler or tss")
+
+            if (handler):


Style nitpick: unneeded parenthesis. There are other instances of this in this file.

This enables the MMU-based stack protection feature, which will cause a fatal error if a thread overflows its stack in kernel mode, at a nontrivial cost in memory (4K per thread). Signed-off-by: Andrew Boie <[email protected]>

lpereira · 2017-07-24T22:15:45Z

scripts/gen_idt.py

+            error("Vector %d specified, but size of IDT is only %d vectors" %
+                    (vec, max_vec))
+
+        if (vectors[vec] != None):


Stray parenthesis.

I guess this is what happens when you code switch between C and Python many times in a single day

This is one less host tool we have to compile for every build, and makes the build tools more portable across host OSes. The code is also much simpler to maintain. Issue: ZEP-2063 Signed-off-by: Andrew Boie <[email protected]>

Requested changes made

This has one use-case: configuring the double-fault #DF exception handler to do an IA task switch to a special IA task with a known good stack, such that we can dump diagnostic information and then panic. Will be used for stack overflow detection in kernel mode, as otherwise the CPU will triple-fault and reset. Signed-off-by: Andrew Boie <[email protected]>

We will need this for stack memory protection scenarios where a writable GDT with Task State Segment descriptors will be used. The addresses of the TSS segments cannot be put in the GDT via preprocessor magic due to architecture requirments that the address be split up into different fields in the segment descriptor. Signed-off-by: Andrew Boie <[email protected]>

We now create a special IA hardware task for handling double faults. This has a known good stack so that if the kernel tries to push stack data onto an unmapped page, we don't triple-fault and reset the system. Signed-off-by: Andrew Boie <[email protected]>

Signed-off-by: Andrew Boie <[email protected]>

Each member of the array may need to have a padding size added such that the base address of each array element corresponds to the desired stack alignment. This would mean that sizeof(some array element) would return a larger size than what was originally provided. This won't cause problems at runtime since the space is really there, but for users who are only enabling this padding for debug features, they may be surprised when their stacks are effectively smaller than when this was enabled. Signed-off-by: Andrew Boie <[email protected]>

Subsequent patches will set this guard page as unmapped, triggering a page fault on access. If this is due to stack overflow, a double fault will be triggered, which we are now capable of handling with a switch to a know good stack. Signed-off-by: Andrew Boie <[email protected]>

This will trigger a page fault if the guard area is written to. Since the exception itself will try to write to the memory, a double fault will be triggered and we will do an IA task switch to the df_tss and panic. Signed-off-by: Andrew Boie <[email protected]>

Signed-off-by: Andrew Boie <[email protected]>

Show that this mechanism can detect stack overflows with the guard page. We only do it once since are are in an alternate IA HW task after it happens. Signed-off-by: Andrew Boie <[email protected]>

Client update was expecting the properties to be part of the resource object, not resource.properties as it should be. Fixes zephyrproject-rtos#810. Signed-off-by: James Prestwood <[email protected]>

andrewboie requested review from nashif, lpereira, mbolivar, andyross, bboccoqq, cvinayak, carlescufi, galak, AdithyaBaglody and agross-oss July 15, 2017 19:10

andrewboie force-pushed the x86-stackprot branch from a454f7e to 6202ce5 Compare July 15, 2017 19:25

andrewboie force-pushed the x86-stackprot branch from 6202ce5 to 2bb20a5 Compare July 17, 2017 21:42

lpereira suggested changes Jul 18, 2017

View reviewed changes

andrewboie force-pushed the x86-stackprot branch from 2bb20a5 to e1ee4b1 Compare July 18, 2017 02:08

andrewboie force-pushed the x86-stackprot branch from e1ee4b1 to c34da58 Compare July 20, 2017 18:58

andrewboie force-pushed the x86-stackprot branch 3 times, most recently from 2a92ced to 8ce687f Compare July 21, 2017 10:02

nashif previously approved these changes Jul 22, 2017

View reviewed changes

lpereira previously approved these changes Jul 24, 2017

View reviewed changes

andrewboie dismissed stale reviews from lpereira and nashif via ec6982a July 24, 2017 18:23

andrewboie force-pushed the x86-stackprot branch from 8ce687f to ec6982a Compare July 24, 2017 18:23

andyross approved these changes Jul 24, 2017

View reviewed changes

lpereira previously requested changes Jul 24, 2017

View reviewed changes

x86: add CONFIG_X86_STACK_PROTECTION

5223bdb

This enables the MMU-based stack protection feature, which will cause a fatal error if a thread overflows its stack in kernel mode, at a nontrivial cost in memory (4K per thread). Signed-off-by: Andrew Boie <[email protected]>

lpereira reviewed Jul 24, 2017

View reviewed changes

x86: convert gen_idt to Python

df93ce4

This is one less host tool we have to compile for every build, and makes the build tools more portable across host OSes. The code is also much simpler to maintain. Issue: ZEP-2063 Signed-off-by: Andrew Boie <[email protected]>

andrewboie force-pushed the x86-stackprot branch from ec6982a to ddc18e1 Compare July 24, 2017 22:25

Andrew Boie added 11 commits July 24, 2017 16:11

x86: add API for modifying page tables

d7d9c07

Signed-off-by: Andrew Boie <[email protected]>

tests: fatal: fix stack size to k_thread_create

43bfb4f

Signed-off-by: Andrew Boie <[email protected]>

sys_kernel: fix stack declaration

5d087f3

Signed-off-by: Andrew Boie <[email protected]>

qemu_x86: enable MMU stack protection by default

1f63f46

Signed-off-by: Andrew Boie <[email protected]>

tests: fatal: enable x86 MMU stack protection

a61b27d

Show that this mechanism can detect stack overflows with the guard page. We only do it once since are are in an alternate IA HW task after it happens. Signed-off-by: Andrew Boie <[email protected]>

andrewboie force-pushed the x86-stackprot branch from ddc18e1 to a61b27d Compare July 24, 2017 23:11

nashif approved these changes Jul 25, 2017

View reviewed changes

nashif merged commit 416eca5 into zephyrproject-rtos:master Jul 25, 2017

andrewboie deleted the x86-stackprot branch July 25, 2017 15:47

This was referenced Sep 23, 2017

Convert gen_idt to python #3504

Closed

x86: Implement simple stack memory protection #3626

Closed

linker: implement MMU alignment constraints #3740

Closed

zephyrbot mentioned this pull request Sep 16, 2019

arc: hsdk: bug fixes and new feature supports #19173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x86: MMU-based stack overflow protection #810

x86: MMU-based stack overflow protection #810

andrewboie commented Jul 15, 2017

andrewboie commented Jul 15, 2017

lpereira left a comment

lpereira Jul 17, 2017

andrewboie Jul 18, 2017

andyross Jul 24, 2017

lpereira Jul 17, 2017

lpereira Jul 17, 2017

lpereira Jul 17, 2017

lpereira Jul 18, 2017

andrewboie commented Jul 18, 2017

andrewboie commented Jul 20, 2017

andrewboie commented Jul 24, 2017

andyross left a comment

andyross Jul 24, 2017

andyross Jul 24, 2017

andrewboie Jul 24, 2017

andyross Jul 24, 2017

andrewboie Jul 24, 2017

lpereira Jul 24, 2017

andrewboie Jul 24, 2017

andyross Jul 24, 2017

andrewboie Jul 24, 2017

andyross Jul 24, 2017

lpereira left a comment

lpereira Jul 24, 2017

lpereira Jul 24, 2017

andrewboie Jul 24, 2017 •

edited

Loading

lpereira Jul 24, 2017

lpereira Jul 24, 2017

lpereira Jul 24, 2017

andrewboie Jul 24, 2017

		return (base_lo, base_mid, base_hi, limit_lo, limit_hi)


		gdt_ent_fmt = "<HHBBBB"



		def get_symbols(elf):
		symbols = elf.get_section_by_name(".symtab")

x86: MMU-based stack overflow protection #810

x86: MMU-based stack overflow protection #810

Conversation

andrewboie commented Jul 15, 2017

andrewboie commented Jul 15, 2017

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie commented Jul 18, 2017

andrewboie commented Jul 20, 2017

andrewboie commented Jul 24, 2017

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie Jul 24, 2017 •

edited

Loading