[PATCH] lib: utils: Initial compiler built-in stack protector support
Alvin Chang
alvinga at andestech.com
Sun May 18 18:48:56 PDT 2025
Hi Yu-Chien,
> -----Original Message-----
> From: Yu-Chien Lin <mail at yclin.org>
> Sent: Wednesday, May 7, 2025 10:35 PM
> To: opensbi at lists.infradead.org; Alvin Che-Chia Chang(張哲嘉)
> <alvinga at andestech.com>
> Subject: Re: [PATCH] lib: utils: Initial compiler built-in stack
> protector support
>
> [EXTERNAL MAIL]
>
> Hi Alvin,
>
> I encountered an issue when building with DEBUG=1 and
> CONFIG_STACK_PROTECTOR_ALL=y. An exception occurs at the
> __check_csr_64 macro which calls sbi_hart_expected_trap_addr().
> Not sure the root cause but here's a workaround that disables stack
> protection specifically for this function:
>
> + #pragma GCC push_options
> + #pragma GCC optimize ("no-stack-protector")
> static inline ulong sbi_hart_expected_trap_addr(void) {
> return (ulong)sbi_hart_expected_trap; }
> + #pragma GCC pop_options
>
> Regards,
> Peter Lin
Thanks for reporting this issue.
The root cause is that when OpenSBI is compiled with -O0 optimization level,
the sbi_hart_expected_trap_addr() in the following code block becomes a "regular function call" rather than "inline function":
#define csr_read_allowed(csr_num, trap) \
({ \
register ulong tinfo asm("a3") = (ulong)trap; \
register ulong ttmp asm("a4"); \
register ulong mtvec = sbi_hart_expected_trap_addr(); \
register ulong ret = 0; \
((struct sbi_trap_info *)(trap))->cause = 0; \
asm volatile( \
"add %[ttmp], %[tinfo], zero\n" \
"csrrw %[mtvec], " STR(CSR_MTVEC) ", %[mtvec]\n" \
"csrr %[ret], %[csr]\n" \
"csrw " STR(CSR_MTVEC) ", %[mtvec]" \
: [mtvec] "+&r"(mtvec), [tinfo] "+&r"(tinfo), \
[ttmp] "+&r"(ttmp), [ret] "=&r" (ret) \
: [csr] "i" (csr_num) \
: "memory"); \
ret; \
}) \
When compiling OpenSBI with -O0 and -fstack-protector-all:
Compiler uses register a3 in sbi_hart_expected_trap_addr() to check the stack guard variable.
However, register a3 is not preserved when calling sbi_hart_expected_trap_addr(), so a3 is clobbered by sbi_hart_expected_trap_addr().
val = csr_read_allowed(CSR_TSELECT, &trap);
6dae: fb040793 addi a5,s0,-80
6db2: 86be mv a3,a5
6db4: af5ff0ef jal 68a8 <sbi_hart_expected_trap_addr>
6db8: 84aa mv s1,a0
6dba: fa043823 sd zero,-80(s0)
6dbe: 87a6 mv a5,s1
6dc0: 00068733 add a4,a3,zero
6dc4: 305797f3 csrrw a5,mtvec,a5
6dc8: 7a002673 csrr a2,tselect
6dcc: 30579073 csrw mtvec,a5
6dd0: 8932 mv s2,a2
6dd2: 87ca mv a5,s2
6dd4: f8f43023 sd a5,-128(s0)
In addition to the workaround you provided, there are two workarounds I found:
1. Instead of calling sbi_hart_expected_trap_addr(), we can directly cast sbi_hart_expected_trap since it is extern variable.
- register ulong mtvec = sbi_hart_expected_trap_addr();
+ register ulong mtvec = (ulong)sbi_hart_expected_trap;
2. Reorder sbi_hart_expected_trap_addr() before using a3:
register ulong mtvec = sbi_hart_expected_trap_addr(); \
register ulong tinfo asm("a3") = (ulong)trap; \
register ulong ttmp asm("a4");
I am not sure which workaround is the best one.
Maybe there are better solutions which I am not aware of.
Need other reviewer's comments.
Regards,
Alvin Chang
>
> On 5/4/25 12:19 AM, Alvin Chang wrote:
> > Add stack_protector.c which defines the stack guard variable and the
> > function which will be referenced by compiler built-in stack protector.
> >
> > This patch just try to support stack-protector so the value of the
> > stack guard variable is simply fixed for now. It could be improved
> > by deriving from a random number generator, such as Zkr extension or
> > any platform-specific random number sources.
> >
> > Introduce three configurations for the stack protector:
> > 1. CONFIG_STACK_PROTECTOR to enable the stack protector feature by
> > providing "-fstack-protector" compiler flag 2.
> > CONFIG_STACK_PROTECTOR_STRONG to provide "-fstack-protector-strong"
> > 3. CONFIG_STACK_PROTECTOR_ALL to provide "-fstack-protector-all"
> >
> > Instead of fixing the compiler flag of stack-protector feature as
> > "-fstack-protector", we derive it from the introduced Kconfig
> > configurations. The compiler flag "stack-protector-cflags-y" is
> > defined as Makefile "immediately expanded variables" with ":=".
> > Thus, the stronger configuration of the stack protector can
> > overwrite the preceding one.
> >
> > Signed-off-by: Alvin Chang <alvinga at andestech.com>
> > ---
> > Makefile | 3 ++-
> > lib/utils/Kconfig | 2 ++
> > lib/utils/stack_protector/Kconfig | 28
> +++++++++++++++++++++
> > lib/utils/stack_protector/objects.mk | 18 +++++++++++++
> > lib/utils/stack_protector/stack_protector.c | 18 +++++++++++++
> > 5 files changed, 68 insertions(+), 1 deletion(-)
> > create mode 100644 lib/utils/stack_protector/Kconfig
> > create mode 100644 lib/utils/stack_protector/objects.mk
> > create mode 100644 lib/utils/stack_protector/stack_protector.c
> >
> > diff --git a/Makefile b/Makefile
> > index e90836c..eae6d16 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -360,7 +360,7 @@ GENFLAGS += $(libsbiutils-genflags-y)
> > GENFLAGS += $(platform-genflags-y)
> > GENFLAGS += $(firmware-genflags-y)
> >
> > -CFLAGS = -g -Wall -Werror -ffreestanding -nostdlib
> -fno-stack-protector -fno-strict-aliasing -ffunction-sections
> -fdata-sections
> > +CFLAGS = -g -Wall -Werror -ffreestanding
> -nostdlib -fno-strict-aliasing -ffunction-sections -fdata-sections
> > CFLAGS += -fno-omit-frame-pointer
> -fno-optimize-sibling-calls
> > # Optionally supported flags
> > ifeq ($(CC_SUPPORT_VECTOR),y)
> > @@ -379,6 +379,7 @@ CFLAGS += $(GENFLAGS)
> > CFLAGS += $(platform-cflags-y)
> > CFLAGS += -fPIE -pie
> > CFLAGS += $(firmware-cflags-y)
> > +CFLAGS += $(stack-protector-cflags-y)
> >
> > CPPFLAGS += $(GENFLAGS)
> > CPPFLAGS += $(platform-cppflags-y)
> > diff --git a/lib/utils/Kconfig b/lib/utils/Kconfig index
> > 901ba56..40e5633 100644
> > --- a/lib/utils/Kconfig
> > +++ b/lib/utils/Kconfig
> > @@ -26,6 +26,8 @@ source "$(OPENSBI_SRC_DIR)/lib/utils/reset/Kconfig"
> >
> > source "$(OPENSBI_SRC_DIR)/lib/utils/serial/Kconfig"
> >
> > +source "$(OPENSBI_SRC_DIR)/lib/utils/stack_protector/Kconfig"
> > +
> > source "$(OPENSBI_SRC_DIR)/lib/utils/suspend/Kconfig"
> >
> > source "$(OPENSBI_SRC_DIR)/lib/utils/sys/Kconfig"
> > diff --git a/lib/utils/stack_protector/Kconfig
> > b/lib/utils/stack_protector/Kconfig
> > new file mode 100644
> > index 0000000..c1fee19
> > --- /dev/null
> > +++ b/lib/utils/stack_protector/Kconfig
> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +
> > +menu "Stack Protector Support"
> > +
> > +config STACK_PROTECTOR
> > + bool "Stack Protector buffer overflow detection"
> > + default n
> > + help
> > + This option turns on the "stack-protector" compiler feature.
> > +
> > +config STACK_PROTECTOR_STRONG
> > + bool "Strong Stack Protector"
> > + depends on STACK_PROTECTOR
> > + default n
> > + help
> > + Turn on the "stack-protector" with "-fstack-protector-strong"
> option.
> > + Like -fstack-protector but includes additional functions to be
> > + protected.
> > +
> > +config STACK_PROTECTOR_ALL
> > + bool "Almighty Stack Protector"
> > + depends on STACK_PROTECTOR
> > + default n
> > + help
> > + Turn on the "stack-protector" with "-fstack-protector-all" option.
> > + Like -fstack-protector except that all functions are protected.
> > +
> > +endmenu
> > diff --git a/lib/utils/stack_protector/objects.mk
> > b/lib/utils/stack_protector/objects.mk
> > new file mode 100644
> > index 0000000..d7563e3
> > --- /dev/null
> > +++ b/lib/utils/stack_protector/objects.mk
> > @@ -0,0 +1,18 @@
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause # # Copyright (c) 2025
> > +Andes Technology Corporation # # Authors:
> > +# Alvin Chang <alvinga at andestech.com>
> > +#
> > +
> > +libsbiutils-objs-$(CONFIG_STACK_PROTECTOR) +=
> > +stack_protector/stack_protector.o
> > +
> > +ifeq ($(CONFIG_STACK_PROTECTOR),y)
> > +stack-protector-cflags-$(CONFIG_STACK_PROTECTOR) :=
> > +-fstack-protector
> > +stack-protector-cflags-$(CONFIG_STACK_PROTECTOR_STRONG) :=
> > +-fstack-protector-strong
> > +stack-protector-cflags-$(CONFIG_STACK_PROTECTOR_ALL) :=
> > +-fstack-protector-all else stack-protector-cflags-y :=
> > +-fno-stack-protector endif
> > diff --git a/lib/utils/stack_protector/stack_protector.c
> > b/lib/utils/stack_protector/stack_protector.c
> > new file mode 100644
> > index 0000000..3088421
> > --- /dev/null
> > +++ b/lib/utils/stack_protector/stack_protector.c
> > @@ -0,0 +1,18 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (c) 2025 Andes Technology Corporation
> > + *
> > + * Authors:
> > + * Alvin Chang <alvinga at andestech.com>
> > + */
> > +
> > +#include <sbi/sbi_console.h>
> > +#include <sbi/sbi_types.h>
> > +
> > +void *__stack_chk_guard = (void *)0x95B5FF5AUL;
> > +
> > +void __noreturn __stack_chk_fail(void) {
> > + sbi_panic("Stack corruption detected\n"); }
More information about the opensbi
mailing list