fw/syscall: run syscalls on a dedicated stack on ARMv7-M (FIRM-4306)

kernel/arch/arm/arch.c

@@ -141,21 +141,25 @@ void arch_thread_init(struct pbl_thread *t, void (*entry)(void *), void *arg) {
 
 // The port expects the attribute word to hold RASR/RLAR verbatim and derives
 // RBAR from the base: on ARMv7-M the region number and VALID bit are ORed in
-// so no RNR write is needed per region.
+// so no RNR write is needed per region. An empty slot must still carry them:
+// an RBAR write with VALID clear lands on whatever region RNR last selected,
+// i.e. it would wipe the slot programmed just before it.
 void arch_thread_regions_set(struct pbl_thread *t, const MpuRegion *const *regions) {
   for (unsigned int i = 0; i < NUM_MPU_REGIONS; i++) {
     const MpuRegion *r = regions ? regions[i] : NULL;
     uint32_t rbar = 0;
     uint32_t attr = 0;
+#ifndef CONFIG_MPU_TYPE_ARMV8M
+    rbar = MPU_RBAR_VALID_Msk | (FIRST_MPU_REGION + i);
+#endif
     if (r != NULL) {
       KERNEL_ASSERT(r->region_num == FIRST_MPU_REGION + i);
       uint32_t base_reg;
       mpu_get_register_settings(r, &base_reg, &attr);
 #ifdef CONFIG_MPU_TYPE_ARMV8M
       rbar = base_reg;
 #else
-      rbar = (base_reg & ~(MPU_RBAR_VALID_Msk | MPU_RBAR_REGION_Msk)) | MPU_RBAR_VALID_Msk |
-             (FIRST_MPU_REGION + i);
+      rbar |= base_reg & ~(MPU_RBAR_VALID_Msk | MPU_RBAR_REGION_Msk);
 #endif
     }
     t->backend.arch.mpu[2 * i] = rbar;

src/fw/apps/demo/mpu_violation_test/test_mpu_violation.c

@@ -19,10 +19,12 @@
 // Demo app that deliberately runs a series of memory accesses that the
 // MPU is supposed to deny for the unprivileged App task. Use up/down to
 // cycle through tests; press select to run the highlighted test. The
-// expected outcome for every test is a MemManage fault: the kernel
+// expected outcome for most tests is a MemManage fault: the kernel
 // kills the App task and the launcher reclaims the screen. If the app
 // stays alive long enough to render "SURVIVED!" the MPU let the access
-// through -- that's the regression signal.
+// through -- that's the regression signal. Tests marked as expected to
+// survive exercise the other direction: the kernel must keep working
+// where the task's own stack is nearly gone.
 //
 // SimpleMenuLayer would have been a nicer UI but it touches kernel data
 // not accessible to an unprivileged App task, so we use a plain Window
@@ -33,6 +35,7 @@ extern const uint32_t __WORKER_RAM__[];
 extern const uint32_t __FLASH_start__[];
 extern const uint32_t __APP_RAM__[];
 extern const uint32_t __kernel_main_stack_start__[];
+extern const uint32_t __stack_guard_size__[];
 #ifdef CONFIG_SOC_SF32LB52
 extern const uint32_t __ramfunc_start[];
 #endif
@@ -58,6 +61,7 @@ typedef enum {
   TestKind_StackGuardWrite,
 #endif
   TestKind_StackOverflow,
+  TestKind_SyscallNearLimit,
   TestKindCount,
 } TestKind;
 
@@ -78,6 +82,11 @@ static const char *const s_test_titles[TestKindCount] = {
   [TestKind_StackGuardWrite] = "Stack guard W",
 #endif
   [TestKind_StackOverflow] = "Stack overflow",
+  [TestKind_SyscallNearLimit] = "Syscall near limit",
+};
+
+static const bool s_test_expect_survive[TestKindCount] = {
+  [TestKind_SyscallNearLimit] = true,
 };
 
 typedef struct {
@@ -104,6 +113,25 @@ static uint32_t __attribute__((noinline)) prv_overflow_recurse(uint32_t depth) {
 }
 #pragma GCC diagnostic pop
 
+// Leaves only `headroom` bytes of the task stack and then makes a syscall
+// whose privileged call chain needs far more than that. The kernel must
+// run it on its own syscall stack; running it on ours overflows into the
+// stack guard while privileged, which reboots the system.
+static void __attribute__((noinline)) prv_syscall_near_limit(size_t headroom) {
+  volatile uint8_t marker;
+  const uintptr_t stack_base = (uintptr_t)__APP_RAM__ + (uintptr_t)__stack_guard_size__;
+  const uintptr_t sp = (uintptr_t)▮
+  const size_t burn = (sp > stack_base + headroom) ? (sp - stack_base - headroom) : 1;
+  volatile uint8_t pad[burn];
+  for (size_t i = 0; i < burn; i += 32) {
+    pad[i] = (uint8_t)i;
+  }
+  // Loading a system font the app has not used yet walks resource_storage
+  // and the filesystem, several hundred bytes of privileged stack.
+  (void)fonts_get_system_font(FONT_KEY_DROID_SERIF_28_BOLD);
+  marker = pad[0];
+}
+
 static void prv_run_test(TestKind kind) {
   switch (kind) {
     case TestKind_WorkerRamWrite: {
@@ -171,6 +199,11 @@ static void prv_run_test(TestKind kind) {
       // (ARMv7-M), since each call frame consumes ~128 B.
       (void)prv_overflow_recurse(0);
       break;
+    case TestKind_SyscallNearLimit:
+      // Enough for the SVC exception frame (with FP state) and the syscall
+      // wrapper's pushes, nothing more.
+      prv_syscall_near_limit(160);
+      break;
     case TestKindCount:
       break;
   }
@@ -188,9 +221,11 @@ static void prv_attempt(void *cb_data) {
   prv_run_test((TestKind)data->selected_index);
 
   // Reaching this point means no fault. Surface that prominently --
-  // the previous "TESTING..." text gets replaced so the survival is
+  // the previous "TESTING..." text gets replaced so the outcome is
   // obvious in a screenshot.
-  text_layer_set_text(&data->selection_text, "SURVIVED!\n(MPU MISS)");
+  text_layer_set_text(&data->selection_text, s_test_expect_survive[data->selected_index]
+                                                 ? "SURVIVED\n(expected)"
+                                                 : "SURVIVED!\n(MPU MISS)");
   layer_mark_dirty(text_layer_get_layer(&data->selection_text));
   data->test_running = false;
 }

src/fw/kernel/fault_handling.c

@@ -19,6 +19,7 @@
 #include "process_state/worker_state/worker_state.h"
 #include "syscall/syscall.h"
 #include "syscall/syscall_internal.h"
+#include "pbl/util/size.h"
 #include <pbl/logging/logging.h>
 #include "system/reboot_reason.h"
 #include "syscall/syscall.h"
@@ -339,13 +340,17 @@ static void mem_manage_handler_c(unsigned int *stacked_args, unsigned int lr) {
   const uint8_t mmfsr = cfsr & 0xff;
   if (mmfsr & (1 << 7)) {
     uint32_t fault_addr = SCB->MMFAR;
-    MpuRegion mpu_region = mpu_get_region(MemoryRegion_IsrStackGuard);
-    if (memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
-      stack_overflow = true;
-    } else {
-      mpu_region = mpu_get_region(MemoryRegion_TaskStackGuard);
-      if (memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
+    static const uint8_t s_guard_regions[] = {
+      MemoryRegion_IsrStackGuard,
+      MemoryRegion_TaskStackGuard,
+      MemoryRegion_Task4, // syscall stack guard, when the task has one
+    };
+    for (unsigned int i = 0; i < ARRAY_LENGTH(s_guard_regions); i++) {
+      MpuRegion mpu_region = mpu_get_region(s_guard_regions[i]);
+      if (mpu_region.enabled &&
+          memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
         stack_overflow = true;
+        break;
       }
     }
   }

src/fw/kernel/pebble_tasks.c

@@ -297,7 +297,7 @@ struct pbl_thread *pebble_task_create(PebbleTask pebble_task, struct pbl_thread_
   attr->regions[0] = &app_region;
   attr->regions[1] = &worker_region;
   attr->regions[2] = stack_guard_region;
-  attr->regions[3] = NULL;
+  attr->regions[3] = syscall_get_stack_guard_region(pebble_task);
 
   struct pbl_thread *thread = &s_threads[pebble_task];
   PBL_ASSERT(pbl_thread_create(thread, attr) == 0, "Could not start task %s", attr->name);

src/fw/syscall/syscall_internal.c

@@ -5,6 +5,7 @@
 #include "syscall_internal.h"
 
 #include "applib/app_logging.h"
+#include "kernel/memory_layout.h"
 #include "kernel/pebble_tasks.h"
 #include "pbl/mcu/privilege.h"
 #include "process_management/app_manager.h"
@@ -22,13 +23,10 @@
 // Run App/Worker syscalls on a dedicated privileged stack instead of the
 // caller's small unprivileged one, so a task that exhausts its stack faults
 // unprivileged (only that process dies) instead of rebooting the system.
-// Enabled on ARMv8-M (needs PSPLIM); ARMv7-M keeps the old behaviour.
+// ARMv8-M bounds the syscall stack with PSPLIM; ARMv7-M plants a no-access
+// MPU guard below it (MemoryRegion_Task4, see pebble_tasks.c).
 #if !defined(SYSCALL_PRIVILEGED_STACK)
-#if defined(CONFIG_MPU_TYPE_ARMV8M)
 #define SYSCALL_PRIVILEGED_STACK 1
-#else
-#define SYSCALL_PRIVILEGED_STACK 0
-#endif
 #endif
 
 // Per-thread slots for the syscall return address and pre-syscall stack pointer
@@ -272,8 +270,57 @@ void syscall_assert_userspace_buffer(const void *buf, size_t num_bytes) {
 // in the privileged-only .kernel_bss output (RAM): unreadable by app
 // code, zeroed at boot. (Not section(".kernel_bss") -- that would orphan them.)
 #define SYSCALL_STACK_WORDS 512u // 2 KiB each; size against measured high-water.
-static uint32_t s_app_syscall_stack[SYSCALL_STACK_WORDS] __attribute__((aligned(8)));
-static uint32_t s_worker_syscall_stack[SYSCALL_STACK_WORDS] __attribute__((aligned(8)));
+#ifdef CONFIG_MPU_TYPE_ARMV8M
+#define SYSCALL_STACK_GUARD_WORDS 0u
+#else
+#define SYSCALL_STACK_GUARD_WORDS 8u // smallest ARMv7-M MPU region, naturally aligned
+#endif
+
+typedef struct SyscallStack {
+#if SYSCALL_STACK_GUARD_WORDS
+  uint32_t guard[SYSCALL_STACK_GUARD_WORDS];
+#endif
+  uint32_t words[SYSCALL_STACK_WORDS];
+} SyscallStack;
+
+static SyscallStack s_app_syscall_stack __attribute__((aligned(32)));
+static SyscallStack s_worker_syscall_stack __attribute__((aligned(32)));
+
+#if SYSCALL_STACK_GUARD_WORDS
+static const MpuRegion s_app_syscall_stack_guard_region = {
+  .region_num = MemoryRegion_Task4,
+  .enabled = true,
+  .base_address = (uintptr_t)s_app_syscall_stack.guard,
+  .size = sizeof(s_app_syscall_stack.guard),
+  .cache_policy = MpuCachePolicy_NotCacheable,
+  .permissions = MpuPermissions_NoAccess,
+};
+
+static const MpuRegion s_worker_syscall_stack_guard_region = {
+  .region_num = MemoryRegion_Task4,
+  .enabled = true,
+  .base_address = (uintptr_t)s_worker_syscall_stack.guard,
+  .size = sizeof(s_worker_syscall_stack.guard),
+  .cache_policy = MpuCachePolicy_NotCacheable,
+  .permissions = MpuPermissions_NoAccess,
+};
+#endif
+
+const MpuRegion *syscall_get_stack_guard_region(PebbleTask task) {
+#if SYSCALL_STACK_GUARD_WORDS
+  switch (task) {
+    case PebbleTask_App:
+      return &s_app_syscall_stack_guard_region;
+    case PebbleTask_Worker:
+      return &s_worker_syscall_stack_guard_region;
+    default:
+      break;
+  }
+#else
+  (void)task;
+#endif
+  return NULL;
+}
 
 // Kernel hook: top of the current task's dedicated syscall stack (base in
 // *base_out), or NULL to keep it on the caller's stack. App + Worker only;
@@ -290,10 +337,10 @@ uint32_t *pbl_kernel_syscall_stack(uintptr_t *base_out) {
       }
     }
 #endif
-      stack = s_app_syscall_stack;
+      stack = s_app_syscall_stack.words;
       break;
     case PebbleTask_Worker:
-      stack = s_worker_syscall_stack;
+      stack = s_worker_syscall_stack.words;
       break;
     default:
       return NULL;
@@ -310,8 +357,8 @@ static bool prv_psp_in_syscall_stack(uintptr_t psp, const uint32_t *stack) {
 // packed as (psplim << 32 | sp) to return in r0:r1; 0 = no switch needed.
 USED uint64_t syscall_stack_restore_target(void) {
   const uintptr_t psp = __get_PSP();
-  if (prv_psp_in_syscall_stack(psp, s_app_syscall_stack) ||
-      prv_psp_in_syscall_stack(psp, s_worker_syscall_stack)) {
+  if (prv_psp_in_syscall_stack(psp, s_app_syscall_stack.words) ||
+      prv_psp_in_syscall_stack(psp, s_worker_syscall_stack.words)) {
     const uint32_t sp = (uint32_t)prv_get_syscall_sp(); // slot1 = pre-syscall task SP
     struct pbl_thread_stack_info info;
     pbl_thread_stack_info(pbl_thread_current(), &info);
@@ -332,11 +379,11 @@ static uint16_t prv_syscall_stack_free_bytes(const uint32_t *stack) {
 }
 
 uint16_t syscall_app_stack_free_bytes(void) {
-  return prv_syscall_stack_free_bytes(s_app_syscall_stack);
+  return prv_syscall_stack_free_bytes(s_app_syscall_stack.words);
 }
 
 uint16_t syscall_worker_stack_free_bytes(void) {
-  return prv_syscall_stack_free_bytes(s_worker_syscall_stack);
+  return prv_syscall_stack_free_bytes(s_worker_syscall_stack.words);
 }
 
 // Drop privilege and return to the task. If the syscall ran on a dedicated
@@ -354,8 +401,10 @@ EXTERNALLY_VISIBLE void NAKED_FUNC USED prv_drop_privilege(void) {
       " mov r12, r0 \n"                     // r12 = real LR (caller-saved; no bl follows)
       " pop {r0, r1} \n"                    // r0,r1 = syscall return value
       " cbz r2, 1f \n"                      // skip stack switch if not relocated
-      " msr psp, r2 \n"    // back to the app stack (higher addr; safe vs low limit)
+      " msr psp, r2 \n" // back to the app stack (higher addr; safe vs low limit)
+#ifdef CONFIG_MPU_TYPE_ARMV8M
       " msr psplim, r3 \n" // restore the app stack limit
+#endif
       " isb \n"
       "1: \n"
       " mrs r2, control \n" // drop privilege: CONTROL.nPRIV = 1
@@ -366,6 +415,10 @@ EXTERNALLY_VISIBLE void NAKED_FUNC USED prv_drop_privilege(void) {
   );
 }
 #else
+const MpuRegion *syscall_get_stack_guard_region(PebbleTask task) {
+  (void)task;
+  return NULL;
+}
 uint16_t syscall_app_stack_free_bytes(void) {
   return 0xFFFF;
 }

src/fw/syscall/syscall_internal.h

@@ -3,8 +3,11 @@
 
 #pragma once
 
+#include "kernel/pebble_tasks.h"
 #include "pbl/util/attributes.h"
 
+#include <pbl/drivers/mpu.h>
+
 #include <stdbool.h>
 #include <stdint.h>
 #include <stddef.h>
@@ -58,6 +61,10 @@ bool mcu_call_unprivileged_reentry_setup(uintptr_t orig_sp, uintptr_t *lr_ptr);
 uint16_t syscall_app_stack_free_bytes(void);
 uint16_t syscall_worker_stack_free_bytes(void);
 
+//! No-access MPU region guarding the bottom of @p task's syscall stack, or
+//! NULL when the platform bounds it some other way (PSPLIM) or has none.
+const MpuRegion *syscall_get_stack_guard_region(PebbleTask task);
+
 // Test overrides.
 // TODO: really implement privilege escalation in unit tests. See PBL-9688
 #if defined(UNITTEST)