Re: [PATCH] lib: sbi: Add runtime stack overrun detection

Bo Gan
2025-11-18T08:45:24Z

Hi Xiang,

On 11/17/25 20:19, Xiang W wrote:
> Implement lightweight stack overrun detection using toolchain's
> -finstrument-functions instrumentation
>
> Reviewed-by: Xiang W <wangxiang@iscas.ac.cn>
> ---
> Makefile | 4 +++
> firmware/fw_base.S | 3 +++
> firmware/payloads/test_head.S | 16 ++++++++++++
> lib/sbi/Kconfig | 4 +++
> lib/sbi/objects.mk | 2 ++
> lib/sbi/sbi_sochk.c | 47 +++++++++++++++++++++++++++++++++++
> 6 files changed, 76 insertions(+)
> create mode 100644 lib/sbi/sbi_sochk.c
>
> diff --git a/Makefile b/Makefile
> index 398eabe8..cd7d1ad5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -449,6 +449,10 @@ else
> CFLAGS += -O2
> endif
>
> +ifneq ($(CONFIG_SBI_SOCHK),)
> +CFLAGS += -finstrument-functions
> +endif
> +
> # Setup functions for compilation
> define dynamic_flags
> -I$(shell dirname $(2)) -D__OBJNAME__=$(subst -,_,$(shell basename $(1) .o))
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> index 5300ecf2..6cd5725f 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -366,6 +366,9 @@ _start_warm:
> /* Setup stack */
> add sp, tp, zero
>
> +#ifdef CONFIG_SBI_SOCHK
> + call sbi_sochk_init
> +#endif
> /* Setup trap handler */
> lla a4, _trap_handler
> csrr a5, CSR_MISA
> diff --git a/firmware/payloads/test_head.S b/firmware/payloads/test_head.S
> index 070ce8aa..de4e334f 100644
> --- a/firmware/payloads/test_head.S
> +++ b/firmware/payloads/test_head.S
> @@ -112,3 +112,19 @@ _boot_a1:
> .type __stack_chk_guard, %object
> __stack_chk_guard:
> RISCV_PTR 0x95B5FF5A
> +
> +#ifdef CONFIG_SBI_SOCHK
> + .section .text
> + .align 3
> + .weak __cyg_profile_func_enter
> + .type __cyg_profile_func_enter, %function
> +__cyg_profile_func_enter:
> + ret
> +
> +.section .text
> + .align 3
> + .weak __cyg_profile_func_exit
> + .type __cyg_profile_func_exit, %function
> +__cyg_profile_func_exit:
> + ret
> +#endif
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index c6cc04bc..77077991 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -6,6 +6,10 @@ config CONSOLE_EARLY_BUFFER_SIZE
> int "Early console buffer size (bytes)"
> default 256
>
> +config SBI_SOCHK
> + bool "Enable Stack Overflow runtime checking"
> + default n
> +
> config SBI_ECALL_TIME
> bool "Timer extension"
> default y
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index 8abe1e8e..ed6107ad 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -64,6 +64,8 @@ libsbi-objs-$(CONFIG_SBI_ECALL_SSE) += sbi_ecall_sse.o
> carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_MPXY) += ecall_mpxy
> libsbi-objs-$(CONFIG_SBI_ECALL_MPXY) += sbi_ecall_mpxy.o
>
> +libsbi-objs-$(CONFIG_SBI_SOCHK) += sbi_sochk.o
> +
> libsbi-objs-y += sbi_bitmap.o
> libsbi-objs-y += sbi_bitops.o
> libsbi-objs-y += sbi_console.o
> diff --git a/lib/sbi/sbi_sochk.c b/lib/sbi/sbi_sochk.c
> new file mode 100644
> index 00000000..950275d1
> --- /dev/null
> +++ b/lib/sbi/sbi_sochk.c
> @@ -0,0 +1,47 @@
> +
> +#include <sbi/sbi_scratch.h>
> +#include <sbi/sbi_platform.h>
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_hart.h>
> +
> +extern struct sbi_platform platform;
> +
> +static bool __scratch_init_done = false;
> +
> +__attribute__((no_instrument_function, weak))
> +void sbi_sochk_init(void)
> +{
> + __scratch_init_done = true;
> +}
> +
> +
> +__attribute__((no_instrument_function, weak))
> +void __cyg_profile_func_enter(void *this_func, void *call_site)
> +{
> + struct sbi_scratch * scratch;
> + unsigned long sp, stack_start, stack_end;
> +
> + if (!__scratch_init_done)
> + return;
> +
> + scratch = sbi_scratch_thishart_ptr();
> +
> + asm volatile("mv %0, sp" : "=r"(sp));
> + stack_start = (unsigned long)scratch + SBI_SCRATCH_SIZE - platform.hart_stack_size;
> + stack_end = (unsigned long)scratch;
> +
> + if (sp < stack_start || sp > stack_end) {
> + /* Reset SP to output error messages */
> + asm volatile("mv sp, %0"::"r"(stack_end));

I don't think it's safe to switch stack in C function. I know you want
to avoid re-entrant, but do it in C function is way too risky. Better
have an asm wapper over this function and do it before invoking the C
portion.

Bo

> + sbi_printf("Stack overflow detected in function %p (caller %p), "
> + "sp=%p (%p - %p)\n",
> + this_func, call_site, (void*)sp,
> + (void*)stack_start, (void*)stack_end);
> + sbi_hart_hang();
> + }
> +}
> +
> +__attribute__((no_instrument_function, weak))
> +void __cyg_profile_func_exit(void *this_func, void *call_site)
> +{
> +}