From cd3ad174e5e345eb61b3fa4572a33173c9b8b9f6 Mon Sep 17 00:00:00 2001 From: Gerard Marull-Paretas Date: Wed, 16 Sep 2026 13:57:22 +0200 Subject: [PATCH 1/3] kernel: keep empty MPU slots from wiping the previous region arch_thread_regions_set() encodes an unused per-thread region slot as RBAR=0 / RASR=0. On ARMv7-M the context switch streams the four slots through MPU_RBAR..MPU_RASR_A3, and an RBAR write with VALID clear does not change RNR: it updates the region RNR already points at, i.e. the one the previous slot just programmed. For App and Worker the empty slot 3 therefore lands on region 6 and its RASR=0 disables the task stack guard. The FreeRTOS port ORed VALID|region into every slot, which is why v4.36 firmware still faults on the guard while v4.37+ lets an app stack overflow walk into the kernel heap unnoticed. Keep VALID|region on empty slots (RASR stays 0, so the region is simply disabled). ARMv8-M selects the region through RNR and the aliases and is unaffected. Observed on qemu_flint: with the App thread running, region 6 read back as RBAR=0x6 RASR=0 while the thread's saved MPU words held the guard. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Gerard Marull-Paretas --- kernel/arch/arm/arch.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/kernel/arch/arm/arch.c b/kernel/arch/arm/arch.c index 43a4a12fd9..b01aa55e92 100644 --- a/kernel/arch/arm/arch.c +++ b/kernel/arch/arm/arch.c @@ -141,12 +141,17 @@ 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; @@ -154,8 +159,7 @@ void arch_thread_regions_set(struct pbl_thread *t, const MpuRegion *const *regio #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; From d95651ecb95ebadef235605838392d1489885a0b Mon Sep 17 00:00:00 2001 From: Gerard Marull-Paretas Date: Wed, 16 Sep 2026 13:57:23 +0200 Subject: [PATCH 2/3] fw/syscall: run syscalls on the dedicated stack on ARMv7-M too be4d98df1 gave App/Worker syscalls their own 2 KiB privileged stack but only on ARMv8-M, because the relocation bounded that stack with PSPLIM. ARMv7-M apps kept running the whole privileged call chain on their 2 KiB task stack, and asterix reboots with StackOverflow / HardFault when a text-heavy watchface renders a glyph that misses the cache: text layout -> text_resources -> sys_resource_load_range -> resource_storage -> pfs_open -> FTL -> flash consumes ~1.3 KB on top of whatever the app already used, and the overflow happens privileged, so the kernel cannot recover and resets. Enable the relocation everywhere. The kernel side already handles the missing PSPLIM; on ARMv7-M the syscall stacks get a 32 B no-access MPU region below them instead, programmed through the free fourth per-task slot (MemoryRegion_Task4). The fault handler treats a hit in that region like the other stack guards, so a genuine kernel-side over-budget still reboots with StackOverflow, while an app that runs out of its own stack now faults unprivileged and is killed. Verified on qemu_flint with a stress app that burns N bytes of stack and then draws text in a custom font: with the guard alone, 512 B of extra app usage reboots the emulator (the device failure mode); with this change the same load succeeds (app peak 1968/2016 B, syscall stack peak 608 B) and 768 B only kills the app. WeatherGraph from the app store runs unchanged. Fixes FIRM-4306 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Gerard Marull-Paretas --- src/fw/kernel/fault_handling.c | 17 ++++--- src/fw/kernel/pebble_tasks.c | 2 +- src/fw/syscall/syscall_internal.c | 81 +++++++++++++++++++++++++------ src/fw/syscall/syscall_internal.h | 7 +++ 4 files changed, 86 insertions(+), 21 deletions(-) diff --git a/src/fw/kernel/fault_handling.c b/src/fw/kernel/fault_handling.c index 27e6aefc74..aec7bc9e40 100644 --- a/src/fw/kernel/fault_handling.c +++ b/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 #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; } } } diff --git a/src/fw/kernel/pebble_tasks.c b/src/fw/kernel/pebble_tasks.c index 0a5d0dc332..f6a7abe784 100644 --- a/src/fw/kernel/pebble_tasks.c +++ b/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); diff --git a/src/fw/syscall/syscall_internal.c b/src/fw/syscall/syscall_internal.c index 210bb5e60a..046b07e956 100644 --- a/src/fw/syscall/syscall_internal.c +++ b/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; } diff --git a/src/fw/syscall/syscall_internal.h b/src/fw/syscall/syscall_internal.h index 060b5280ed..8b32c226fd 100644 --- a/src/fw/syscall/syscall_internal.h +++ b/src/fw/syscall/syscall_internal.h @@ -3,8 +3,11 @@ #pragma once +#include "kernel/pebble_tasks.h" #include "pbl/util/attributes.h" +#include + #include #include #include @@ -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) From 718586d22cf77a080dc28af6c5c313ac6ed3aff3 Mon Sep 17 00:00:00 2001 From: Gerard Marull-Paretas Date: Wed, 16 Sep 2026 14:25:51 +0200 Subject: [PATCH 3/3] apps/demo: probe the syscall stack from the MPU violation test Add a "Syscall near limit" case that burns the App task's stack down to 160 B and then loads a system font the app has not touched, a syscall whose privileged chain (resource_storage, filesystem, flash) needs a few hundred bytes. With syscalls relocated to their own stack the call completes and the app reports "SURVIVED (expected)"; on a kernel that still runs syscalls on the task stack it overflows privileged and the watch reboots, which is the FIRM-4306 failure mode. Tests can now declare that surviving is the expected outcome, so the "MPU MISS" wording stays reserved for accesses that should have faulted. On qemu_flint: upstream/main "survives" only because the wiped guard lets the overflow run into the kernel heap; with the guard restored alone the emulator reboots; with the dedicated syscall stack the test passes and the guard stays untouched. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Gerard Marull-Paretas --- .../mpu_violation_test/test_mpu_violation.c | 43 +++++++++++++++++-- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/fw/apps/demo/mpu_violation_test/test_mpu_violation.c b/src/fw/apps/demo/mpu_violation_test/test_mpu_violation.c index 4894c98fc2..8d006e8e46 100644 --- a/src/fw/apps/demo/mpu_violation_test/test_mpu_violation.c +++ b/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; }