| Message ID | 20250213220021.2897526-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add/enable stack protector | expand |
On 13.02.2025 23:00, 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. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > > Changes in v5: > > - Fixed indentation > - Added stack-protector.h With this I question the retaining of Andrew's R-b. > --- /dev/null > +++ b/xen/common/stack-protector.c > @@ -0,0 +1,51 @@ > +/* 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 > + > +/* > + * This function should be called from early asm or from a C function > + * that escapes stack canary tracking (by calling > + * reset_stack_and_jump() for example). > + */ This comment is now stale, and ... > +void __init asmlinkage boot_stack_chk_guard_setup(void) ... I question the asmlinkage linkage here now, too. Based on how things are moving, I don't think this should be rushed into 4.20 (anymore). Jan
Hi Volodymyr, On 13/02/2025 22:00, Volodymyr Babchuk wrote: > diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c > new file mode 100644 > index 0000000000..286753a1b1 > --- /dev/null > +++ b/xen/common/stack-protector.c > @@ -0,0 +1,51 @@ > +/* 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 > + > +/* > + * This function should be called from early asm or from a C function > + * that escapes stack canary tracking (by calling > + * reset_stack_and_jump() for example). > + */ > +void __init asmlinkage boot_stack_chk_guard_setup(void) I am probably missing something. But what prevent the compiler to insert a stack guard when entering this function and checking on exit? Wouldn't this fail because __stack_chk_guard would be different? IOW, shouldn't this function be a static always inline like it used to be? Cheers,
Hi Julien, Julien Grall <julien@xen.org> writes: > Hi Volodymyr, > > On 13/02/2025 22:00, Volodymyr Babchuk wrote: >> diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c >> new file mode 100644 >> index 0000000000..286753a1b1 >> --- /dev/null >> +++ b/xen/common/stack-protector.c >> @@ -0,0 +1,51 @@ >> +/* 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 >> + >> +/* >> + * This function should be called from early asm or from a C function >> + * that escapes stack canary tracking (by calling >> + * reset_stack_and_jump() for example). >> + */ >> +void __init asmlinkage boot_stack_chk_guard_setup(void) > > I am probably missing something. But what prevent the compiler to > insert a stack guard when entering this function and checking on exit? > Wouldn't this fail because __stack_chk_guard would be different? Yes, you are right. I got carried away a bit this time. It is working right now only because GCC does not emit stack checking code in this particular function. With "-fstack-protector-all" it panics, as expected. > IOW, shouldn't this function be a static always inline like it used to be? Yes, I am going to make it inline in the next version.
diff --git a/xen/Makefile b/xen/Makefile index a0c774ab7d..48bc17c418 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 6166327f4d..bd53dae43c 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 cba3b32733..8adbf6a3b5 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..286753a1b1 --- /dev/null +++ b/xen/common/stack-protector.c @@ -0,0 +1,51 @@ +/* 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 + +/* + * This function should be called from early asm or from a C function + * that escapes stack canary tracking (by calling + * reset_stack_and_jump() for example). + */ +void __init asmlinkage boot_stack_chk_guard_setup(void) +{ + /* + * 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; +} + +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..714116498b --- /dev/null +++ b/xen/include/xen/stack-protector.h @@ -0,0 +1,14 @@ +#ifndef __XEN_STACK_PROTECTOR_H__ +#define __XEN_STACK_PROTECTOR_H__ + +#ifdef CONFIG_STACK_PROTECTOR + +void asmlinkage boot_stack_chk_guard_setup(void); + +#else + +static inline void boot_stack_chk_guard_setup(void) {}; + +#endif + +#endif /* __XEN_STACK_PROTECTOR_H__ */