Merge pull request #169 from f9micro/overflow-protection

Implement stack overflow protection with canary checks

Author: jserv

include/platform/cortex_m.h

@@ -104,6 +104,7 @@ inline void __ISB(void) {
 #define SCB_VTOR                        (volatile uint32_t *) (SCB_BASE + 0x008)                /* Vector Table Offset Register */
 #define SCB_AIRCR                       (volatile uint32_t *) (SCB_BASE + 0x00c)                /* Application Interrupt/Reset Control Register */
 #define SCB_SCR                         (volatile uint32_t *) (SCB_BASE + 0x010)                /* System Control Register */
+#define SCB_CCR                         (volatile uint32_t *) (SCB_BASE + 0x014)                /* Configuration and Control Register */
 #define SCB_SHPR                        (volatile uint8_t *)  (SCB_BASE + 0x018)                /* System Handler Priority Register */
 #define SCB_SHCSR                       (volatile uint32_t *) (SCB_BASE + 0x024)                /* System Handler Control and State Register */
 #define SCB_CFSR                        (volatile uint32_t *) (SCB_BASE + 0x028)                /* Configurable fault status register - Describes Usage, Bus, and Memory faults */
@@ -143,6 +144,8 @@ inline void __ISB(void) {
 #define SCB_SCR_SLEEPDEEP               (uint32_t) (1 << 2)                                     /* Use deep sleep as low power mode */
 #define SCB_SCR_SEVONPEND               (uint32_t) (1 << 4)                                     /* Send event on pending exception */
 
+#define SCB_CCR_STKALIGN                (uint32_t) (1 << 9)                                     /* Stack 8-byte alignment on exception entry */
+
 #define SCB_SHCSR_MEMFAULTENA           (uint32_t) (1 << 16)                                    /* Enables Memory Management Fault */
 #define SCB_SHCSR_BUSFAULTENA           (uint32_t) (1 << 17)                                    /* Enables Bus Fault */
 #define SCB_SHCSR_USEFAULTENA           (uint32_t) (1 << 18)                                    /* Enables Usage Fault */

include/platform/irq.h

@@ -9,6 +9,7 @@
 #include <softirq.h>
 #include <thread.h>
 #include <sched.h>
+#include <error.h>
 #include <platform/link.h>
 #include <platform/cortex_m.h>
 
@@ -168,7 +169,24 @@ extern volatile uint32_t __irq_saved_regs[8];
 	{								\
 		register tcb_t *sel;					\
 		sel = schedule_select();				\
+		/* Check current thread canary before any return path.	\
+		 * Catches overflow that occurred while thread ran. */	\
+		if (!thread_check_canary((tcb_t *)current)) {		\
+			panic("Stack overflow (current): tid=%t, "	\
+			      "stack_base=%p, canary=%p\n",		\
+			      current->t_globalid, current->stack_base,	\
+			      current->stack_base ?			\
+			      *((uint32_t *)current->stack_base) : 0);	\
+		}							\
 		if (sel != current) {					\
+			/* Check next thread before switching to it */	\
+			if (!thread_check_canary(sel)) {		\
+				panic("Stack overflow (next): tid=%t, "	\
+				      "stack_base=%p, canary=%p\n",	\
+				      sel->t_globalid, sel->stack_base,	\
+				      sel->stack_base ?			\
+				      *((uint32_t *)sel->stack_base) : 0); \
+			}						\
 			context_switch(current, sel);			\
 		} else {						\
 			/* No context switch - restore saved registers	\

include/thread.h

@@ -17,6 +17,11 @@
  * @file thread.h
  * @brief Thread dispatcher definitions
  *
+ * Stack Overflow Protection:
+ * A canary value is placed at the bottom of each thread's stack (lowest
+ * address, since ARM stacks grow downward). The canary is checked on
+ * every context switch - if corrupted, the stack has overflowed.
+ *
  * Thread ID type is declared in @file types.h and called l4_thread_t
  *
  * For Global Thread ID only high 18 bits are used and lower are reserved,
@@ -38,6 +43,11 @@
 
 #define THREAD_BY_TID(id)	 thread_by_globalid(TID_TO_GLOBALID(id))
 
+/* Stack canary for overflow detection.
+ * Placed at stack_base (lowest address). Checked on context switch.
+ */
+#define STACK_CANARY		0xDEADBEEF
+
 typedef enum {
 	THREAD_IDLE,
 	THREAD_KERNEL,
@@ -98,6 +108,23 @@ struct tcb {
 };
 typedef struct tcb tcb_t;
 
+/* Initialize stack canary at bottom of stack.
+ * Must be called after stack_base is set.
+ */
+static inline void thread_init_canary(tcb_t *thr)
+{
+	if (thr->stack_base)
+		*((uint32_t *) thr->stack_base) = STACK_CANARY;
+}
+
+/* Check stack canary. Returns 1 if valid, 0 if corrupted. */
+static inline int thread_check_canary(tcb_t *thr)
+{
+	if (!thr->stack_base)
+		return 1;  /* No stack tracking, skip check */
+	return *((uint32_t *) thr->stack_base) == STACK_CANARY;
+}
+
 void thread_init_subsys(void);
 
 tcb_t *thread_by_globalid(l4_thread_t globalid);

kernel/ipc.c

@@ -287,8 +287,23 @@ void sys_ipc(uint32_t *param1)
 				           "IPC: %t thread start sp:%p stack_size:%p\n",
 				           to_tid, sp, stack_size);
 
+				/* Security check: Ensure stack is in user-writable memory */
+				int pid = mempool_search(sp - stack_size, stack_size);
+				mempool_t *mp = mempool_getbyid(pid);
+
+				if (!mp || !(mp->flags & MP_UW)) {
+					dbg_printf(DL_IPC,
+					           "IPC: REJECT invalid stack for %t: %p (pool %s)\n",
+					           to_tid, sp - stack_size, mp ? mp->name : "N/A");
+					user_ipc_error(caller,
+					               UE_IPC_ABORTED | UE_IPC_PHASE_SEND);
+					caller->state = T_RUNNABLE;
+					return;
+				}
+
 				to_thr->stack_base = sp - stack_size;
 				to_thr->stack_size = stack_size;
+				thread_init_canary(to_thr);
 
 				dbg_printf(DL_IPC,
 				           "IPC: %t stack_base:%p stack_size:%p\n",

kernel/kdb.c

@@ -23,6 +23,7 @@ extern void kdb_dump_ktable(void);
 extern void kdb_show_ktimer(void);
 extern void kdb_dump_softirq(void);
 extern void kdb_dump_threads(void);
+extern void kdb_dump_stacks(void);
 extern void kdb_dump_mpu(void);
 extern void kdb_dump_mempool(void);
 extern void kdb_dump_as(void);
@@ -60,6 +61,12 @@ struct kdb_t kdb_functions[] = {
 		.menuentry = "dump threads",
 		.function = kdb_dump_threads
 	},
+	{
+		.option = 'S',
+		.name = "STACKS",
+		.menuentry = "dump stack usage",
+		.function = kdb_dump_stacks
+	},
 	{
 		.option = 'M',
 		.name = "MPU",

kernel/memory.c

@@ -11,6 +11,7 @@
 #include <fpage_impl.h>
 #include <init_hook.h>
 #include <kip.h>
+#include <platform/cortex_m.h>
 
 /**
  * @file    memory.c
@@ -181,6 +182,15 @@ void memory_init()
 	int j = 0;
 	uint32_t *shcsr = (uint32_t *) 0xE000ED24;
 
+	/* Verify and set STKALIGN for 8-byte stack alignment on exceptions.
+	 * This is critical for Cortex-M: misaligned stacks cause HardFault.
+	 * STKALIGN (bit 9 of CCR) ensures automatic 8-byte alignment.
+	 */
+	if (!(*SCB_CCR & SCB_CCR_STKALIGN)) {
+		*SCB_CCR |= SCB_CCR_STKALIGN;
+		__ISB();
+	}
+
 	fpages_init();
 
 	ktable_init(&as_table);

kernel/systhread.c

@@ -41,6 +41,7 @@ void create_root_thread(void)
 	root->stack_base = (memptr_t) &root_stack_start;
 	root->stack_size = (uint32_t) &root_stack_end -
 	                   (uint32_t) &root_stack_start;
+	thread_init_canary(root);
 
 	sched_slot_dispatch(SSI_ROOT_THREAD, root);
 	root->state = T_RUNNABLE;
@@ -52,6 +53,14 @@ void create_kernel_thread(void)
 
 	thread_init_kernel_ctx(&kernel_stack_end, kernel);
 
+	/* Initialize stack tracking for kernel thread.
+	 * Kernel stack follows idle stack in memory layout.
+	 */
+	kernel->stack_base = (memptr_t) &idle_stack_end;
+	kernel->stack_size = (uint32_t) &kernel_stack_end -
+	                     (uint32_t) &idle_stack_end;
+	thread_init_canary(kernel);
+
 	/* This will prevent running other threads
 	 * than kernel until it will be initialized
 	 */
@@ -64,6 +73,11 @@ void create_idle_thread(void)
 	idle = thread_init(TID_TO_GLOBALID(THREAD_IDLE), NULL);
 	thread_init_ctx((void *) &idle_stack_end, idle_thread, NULL, idle);
 
+	idle->stack_base = (memptr_t) &idle_stack_start;
+	idle->stack_size = (uint32_t) &idle_stack_end -
+	                   (uint32_t) &idle_stack_start;
+	thread_init_canary(idle);
+
 	sched_slot_dispatch(SSI_IDLE, idle);
 	idle->state = T_RUNNABLE;
 }

kernel/thread.c

@@ -441,6 +441,15 @@ void thread_switch(tcb_t *thr)
 	assert((intptr_t) thr);
 	assert(thread_isrunnable(thr));
 
+	/* Check stack canary before switching to this thread.
+	 * If canary is corrupted, the thread's stack has overflowed.
+	 */
+	if (!thread_check_canary(thr)) {
+		panic("Stack overflow: tid=%t, stack_base=%p, canary=%p\n",
+		      thr->t_globalid, thr->stack_base,
+		      thr->stack_base ? *((uint32_t *) thr->stack_base) : 0);
+	}
+
 	current = thr;
 	current_utcb = thr->utcb;
 	if (current->as)
@@ -528,4 +537,39 @@ void kdb_dump_threads(void)
 	}
 }
 
+void kdb_dump_stacks(void)
+{
+	tcb_t *thr;
+	int idx;
+
+	dbg_printf(DL_KDB, "%5s %8s %10s %10s %10s %6s\n",
+	           "type", "global", "stack_base", "stack_size", "sp", "canary");
+
+	for_each_in_ktable(thr, idx, (&thread_table)) {
+		char *canary_status;
+		uint32_t canary_val = 0;
+
+		if (!thr->stack_base) {
+			canary_status = "N/A";
+		} else {
+			canary_val = *((uint32_t *) thr->stack_base);
+			canary_status = (canary_val == STACK_CANARY) ? "OK" : "FAIL";
+		}
+
+		dbg_printf(DL_KDB, "%5s %t %p %10d %p %6s\n",
+		           kdb_get_thread_type(thr),
+		           thr->t_globalid,
+		           thr->stack_base,
+		           thr->stack_size,
+		           thr->ctx.sp,
+		           canary_status);
+
+		/* If canary failed, show what was found */
+		if (thr->stack_base && canary_val != STACK_CANARY) {
+			dbg_printf(DL_KDB, "      expected: %p, found: %p\n",
+			           STACK_CANARY, canary_val);
+		}
+	}
+}
+
 #endif	/* CONFIG_KDB */