Hardened AP Stack Management (#631)

* Hardened AP Stack Management (see PR for details)

* Fix low-memory boot issues caused by the larger AP stack area
 Zero only _bss.._stacks at first boot (the full sweep wiped ACPI/SMBIOS anchors above 0xE0000 on writable-shadow platforms), verify relocation addresses against pm_map, and relocate before smp_start() when the enabled CPUs' stacks would extend beyond usable low RAM, setting up the AP trampoline at start time.

app/display.c

@@ -6,6 +6,7 @@
 #include <stdint.h>
 
 #include "cpuid.h"
+#include "cpulocal.h"
 #include "cpuinfo.h"
 #include "hwctrl.h"
 #include "i2c_x86.h"
@@ -15,6 +16,7 @@
 #include "serial.h"
 #include "pmem.h"
 #include "smbios.h"
+#include "smp.h"
 #include "spd.h"
 #include "temperature.h"
 #include "tsc.h"
@@ -539,16 +541,20 @@ void scroll(void)
     if (scroll_message_row < ROW_SCROLL_B) {
         scroll_message_row++;
     } else {
-        if (scroll_lock) {
-            display_footer_message("<Enter> Single step     ");
-        }
-        scroll_wait = true;
-        do {
-            check_input();
-        } while (scroll_wait && scroll_lock);
+        // Only the master CPU may poll the keyboard, so the scroll-lock
+        // single-step wait is only available to it.
+        if (smp_my_cpu_num() == master_cpu) {
+            if (scroll_lock) {
+                display_footer_message("<Enter> Single step     ");
+            }
+            scroll_wait = true;
+            do {
+                check_input();
+            } while (scroll_wait && scroll_lock);
 
-        scroll_wait = false;
-        clear_footer_message();
+            scroll_wait = false;
+            clear_footer_message();
+        }
         scroll_screen_region(ROW_SCROLL_T, 0, ROW_SCROLL_B, SCREEN_WIDTH - 1);
     }
 }
@@ -632,6 +638,15 @@ void do_tick(int my_cpu)
     // This only tick one time per second
     if (!timed_update_done) {
 
+        // A corrupted stack canary means a CPU overran its stack slot and may
+        // have corrupted the thread-local barrier flags below it (see boot.h).
+        static int last_overflow_cpu = -1;
+        int overflow_cpu = stack_canary_check();
+        if (overflow_cpu >= 0 && overflow_cpu != last_overflow_cpu) {
+            last_overflow_cpu = overflow_cpu;
+            do_trace(overflow_cpu, "CPU stack overflow detected - test results are unreliable");
+        }
+
         // Display FAIL banner if (new) errors detected
         if (err_banner_redraw && !big_status_displayed && error_count > 1) {
             display_big_status(false);

app/error.c

@@ -14,6 +14,7 @@
 #include <limits.h>
 
 #include "smp.h"
+
 #include "vmem.h"
 
 #include "badram.h"
@@ -276,7 +277,11 @@ static void common_err(error_type_t type, uintptr_t addr, testword_t good, testw
 #endif
         }
         if (new_address) {
-            check_input();
+            // Only the master CPU may poll the keyboard: polling from other
+            // CPUs would race the master's polling.
+            if (smp_my_cpu_num() == master_cpu) {
+                check_input();
+            }
             scroll();
 
             set_foreground_colour(YELLOW);

app/loongarch/interrupt.c

@@ -186,7 +186,9 @@ void interrupt(struct system_context *system_context)
 
     ecode = (system_context->estat >> 16) & 0x3F;
 
-    spin_lock(error_mutex);
+    // Don't wait for the error mutex: it may be held by a stopped CPU, or by
+    // this very CPU if the interrupt was taken inside the error reporting path.
+    spin_trylock(error_mutex);
 
     clear_message_area();
 

app/main.c

@@ -21,6 +21,7 @@
 #include "acpi.h"
 #include "cache.h"
 #include "cpuid.h"
+#include "cpulocal.h"
 #include "cpuinfo.h"
 #include "heap.h"
 #include "hwctrl.h"
@@ -177,10 +178,50 @@ static void run_at(uintptr_t addr, int my_cpu)
 #endif
 }
 
+// A BSP-only version of run_at(), used before the APs have been started. The
+// thread-local storage is zeroed, as its original copy may not be backed by RAM.
+static void relocate_to(uintptr_t addr)
+{
+    uintptr_t *new_start_addr = (uintptr_t *)(addr + startup - _start);
+
+    // Copy the program code and all data except the stacks.
+    memmove((void *)addr, (void *)_start, _stacks - _start);
+    // Zero the thread-local storage.
+    size_t locals_offset = _stacks - _start + BSP_STACK_SIZE - LOCALS_SIZE;
+    for (int cpu_num = 0; cpu_num < num_available_cpus; cpu_num++) {
+        memset((void *)(addr + locals_offset), 0, LOCALS_SIZE);
+        locals_offset += AP_STACK_SIZE;
+    }
+
+    // Jump to new_start_addr.
+#ifdef __i386__
+    // The 32-bit startup code needs to know where it is located.
+    __asm__ __volatile__("movl %0, %%edi; jmp *%0" : : "r" (new_start_addr));
+    __builtin_unreachable();
+#else
+    ((void (*)(void))new_start_addr)();
+#endif
+}
+
+// Checks that the given address range lies entirely within a single region of
+// usable RAM (the BIOS bootloader may have loaded us straddling the VGA/ROM hole).
+static bool addr_range_is_usable(uintptr_t start, size_t size)
+{
+    for (int i = 0; i < pm_map_size; i++) {
+        uintptr_t region_start = pm_map[i].start << PAGE_SHIFT;
+        uintptr_t region_end   = pm_map[i].end   << PAGE_SHIFT;
+        if (start >= region_start && (start + size) <= region_end) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static bool set_load_addr(uintptr_t *load_addr, size_t program_size, uintptr_t lower_limit, uintptr_t upper_limit)
 {
     uintptr_t current_start = (uintptr_t)_start;
-    if (current_start >= lower_limit && (current_start + program_size) <= upper_limit) {
+    if (current_start >= lower_limit && (current_start + program_size) <= upper_limit
+        && addr_range_is_usable(current_start, program_size)) {
         *load_addr = current_start;
         return true;
     }
@@ -206,6 +247,13 @@ static bool set_load_addr(uintptr_t *load_addr, size_t program_size, uintptr_t l
 
 static void global_init(void)
 {
+    // Set once initialisation is complete; the early relocation below
+    // restarts the program and re-enters here.
+    static bool init_complete = false;
+    if (init_complete) {
+        return;
+    }
+
     floppy_off();
 
     cpuid_init();
@@ -335,6 +383,20 @@ static void global_init(void)
     start_run = true;
     dummy_run = true;
     restart = false;
+
+    init_complete = true;
+
+    // If the bootloader placed us so that the stack area extends beyond usable RAM,
+    // move before the APs start; avoid a target that overlaps the running code.
+    uintptr_t current_start = (uintptr_t)_start;
+    if (!addr_range_is_usable(current_start, program_size)) {
+        uintptr_t target = low_load_addr;
+        if (target < (current_start + program_size) && current_start < (target + program_size)) {
+            target = high_load_addr;
+        }
+        trace(0, "relocating to %0*x before starting CPUs", 2*sizeof(uintptr_t), target);
+        relocate_to(target);
+    }
 }
 
 static void ap_enumerate(int my_cpu)
@@ -452,6 +514,10 @@ static void test_all_windows(int my_cpu)
         }
     }
     if (i_am_master) {
+        // CPUs not taking part in this test won't re-arm their stack
+        // canaries, and the coming relocations will invalidate them.
+        stack_canary_disarm_all();
+
         num_active_cpus = 1;
         if (!dummy_run) {
             if (parallel_test) {

app/x86/interrupt.c

@@ -202,7 +202,9 @@ ISR_GP_REGS_ONLY void interrupt(struct trap_regs *trap_regs)
         }
     }
 
-    spin_lock(error_mutex);
+    // Don't wait for the error mutex: it may be held by a stopped CPU, or by
+    // this very CPU if the interrupt was taken inside the error reporting path.
+    spin_trylock(error_mutex);
 
     clear_message_area();
 

boot/boot.h

@@ -19,11 +19,16 @@
 #define	MAX_APS		511		/* Maximum number of active APs */
 
 #define BSP_STACK_SIZE	16384		/* Stack size for the BSP */
-#ifdef __loongarch_lp64
-#define AP_STACK_SIZE	2048		/* Stack size for each AP */
-#else
-#define AP_STACK_SIZE	1024		/* Stack size for each AP */
-#endif
+
+/*
+ * Stack size for each AP. Each CPU's thread-local storage (the barrier
+ * waiting flags) occupies the top LOCALS_SIZE bytes of its own stack slot,
+ * directly below the bottom of the next CPU's stack: an AP overrunning its
+ * stack silently corrupts its neighbour's flags and freezes the whole test.
+ * 1024 bytes proved too small for the error reporting path; the canary in
+ * system/cpulocal.c turns any future overrun into a visible report.
+ */
+#define AP_STACK_SIZE	2048
 
 #define	STACKS_SIZE	(BSP_STACK_SIZE + MAX_APS * AP_STACK_SIZE)
 

boot/x86/startup32.S

@@ -167,13 +167,14 @@ init_idt_descr:
 
 	lidt	idt_descr@GOTOFF(%ebx)
 
-	# Zero the BSS (if first boot).
+	# Zero the BSS (if first boot). The stack area is excluded: it needs no
+	# init and, when loaded in low memory, may extend into the video/ROM region.
 
 	cmpl	$1, first_boot@GOTOFF(%ebx)
 	jne	1f
 	xorl	%eax, %eax
 	leal	_bss@GOTOFF(%ebx), %edi
-	leal	_end@GOTOFF(%ebx), %ecx
+	leal	_stacks@GOTOFF(%ebx), %ecx
 	subl	%edi, %ecx
 0:	movl	%eax, (%edi)
 	addl	$4, %edi

boot/x86/startup64.S

@@ -253,13 +253,17 @@ flush:	movw	$KERNEL_DS, %ax
 
 	lidt	idt_descr(%rip)
 
-	# Zero the BSS (if first boot).
+	# Zero the BSS (if first boot). The stack area is excluded: it doesn't
+	# need initialising (the thread-local storage it contains is always
+	# written before use), and when loaded in low memory it can extend
+	# beyond the end of usable RAM, into the video/ROM region, where the
+	# ACPI and SMBIOS structures would be destroyed by the write sweep.
 
 	cmpl	$1, first_boot(%rip)
 	jne	1f
 	xorq	%rax, %rax
 	leaq	_bss(%rip), %rdi
-	leaq	_end(%rip), %rcx
+	leaq	_stacks(%rip), %rcx
 	subq	%rdi, %rcx
 0:	movq	%rax, (%rdi)
 	addq	$8, %rdi

lib/spinlock.h

@@ -70,6 +70,18 @@ static inline void spin_lock(spinlock_t *lock)
     }
 }
 
+/**
+ * Locks the mutex if it is not currently locked, without waiting. Returns
+ * true if the mutex was locked by this call, false if it was already held.
+ */
+static inline bool spin_trylock(spinlock_t *lock)
+{
+    if (lock) {
+        return __sync_bool_compare_and_swap(lock, false, true);
+    }
+    return true;
+}
+
 /**
  * Unlocks the mutex.
  */

system/cpulocal.c

@@ -2,17 +2,43 @@
 // Copyright (C) 2022 Martin Whitaker.
 
 #include <stdbool.h>
+#include <stdint.h>
 
 #include "boot.h"
+#include "smp.h"
 
 #include "cpulocal.h"
 
+//------------------------------------------------------------------------------
+// Constants
+//------------------------------------------------------------------------------
+
+// An arbitrary value that is unlikely to appear in a stack frame.
+#define STACK_CANARY    UINT32_C(0x446D6153)
+
 //------------------------------------------------------------------------------
 // Variables
 //------------------------------------------------------------------------------
 
 int local_bytes_used = 0;
 
+// Whether each stack canary is currently valid. The stack area is not
+// preserved across program relocations, hence the arm/disarm cycle.
+static bool canary_armed[MAX_CPUS];
+
+//------------------------------------------------------------------------------
+// Private Functions
+//------------------------------------------------------------------------------
+
+static uint32_t *stack_canary_addr(int cpu_num)
+{
+    uintptr_t slot_bottom = (uintptr_t)_stacks;
+    if (cpu_num > 0) {
+        slot_bottom += BSP_STACK_SIZE + (uintptr_t)(cpu_num - 1) * AP_STACK_SIZE;
+    }
+    return (uint32_t *)slot_bottom;
+}
+
 //------------------------------------------------------------------------------
 // Public Functions
 //------------------------------------------------------------------------------
@@ -24,3 +50,29 @@ int allocate_local_flag(void)
     }
     return local_bytes_used += sizeof(bool);
 }
+
+void stack_canary_arm(int cpu_num)
+{
+    if (cpu_num < 0 || cpu_num >= MAX_CPUS) {
+        return;
+    }
+    *stack_canary_addr(cpu_num) = STACK_CANARY;
+    canary_armed[cpu_num] = true;
+}
+
+void stack_canary_disarm_all(void)
+{
+    for (int i = 0; i < MAX_CPUS; i++) {
+        canary_armed[i] = false;
+    }
+}
+
+int stack_canary_check(void)
+{
+    for (int i = 0; i < num_available_cpus; i++) {
+        if (canary_armed[i] && *stack_canary_addr(i) != STACK_CANARY) {
+            return i;
+        }
+    }
+    return -1;
+}

system/cpulocal.h

@@ -43,4 +43,21 @@ static inline local_flag_t *local_flags(int flag_num)
     return (local_flag_t *)(_stacks + BSP_STACK_SIZE - LOCALS_SIZE + flag_num);
 }
 
+/**
+ * Writes a canary value at the bottom of the given CPU's stack slot and
+ * marks it as armed. Must be called again after each program relocation.
+ */
+void stack_canary_arm(int cpu_num);
+
+/**
+ * Marks all stack canaries as no longer valid.
+ */
+void stack_canary_disarm_all(void);
+
+/**
+ * Checks all armed stack canaries. Returns the number of the first CPU
+ * whose canary has been overwritten, or -1 if all are intact.
+ */
+int stack_canary_check(void);
+
 #endif // CPULOCAL_H

system/smp.c

@@ -1076,14 +1076,8 @@ void smp_init(bool smp_enable)
     smp_heap_page = heap_alloc(HEAP_TYPE_LM_1, PAGE_SIZE, PAGE_SIZE) >> PAGE_SHIFT;
 
 #if defined(__i386__) || defined(__x86_64__)
-    ap_startup_addr = (uintptr_t)startup;
-
-    size_t ap_trampoline_size = ap_trampoline_end - ap_trampoline;
-    memcpy((uint8_t *)HEAP_BASE_ADDR, ap_trampoline, ap_trampoline_size);
-
-    alloc_addr = HEAP_BASE_ADDR + ap_trampoline_size;
+    alloc_addr = HEAP_BASE_ADDR + (ap_trampoline_end - ap_trampoline);
 #elif defined(__loongarch_lp64)
-    ap_startup_addr = (uintptr_t)startup64;
     alloc_addr = HEAP_BASE_ADDR;
 #endif
 }
@@ -1092,6 +1086,16 @@ int smp_start(cpu_state_t cpu_state[MAX_CPUS])
 {
     int cpu_num;
 
+    // Set up the AP startup vector here rather than in smp_init(): the program
+    // may have been relocated in between, and the APs must enter the running copy.
+#if defined(__i386__) || defined(__x86_64__)
+    ap_startup_addr = (uintptr_t)startup;
+
+    memcpy((uint8_t *)HEAP_BASE_ADDR, ap_trampoline, ap_trampoline_end - ap_trampoline);
+#elif defined(__loongarch_lp64)
+    ap_startup_addr = (uintptr_t)startup64;
+#endif
+
     cpu_state[0] = CPU_STATE_RUNNING;  // we don't support disabling the boot CPU
 
     for (cpu_num = 1; cpu_num < num_available_cpus; cpu_num++) {

tests/tests.c

@@ -15,6 +15,7 @@
 
 #include "cache.h"
 #include "cpuid.h"
+#include "cpulocal.h"
 #include "memsize.h"
 #include "simd.h"
 #include "tsc.h"
@@ -122,6 +123,10 @@ void test_list_init(void)
 
 int run_test(int my_cpu, int test, int stage, int iterations)
 {
+    // (Re)arm the canary guarding against this CPU overrunning its stack.
+    // Needed on every call: relocation invalidates the stack area.
+    stack_canary_arm(my_cpu);
+
     if (my_cpu == master_cpu) {
         if (window_num == 0) {
             // First window, so we need to test all selected lower memory.