| Message ID | 20250318023234.1210659-2-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add/enable stack protector | expand |
On 18.03.2025 03:34, Volodymyr Babchuk wrote: > Both GCC and Clang support -fstack-protector feature, which add stack > canaries to functions where stack corruption is possible. This patch > makes general preparations to enable this feature on different > supported architectures: > > - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture > can enable this feature individually > - Added user-selectable CONFIG_STACK_PROTECTOR option > - Implemented code that sets up random stack canary and a basic > handler for stack protector failures > > Stack guard value is initialized in two phases: > > 1. Pre-defined randomly-selected value. > > 2. Own implementation linear congruent random number generator. It > relies on get_cycles() being available very early. If get_cycles() > returns zero, it would leave pre-defined value from the previous > step. > > boot_stack_chk_guard_setup() is declared as inline, so it can be It's an always-inline function, and that is so important that it should be got right in the description as well. > --- /dev/null > +++ b/xen/common/stack-protector.c > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/random.h> > +#include <xen/time.h> > + > +/* > + * Initial value is chosen by a fair dice roll. > + * It will be updated during boot process. > + */ > +#if BITS_PER_LONG == 32 > +unsigned long __ro_after_init __stack_chk_guard = 0xdd2cc927UL; > +#else > +unsigned long __ro_after_init __stack_chk_guard = 0x2d853605a4d9a09cUL; > +#endif > + > +void asmlinkage __stack_chk_fail(void) The use of asmlinkage here comes close to an abuse: The Misra deviation is about C code called from assembly code only. This isn't the case here; instead it's a function that the compiler generates calls to without source code explicitly saying so. This imo wants approving from the Misra side as well, and even if approved likely requires a justifying code comment. > --- /dev/null > +++ b/xen/include/xen/stack-protector.h > @@ -0,0 +1,39 @@ > +#ifndef __XEN_STACK_PROTECTOR_H__ > +#define __XEN_STACK_PROTECTOR_H__ > + > +extern unsigned long __stack_chk_guard; > + > +/* > + * This function should be called from a C function that escapes stack > + * canary tracking (by calling reset_stack_and_jump() for example). > + */ > +static always_inline void boot_stack_chk_guard_setup(void) > +{ > +#ifdef CONFIG_STACK_PROTECTOR > + > + /* Nit: Hard tab slipped in. Jan
On 2025-03-24 13:50, Jan Beulich wrote: > On 18.03.2025 03:34, Volodymyr Babchuk wrote: >> Both GCC and Clang support -fstack-protector feature, which add stack >> canaries to functions where stack corruption is possible. This patch >> makes general preparations to enable this feature on different >> supported architectures: >> >> - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture >> can enable this feature individually >> - Added user-selectable CONFIG_STACK_PROTECTOR option >> - Implemented code that sets up random stack canary and a basic >> handler for stack protector failures >> >> Stack guard value is initialized in two phases: >> >> 1. Pre-defined randomly-selected value. >> >> 2. Own implementation linear congruent random number generator. It >> relies on get_cycles() being available very early. If get_cycles() >> returns zero, it would leave pre-defined value from the previous >> step. [...] >> +void asmlinkage __stack_chk_fail(void) > > The use of asmlinkage here comes close to an abuse: The Misra deviation > is > about C code called from assembly code only. This isn't the case here; > instead > it's a function that the compiler generates calls to without source > code > explicitly saying so. > > This imo wants approving from the Misra side as well, and even if > approved > likely requires a justifying code comment. > Here my suggestion would be an explicit deviation via a code comment, as described in [1], to describe the motivation of introducing such definition without a declaration. Moreover, asmlinkage is only relevant for the missing declaration, but is not effective for other rules. It is probably appropriate to mark the function "noreturn" as well, given its purpose. [1] https://gitlab.com/xen-project/xen/-/blob/staging/docs/misra/documenting-violations.rst >> --- /dev/null >> +++ b/xen/include/xen/stack-protector.h >> @@ -0,0 +1,39 @@ >> +#ifndef __XEN_STACK_PROTECTOR_H__ >> +#define __XEN_STACK_PROTECTOR_H__ >> + >> +extern unsigned long __stack_chk_guard; >> + >> +/* >> + * This function should be called from a C function that escapes >> stack >> + * canary tracking (by calling reset_stack_and_jump() for example). >> + */ >> +static always_inline void boot_stack_chk_guard_setup(void) >> +{ >> +#ifdef CONFIG_STACK_PROTECTOR >> + >> + /* > > Nit: Hard tab slipped in. > > Jan
diff --git a/xen/Makefile b/xen/Makefile index 58fafab33d..8fc4e042ff 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -435,7 +435,11 @@ else CFLAGS_UBSAN := endif +ifeq ($(CONFIG_STACK_PROTECTOR),y) +CFLAGS += -fstack-protector +else CFLAGS += -fno-stack-protector +endif ifeq ($(CONFIG_LTO),y) CFLAGS += -flto diff --git a/xen/common/Kconfig b/xen/common/Kconfig index a6aa2c5c14..2f6c74f11e 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -83,6 +83,9 @@ config HAS_PMAP config HAS_SCHED_GRANULARITY bool +config HAS_STACK_PROTECTOR + bool + config HAS_UBSAN bool @@ -216,6 +219,18 @@ config SPECULATIVE_HARDEN_LOCK endmenu +menu "Other hardening" + +config STACK_PROTECTOR + bool "Stack protector" + depends on HAS_STACK_PROTECTOR + help + Enable the Stack Protector compiler hardening option. This inserts a + canary value in the stack frame of functions, and performs an integrity + check on function exit. + +endmenu + config DIT_DEFAULT bool "Data Independent Timing default" depends on HAS_DIT diff --git a/xen/common/Makefile b/xen/common/Makefile index ac23120d7d..92c49127c9 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -46,6 +46,7 @@ obj-y += shutdown.o obj-y += softirq.o obj-y += smp.o obj-y += spinlock.o +obj-$(CONFIG_STACK_PROTECTOR) += stack-protector.o obj-y += stop_machine.o obj-y += symbols.o obj-y += tasklet.o diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c new file mode 100644 index 0000000000..9089294d30 --- /dev/null +++ b/xen/common/stack-protector.c @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include <xen/init.h> +#include <xen/lib.h> +#include <xen/random.h> +#include <xen/time.h> + +/* + * Initial value is chosen by a fair dice roll. + * It will be updated during boot process. + */ +#if BITS_PER_LONG == 32 +unsigned long __ro_after_init __stack_chk_guard = 0xdd2cc927UL; +#else +unsigned long __ro_after_init __stack_chk_guard = 0x2d853605a4d9a09cUL; +#endif + +void asmlinkage __stack_chk_fail(void) +{ + dump_execution_state(); + panic("Stack Protector integrity violation identified\n"); +} diff --git a/xen/include/xen/stack-protector.h b/xen/include/xen/stack-protector.h new file mode 100644 index 0000000000..c76c601399 --- /dev/null +++ b/xen/include/xen/stack-protector.h @@ -0,0 +1,39 @@ +#ifndef __XEN_STACK_PROTECTOR_H__ +#define __XEN_STACK_PROTECTOR_H__ + +extern unsigned long __stack_chk_guard; + +/* + * This function should be called from a C function that escapes stack + * canary tracking (by calling reset_stack_and_jump() for example). + */ +static always_inline void boot_stack_chk_guard_setup(void) +{ +#ifdef CONFIG_STACK_PROTECTOR + + /* + * Linear congruent generator (X_n+1 = X_n * a + c). + * + * Constant is taken from "Tables Of Linear Congruential + * Generators Of Different Sizes And Good Lattice Structure" by + * Pierre L’Ecuyer. + */ +#if BITS_PER_LONG == 32 + const unsigned long a = 2891336453UL; +#else + const unsigned long a = 2862933555777941757UL; +#endif + const unsigned long c = 1; + + unsigned long cycles = get_cycles(); + + /* Use the initial value if we can't generate random one */ + if ( !cycles ) + return; + + __stack_chk_guard = cycles * a + c; + +#endif /* CONFIG_STACK_PROTECTOR */ +} + +#endif /* __XEN_STACK_PROTECTOR_H__ */
Both GCC and Clang support -fstack-protector feature, which add stack canaries to functions where stack corruption is possible. This patch makes general preparations to enable this feature on different supported architectures: - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture can enable this feature individually - Added user-selectable CONFIG_STACK_PROTECTOR option - Implemented code that sets up random stack canary and a basic handler for stack protector failures Stack guard value is initialized in two phases: 1. Pre-defined randomly-selected value. 2. Own implementation linear congruent random number generator. It relies on get_cycles() being available very early. If get_cycles() returns zero, it would leave pre-defined value from the previous step. boot_stack_chk_guard_setup() is declared as inline, so it can be called from C code. Of course, in this case, caller should ensure that stack protection code will not be reached. It is possible to call the same function from ASM code by introducing simple trampoline in stack-protector.c, but right now there is no use case for such trampoline. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- Changes in v7: - declared boot_stack_chk_guard_setup as always_inline - moved `#ifdef CONFIG_STACK_PROTECTOR` inside the function Changes in v6: - boot_stack_chk_guard_setup() moved to stack-protector.h - Removed Andrew's r-b tag Changes in v5: - Fixed indentation - Added stack-protector.h --- xen/Makefile | 4 ++++ xen/common/Kconfig | 15 ++++++++++++ xen/common/Makefile | 1 + xen/common/stack-protector.c | 21 +++++++++++++++++ xen/include/xen/stack-protector.h | 39 +++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+) create mode 100644 xen/common/stack-protector.c create mode 100644 xen/include/xen/stack-protector.h