Skip to content

Commit ec15a61

Browse files
gmarullclaude
andcommitted
fw/syscall: run syscalls on the dedicated stack on ARMv7-M too
be4d98d 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) <noreply@anthropic.com> Signed-off-by: Gerard Marull-Paretas <gerard@teslabs.com>
1 parent 14fd25c commit ec15a61

4 files changed

Lines changed: 86 additions & 21 deletions

File tree

src/fw/kernel/fault_handling.c

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "process_state/worker_state/worker_state.h"
2020
#include "syscall/syscall.h"
2121
#include "syscall/syscall_internal.h"
22+
#include "pbl/util/size.h"
2223
#include <pbl/logging/logging.h>
2324
#include "system/reboot_reason.h"
2425
#include "syscall/syscall.h"
@@ -335,13 +336,17 @@ static void mem_manage_handler_c(unsigned int* stacked_args, unsigned int lr) {
335336
const uint8_t mmfsr = cfsr & 0xff;
336337
if (mmfsr & (1 << 7)) {
337338
uint32_t fault_addr = SCB->MMFAR;
338-
MpuRegion mpu_region = mpu_get_region(MemoryRegion_IsrStackGuard);
339-
if (memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
340-
stack_overflow = true;
341-
} else {
342-
mpu_region = mpu_get_region(MemoryRegion_TaskStackGuard);
343-
if (memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
339+
static const uint8_t s_guard_regions[] = {
340+
MemoryRegion_IsrStackGuard,
341+
MemoryRegion_TaskStackGuard,
342+
MemoryRegion_Task4, // syscall stack guard, when the task has one
343+
};
344+
for (unsigned int i = 0; i < ARRAY_LENGTH(s_guard_regions); i++) {
345+
MpuRegion mpu_region = mpu_get_region(s_guard_regions[i]);
346+
if (mpu_region.enabled &&
347+
memory_layout_is_pointer_in_region(&mpu_region, (void *)fault_addr)) {
344348
stack_overflow = true;
349+
break;
345350
}
346351
}
347352
}

src/fw/kernel/pebble_tasks.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ struct pbl_thread *pebble_task_create(PebbleTask pebble_task, struct pbl_thread_
295295
attr->regions[0] = &app_region;
296296
attr->regions[1] = &worker_region;
297297
attr->regions[2] = stack_guard_region;
298-
attr->regions[3] = NULL;
298+
attr->regions[3] = syscall_get_stack_guard_region(pebble_task);
299299

300300
struct pbl_thread *thread = &s_threads[pebble_task];
301301
PBL_ASSERT(pbl_thread_create(thread, attr) == 0, "Could not start task %s", attr->name);

src/fw/syscall/syscall_internal.c

Lines changed: 67 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "syscall_internal.h"
66

77
#include "applib/app_logging.h"
8+
#include "kernel/memory_layout.h"
89
#include "kernel/pebble_tasks.h"
910
#include "pbl/mcu/privilege.h"
1011
#include "process_management/app_manager.h"
@@ -22,13 +23,10 @@
2223
// Run App/Worker syscalls on a dedicated privileged stack instead of the
2324
// caller's small unprivileged one, so a task that exhausts its stack faults
2425
// unprivileged (only that process dies) instead of rebooting the system.
25-
// Enabled on ARMv8-M (needs PSPLIM); ARMv7-M keeps the old behaviour.
26+
// ARMv8-M bounds the syscall stack with PSPLIM; ARMv7-M plants a no-access
27+
// MPU guard below it (MemoryRegion_Task4, see pebble_tasks.c).
2628
#if !defined(SYSCALL_PRIVILEGED_STACK)
27-
# if defined(CONFIG_MPU_TYPE_ARMV8M)
28-
# define SYSCALL_PRIVILEGED_STACK 1
29-
# else
30-
# define SYSCALL_PRIVILEGED_STACK 0
31-
# endif
29+
# define SYSCALL_PRIVILEGED_STACK 1
3230
#endif
3331

3432
// Per-thread slots for the syscall return address and pre-syscall stack pointer
@@ -277,8 +275,57 @@ void syscall_assert_userspace_buffer(const void* buf, size_t num_bytes) {
277275
// in the privileged-only .kernel_bss output (RAM): unreadable by app
278276
// code, zeroed at boot. (Not section(".kernel_bss") -- that would orphan them.)
279277
#define SYSCALL_STACK_WORDS 512u // 2 KiB each; size against measured high-water.
280-
static uint32_t s_app_syscall_stack[SYSCALL_STACK_WORDS] __attribute__((aligned(8)));
281-
static uint32_t s_worker_syscall_stack[SYSCALL_STACK_WORDS] __attribute__((aligned(8)));
278+
#ifdef CONFIG_MPU_TYPE_ARMV8M
279+
#define SYSCALL_STACK_GUARD_WORDS 0u
280+
#else
281+
#define SYSCALL_STACK_GUARD_WORDS 8u // smallest ARMv7-M MPU region, naturally aligned
282+
#endif
283+
284+
typedef struct SyscallStack {
285+
#if SYSCALL_STACK_GUARD_WORDS
286+
uint32_t guard[SYSCALL_STACK_GUARD_WORDS];
287+
#endif
288+
uint32_t words[SYSCALL_STACK_WORDS];
289+
} SyscallStack;
290+
291+
static SyscallStack s_app_syscall_stack __attribute__((aligned(32)));
292+
static SyscallStack s_worker_syscall_stack __attribute__((aligned(32)));
293+
294+
#if SYSCALL_STACK_GUARD_WORDS
295+
static const MpuRegion s_app_syscall_stack_guard_region = {
296+
.region_num = MemoryRegion_Task4,
297+
.enabled = true,
298+
.base_address = (uintptr_t)s_app_syscall_stack.guard,
299+
.size = sizeof(s_app_syscall_stack.guard),
300+
.cache_policy = MpuCachePolicy_NotCacheable,
301+
.permissions = MpuPermissions_NoAccess,
302+
};
303+
304+
static const MpuRegion s_worker_syscall_stack_guard_region = {
305+
.region_num = MemoryRegion_Task4,
306+
.enabled = true,
307+
.base_address = (uintptr_t)s_worker_syscall_stack.guard,
308+
.size = sizeof(s_worker_syscall_stack.guard),
309+
.cache_policy = MpuCachePolicy_NotCacheable,
310+
.permissions = MpuPermissions_NoAccess,
311+
};
312+
#endif
313+
314+
const MpuRegion *syscall_get_stack_guard_region(PebbleTask task) {
315+
#if SYSCALL_STACK_GUARD_WORDS
316+
switch (task) {
317+
case PebbleTask_App:
318+
return &s_app_syscall_stack_guard_region;
319+
case PebbleTask_Worker:
320+
return &s_worker_syscall_stack_guard_region;
321+
default:
322+
break;
323+
}
324+
#else
325+
(void)task;
326+
#endif
327+
return NULL;
328+
}
282329

283330
// Kernel hook: top of the current task's dedicated syscall stack (base in
284331
// *base_out), or NULL to keep it on the caller's stack. App + Worker only;
@@ -295,10 +342,10 @@ uint32_t *pbl_kernel_syscall_stack(uintptr_t *base_out) {
295342
}
296343
}
297344
#endif
298-
stack = s_app_syscall_stack;
345+
stack = s_app_syscall_stack.words;
299346
break;
300347
case PebbleTask_Worker:
301-
stack = s_worker_syscall_stack;
348+
stack = s_worker_syscall_stack.words;
302349
break;
303350
default:
304351
return NULL;
@@ -315,8 +362,8 @@ static bool prv_psp_in_syscall_stack(uintptr_t psp, const uint32_t *stack) {
315362
// packed as (psplim << 32 | sp) to return in r0:r1; 0 = no switch needed.
316363
USED uint64_t syscall_stack_restore_target(void) {
317364
const uintptr_t psp = __get_PSP();
318-
if (prv_psp_in_syscall_stack(psp, s_app_syscall_stack) ||
319-
prv_psp_in_syscall_stack(psp, s_worker_syscall_stack)) {
365+
if (prv_psp_in_syscall_stack(psp, s_app_syscall_stack.words) ||
366+
prv_psp_in_syscall_stack(psp, s_worker_syscall_stack.words)) {
320367
const uint32_t sp = (uint32_t)prv_get_syscall_sp(); // slot1 = pre-syscall task SP
321368
struct pbl_thread_stack_info info;
322369
pbl_thread_stack_info(pbl_thread_current(), &info);
@@ -337,11 +384,11 @@ static uint16_t prv_syscall_stack_free_bytes(const uint32_t *stack) {
337384
}
338385

339386
uint16_t syscall_app_stack_free_bytes(void) {
340-
return prv_syscall_stack_free_bytes(s_app_syscall_stack);
387+
return prv_syscall_stack_free_bytes(s_app_syscall_stack.words);
341388
}
342389

343390
uint16_t syscall_worker_stack_free_bytes(void) {
344-
return prv_syscall_stack_free_bytes(s_worker_syscall_stack);
391+
return prv_syscall_stack_free_bytes(s_worker_syscall_stack.words);
345392
}
346393

347394
// Drop privilege and return to the task. If the syscall ran on a dedicated
@@ -360,7 +407,9 @@ EXTERNALLY_VISIBLE void NAKED_FUNC USED prv_drop_privilege(void) {
360407
" pop {r0, r1} \n" // r0,r1 = syscall return value
361408
" cbz r2, 1f \n" // skip stack switch if not relocated
362409
" msr psp, r2 \n" // back to the app stack (higher addr; safe vs low limit)
410+
#ifdef CONFIG_MPU_TYPE_ARMV8M
363411
" msr psplim, r3 \n" // restore the app stack limit
412+
#endif
364413
" isb \n"
365414
"1: \n"
366415
" mrs r2, control \n" // drop privilege: CONTROL.nPRIV = 1
@@ -371,6 +420,10 @@ EXTERNALLY_VISIBLE void NAKED_FUNC USED prv_drop_privilege(void) {
371420
);
372421
}
373422
#else
423+
const MpuRegion *syscall_get_stack_guard_region(PebbleTask task) {
424+
(void)task;
425+
return NULL;
426+
}
374427
uint16_t syscall_app_stack_free_bytes(void) { return 0xFFFF; }
375428
uint16_t syscall_worker_stack_free_bytes(void) { return 0xFFFF; }
376429

src/fw/syscall/syscall_internal.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@
33

44
#pragma once
55

6+
#include "kernel/pebble_tasks.h"
67
#include "pbl/util/attributes.h"
78

9+
#include <pbl/drivers/mpu.h>
10+
811
#include <stdbool.h>
912
#include <stdint.h>
1013
#include <stddef.h>
@@ -59,6 +62,10 @@ bool mcu_call_unprivileged_reentry_setup(uintptr_t orig_sp, uintptr_t *lr_ptr);
5962
uint16_t syscall_app_stack_free_bytes(void);
6063
uint16_t syscall_worker_stack_free_bytes(void);
6164

65+
//! No-access MPU region guarding the bottom of @p task's syscall stack, or
66+
//! NULL when the platform bounds it some other way (PSPLIM) or has none.
67+
const MpuRegion *syscall_get_stack_guard_region(PebbleTask task);
68+
6269
// Test overrides.
6370
// TODO: really implement privilege escalation in unit tests. See PBL-9688
6471
#if defined(UNITTEST)

0 commit comments

Comments
 (0)