From bb6344f015e6213c3d19615eb2ab15701f944dbc Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Thu, 29 Jan 2026 13:02:56 +0800 Subject: [PATCH] Fix concurrency bug and use uintptr_t for ABI This replaces global in_syscall_context flag with per-task tracking in TCB. Global flag was corrupted when Task A yielded during syscall and Task B ran another syscall, clearing the flag before Task A resumed. - Add volatile bool in_syscall to tcb_t (survives preemption) - Update do_syscall to track per-task context - Update mo_task_spawn_kernel to check per-task flag - Use uintptr_t for syscall arguments (matches RISC-V ABI semantics) --- arch/riscv/entry.c | 5 ++++- arch/riscv/hal.c | 10 +++++---- include/sys/syscall.h | 18 ++++++++++------ include/sys/task.h | 6 ++++++ kernel/syscall.c | 49 +++++++++++++++++++++++-------------------- kernel/task.c | 20 ++++++++++-------- 6 files changed, 65 insertions(+), 43 deletions(-) diff --git a/arch/riscv/entry.c b/arch/riscv/entry.c index 9956558e..f3babbc6 100644 --- a/arch/riscv/entry.c +++ b/arch/riscv/entry.c @@ -18,8 +18,11 @@ /* Architecture-specific syscall implementation using ecall trap. * This overrides the weak symbol defined in kernel/syscall.c. + * + * Arguments are uintptr_t (register-sized) to match the syscall ABI. + * On RV32, uintptr_t == uint32_t, so casting to int for registers is safe. */ -int syscall(int num, void *arg1, void *arg2, void *arg3) +int syscall(int num, uintptr_t arg1, uintptr_t arg2, uintptr_t arg3) { register int a0 asm("a0") = (int) arg1; register int a1 asm("a1") = (int) arg2; diff --git a/arch/riscv/hal.c b/arch/riscv/hal.c index 7ad5806f..310f46bd 100644 --- a/arch/riscv/hal.c +++ b/arch/riscv/hal.c @@ -387,15 +387,17 @@ uint32_t do_trap(uint32_t cause, uint32_t epc, uint32_t isr_sp) uint32_t *f = (uint32_t *) isr_sp; int syscall_num = f[FRAME_A7]; - void *arg1 = (void *) f[FRAME_A0]; - void *arg2 = (void *) f[FRAME_A1]; - void *arg3 = (void *) f[FRAME_A2]; + uintptr_t arg1 = (uintptr_t) f[FRAME_A0]; + uintptr_t arg2 = (uintptr_t) f[FRAME_A1]; + uintptr_t arg3 = (uintptr_t) f[FRAME_A2]; /* Dispatch to syscall implementation via direct table lookup. * Must use do_syscall here instead of syscall() to avoid recursive * traps, as the user-space syscall() may be overridden with ecall. + * Arguments are uintptr_t (register-sized) matching RISC-V ABI. */ - extern int do_syscall(int num, void *arg1, void *arg2, void *arg3); + extern int do_syscall(int num, uintptr_t a1, uintptr_t a2, + uintptr_t a3); int retval = do_syscall(syscall_num, arg1, arg2, arg3); /* Store return value and updated PC */ diff --git a/include/sys/syscall.h b/include/sys/syscall.h index 0da62598..1f60bbe4 100644 --- a/include/sys/syscall.h +++ b/include/sys/syscall.h @@ -3,14 +3,17 @@ #pragma once #include +#include /* System Call Table * * This macro defines all system calls in a table format: * _(name, number, return_type, argument_list) * - * The table is used to generate both the enumeration of syscall numbers - * and the function prototypes for user-space wrappers. + * The table generates: + * - SYS_name enumeration values + * - sys_name() user-space wrapper prototypes + * - _name() kernel handler declarations */ #define SYSCALL_TABLE \ /* POSIX-style system calls (1-31) */ \ @@ -63,14 +66,17 @@ typedef enum { * This is the low-level interface for making system calls. Architecture- * specific implementations may override this weak symbol. * + * Arguments are passed as uintptr_t (register-sized integers) to match + * the RISC-V calling convention where syscall args come from a0-a2. + * * @num : System call number (from mo_syscall_t enum) - * @arg1 : First argument (type varies by syscall) - * @arg2 : Second argument (type varies by syscall) - * @arg3 : Third argument (type varies by syscall) + * @arg1 : First argument (register-sized, cast by handler) + * @arg2 : Second argument (register-sized, cast by handler) + * @arg3 : Third argument (register-sized, cast by handler) * * Returns syscall-specific return value */ -int syscall(int num, void *arg1, void *arg2, void *arg3); +int syscall(int num, uintptr_t arg1, uintptr_t arg2, uintptr_t arg3); /* Generate function prototypes for user-space wrappers */ #define _(name, num, rettype, arglist) rettype sys_##name arglist; diff --git a/include/sys/task.h b/include/sys/task.h index ac8db01c..f6b78e50 100644 --- a/include/sys/task.h +++ b/include/sys/task.h @@ -87,6 +87,12 @@ typedef struct tcb { uint8_t state; /* Current lifecycle state (e.g., TASK_READY) */ task_mode_t mode; /* Privilege mode: TASK_MODE_M or TASK_MODE_U */ + /* Syscall Context Tracking (per-task, survives preemption). + * Volatile because this flag is set/cleared around code that may be + * preempted by timer interrupt, and checked from different code paths. + */ + volatile bool in_syscall; /* True while executing within do_syscall */ + /* Real-time Scheduling Support */ void *rt_prio; /* Opaque pointer for custom real-time scheduler hook */ diff --git a/kernel/syscall.c b/kernel/syscall.c index fe6e0433..18175816 100644 --- a/kernel/syscall.c +++ b/kernel/syscall.c @@ -7,14 +7,7 @@ #include "private/task.h" #include "private/utils.h" -/* Syscall context flag for privilege enforcement. - * When set, kernel APIs that check privilege will reject M-mode requests. - * This prevents privilege escalation if U-mode code somehow calls kernel - * functions directly (defense-in-depth alongside hardware protection). - */ -volatile bool in_syscall_context = false; - -/* syscall wrappers */ +/* syscall wrappers - forward declarations */ #define _(name, num, rettype, arglist) static rettype _##name arglist; SYSCALL_TABLE #undef _ @@ -28,10 +21,11 @@ static const void *syscall_table[SYS_COUNT] = {SYSCALL_TABLE}; * Called by trap handlers to invoke syscall implementations without * triggering privilege transitions. User space must not call this directly. * - * Sets in_syscall_context flag during execution to enable runtime privilege - * checks in kernel APIs (defense-in-depth for mo_task_spawn_kernel). + * Per-task syscall context tracking: + * - Sets tcb_t.in_syscall flag for current task (survives preemption) + * - Enables runtime privilege checks in kernel APIs (defense-in-depth) */ -int do_syscall(int num, void *a1, void *a2, void *a3) +int do_syscall(int num, uintptr_t a1, uintptr_t a2, uintptr_t a3) { if (unlikely(num <= 0 || num >= SYS_COUNT)) return -ENOSYS; @@ -39,12 +33,18 @@ int do_syscall(int num, void *a1, void *a2, void *a3) if (unlikely(!syscall_table[num])) return -ENOSYS; - /* Mark syscall context for privilege enforcement */ - in_syscall_context = true; + /* Per-task syscall context tracking (survives preemption) */ + tcb_t *self = (kcb && kcb->task_current) ? kcb->task_current->data : NULL; + + if (self) + self->in_syscall = true; + int result = - ((int (*)(void *, void *, void *)) syscall_table[num])(a1, a2, a3); - in_syscall_context = false; + ((int (*)(uintptr_t, uintptr_t, uintptr_t)) syscall_table[num])(a1, a2, + a3); + if (self) + self->in_syscall = false; return result; } @@ -52,7 +52,10 @@ int do_syscall(int num, void *a1, void *a2, void *a3) * This weak symbol allows architecture-specific implementations to override * with trap-based entry mechanisms. */ -__attribute__((weak)) int syscall(int num, void *a1, void *a2, void *a3) +__attribute__((weak)) int syscall(int num, + uintptr_t a1, + uintptr_t a2, + uintptr_t a3) { return do_syscall(num, a1, a2, a3); } @@ -270,7 +273,7 @@ static int _task_spawn(void *task, int stack_size) int sys_task_spawn(void *task, int stack_size) { - return syscall(SYS_task_spawn, task, (void *) stack_size, 0); + return syscall(SYS_task_spawn, (uintptr_t) task, (uintptr_t) stack_size, 0); } static int _tcancel(int id) @@ -283,7 +286,7 @@ static int _tcancel(int id) int sys_tcancel(int id) { - return syscall(SYS_tcancel, (void *) id, 0, 0); + return syscall(SYS_tcancel, (uintptr_t) id, 0, 0); } static int _tyield(void) @@ -308,7 +311,7 @@ static int _tdelay(int ticks) int sys_tdelay(int ticks) { - return syscall(SYS_tdelay, (void *) ticks, 0, 0); + return syscall(SYS_tdelay, (uintptr_t) ticks, 0, 0); } static int _tsuspend(int id) @@ -321,7 +324,7 @@ static int _tsuspend(int id) int sys_tsuspend(int id) { - return syscall(SYS_tsuspend, (void *) id, 0, 0); + return syscall(SYS_tsuspend, (uintptr_t) id, 0, 0); } static int _tresume(int id) @@ -334,7 +337,7 @@ static int _tresume(int id) int sys_tresume(int id) { - return syscall(SYS_tresume, (void *) id, 0, 0); + return syscall(SYS_tresume, (uintptr_t) id, 0, 0); } static int _tpriority(int id, int priority) @@ -347,7 +350,7 @@ static int _tpriority(int id, int priority) int sys_tpriority(int id, int priority) { - return syscall(SYS_tpriority, (void *) id, (void *) priority, 0); + return syscall(SYS_tpriority, (uintptr_t) id, (uintptr_t) priority, 0); } static int _tid(void) @@ -416,5 +419,5 @@ static int _tputs(const char *str) int sys_tputs(const char *str) { - return syscall(SYS_tputs, (void *) str, 0, 0); + return syscall(SYS_tputs, (uintptr_t) str, 0, 0); } diff --git a/kernel/task.c b/kernel/task.c index 746d139e..0bb5fb66 100644 --- a/kernel/task.c +++ b/kernel/task.c @@ -13,11 +13,6 @@ #include "private/error.h" #include "private/utils.h" -/* Syscall context flag from syscall.c for runtime privilege enforcement. - * When true, M-mode task creation requests are rejected (defense-in-depth). - */ -extern volatile bool in_syscall_context; - static int32_t noop_rtsched(void); void _timer_tick_handler(void); @@ -749,6 +744,7 @@ static int32_t task_spawn_internal(void *task_entry, tcb->rt_prio = NULL; tcb->state = TASK_STOPPED; tcb->mode = user_mode ? TASK_MODE_U : TASK_MODE_M; + tcb->in_syscall = false; /* Not in syscall context at creation */ /* Set default priority with proper scheduler fields */ tcb->prio = TASK_PRIO_NORMAL; @@ -817,7 +813,7 @@ static int32_t task_spawn_internal(void *task_entry, * * Security (defense-in-depth with multiple layers): * 1. Compile-time: private/task.h guard rejects non-kernel builds - * 2. Runtime: in_syscall_context check rejects calls from syscall handlers + * 2. Runtime: per-task in_syscall check rejects calls from syscall handlers * 3. Hardware: read_csr(mstatus) traps immediately if called from U-mode */ int32_t mo_task_spawn_kernel(void *task_entry, uint16_t stack_size) @@ -828,9 +824,15 @@ int32_t mo_task_spawn_kernel(void *task_entry, uint16_t stack_size) */ volatile uint32_t mstatus_check __attribute__((unused)) = read_csr(mstatus); - /* Defense-in-depth: reject M-mode task creation from syscall context */ - if (in_syscall_context) - return -1; + /* Defense-in-depth: reject M-mode task creation from syscall context. + * Uses per-task flag (tcb_t.in_syscall) which survives preemption, + * fixing the concurrency bug where global flag was corrupted. + */ + if (kcb && kcb->task_current && kcb->task_current->data) { + tcb_t *self = kcb->task_current->data; + if (self->in_syscall) + return -1; + } return task_spawn_internal(task_entry, stack_size, false); }