| Message ID | 20241122210719.2572072-2-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add stack protector | expand |
On 22/11/2024 9:07 pm, Volodymyr Babchuk wrote: > diff --git a/Config.mk b/Config.mk > index f1eab9a20a..c9fef4659f 100644 > --- a/Config.mk > +++ b/Config.mk > @@ -190,7 +190,7 @@ endif > APPEND_LDFLAGS += $(foreach i, $(APPEND_LIB), -L$(i)) > APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i)) > > -EMBEDDED_EXTRA_CFLAGS := -fno-pie -fno-stack-protector -fno-stack-protector-all > +EMBEDDED_EXTRA_CFLAGS := -fno-pie > EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables > > XEN_EXTFILES_URL ?= https://xenbits.xen.org/xen-extfiles > diff --git a/stubdom/Makefile b/stubdom/Makefile > index 2a81af28a1..41424f6aca 100644 > --- a/stubdom/Makefile > +++ b/stubdom/Makefile > @@ -54,6 +54,8 @@ TARGET_CFLAGS += $(CFLAGS) > TARGET_CPPFLAGS += $(CPPFLAGS) > $(call cc-options-add,TARGET_CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > +$(call cc-option-add,TARGET_CFLAGS,CC,-fno-stack-protector) > + I'd suggest splitting this into two patches, so removing the flags from EMBEDDED_EXTRA_CFLAGS is separate from the new infrastructure. Also, we're losing -fno-stack-protector-all, with no discussion. AFAICT, it was introduced in c/s f8beb54e245 in 2004, alongside -fno-stack-protector. But further investigation shows that it is not, nor has ever been, a valid option to GCC or Clang. I've submitted a patch in isolation to drop this. (And Jan has reviewed it while I've been writing the rest of the email, so I'll get it committed in due course). > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 90268d9249..0ffd825510 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -86,6 +86,9 @@ config HAS_UBSAN > config HAS_VMAP > bool > > +config HAS_STACK_PROTECTOR > + bool > + > config MEM_ACCESS_ALWAYS_ON > bool > > @@ -516,4 +519,14 @@ config TRACEBUFFER > to be collected at run time for debugging or performance analysis. > Memory and execution overhead when not active is minimal. > > +config STACK_PROTECTOR > + bool "Stack protection" > + depends on HAS_STACK_PROTECTOR > + help > + Use compiler's option -fstack-protector (supported both by GCC > + and clang) to generate code that checks for corrupted stack Clang > + and halts the system in case of any problems. > + > + Please note that this option will impair performance. > + I think we probably want a top-level Hardening menu. Or maybe a Compiler Options? There are multiple levels of stack protector, not to mention other options such as trivial-auto-var-init that we want in due course. > endmenu > diff --git a/xen/common/Makefile b/xen/common/Makefile > index b279b09bfb..a9f2d05476 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -45,6 +45,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..de7c20f682 > --- /dev/null > +++ b/xen/common/stack_protector.c stack-protector.c please. > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include <xen/lib.h> > +#include <xen/random.h> > + > +#ifndef CONFIG_X86 > +/* > + * GCC uses TLS to store stack canary value on x86. > + * All other platforms use this global variable. > + */ > +unsigned long __stack_chk_guard; > +#endif Don't bother excluding x86 like this. Leave that to whomever adds x86 support. Especially as the global form is the only one we're liable to want to introduce in the first place. > + > +void __stack_chk_fail(void) > +{ > + panic("Detected stack corruption\n"); > +} Xen style, not Linux please. > diff --git a/xen/include/xen/stack_protector.h b/xen/include/xen/stack_protector.h > new file mode 100644 > index 0000000000..97f1eb5ac0 > --- /dev/null > +++ b/xen/include/xen/stack_protector.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef XEN__STACK_PROTECTOR_H > +#define XEN__STACK_PROTECTOR_H > + > +#ifdef CONFIG_STACKPROTECTOR > + > +#ifndef CONFIG_X86 > +extern unsigned long __stack_chk_guard; > +#endif > + > +/* > + * This function should be always inlined. Also it should be called > + * from a function that never returns. > + */ > +static inline void boot_stack_chk_guard_setup(void) You must use always_inline if you need it to be always inline. But, the rest of the comment isn't entirely accurate. It can also be from a function with stack-protector disabled. But the best option is to initialise __stack_chk_guard from asm before calling into C. ~Andrew
Hi, Hi Volodymyr, On 22/11/2024 21:07, Volodymyr Babchuk wrote: > diff --git a/xen/include/xen/stack_protector.h b/xen/include/xen/stack_protector.h > new file mode 100644 > index 0000000000..97f1eb5ac0 > --- /dev/null > +++ b/xen/include/xen/stack_protector.h > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef XEN__STACK_PROTECTOR_H > +#define XEN__STACK_PROTECTOR_H > + > +#ifdef CONFIG_STACKPROTECTOR > + > +#ifndef CONFIG_X86 > +extern unsigned long __stack_chk_guard; Is this variable meant to change after boot? If not, then can you tag it with __ro_after_init? > +#endif > + > +/* > + * This function should be always inlined. Also it should be called > + * from a function that never returns. > + */ > +static inline void boot_stack_chk_guard_setup(void) > +{ > + __stack_chk_guard = get_random(); > + if (BITS_PER_LONG == 64) > + __stack_chk_guard |= ((unsigned long)get_random()) << 32; > +} > + > +#else > + > +static inline void boot_stack_chk_guard_setup(void) {} > + > +#endif /* CONFIG_STACKPROTECTOR */ > + > +#endif /* XEN__STACK_PROTECTOR_H */ > + Cheers,
Hi Julien, Julien Grall <julien@xen.org> writes: > Hi, > > Hi Volodymyr, > > On 22/11/2024 21:07, Volodymyr Babchuk wrote: >> diff --git a/xen/include/xen/stack_protector.h b/xen/include/xen/stack_protector.h >> new file mode 100644 >> index 0000000000..97f1eb5ac0 >> --- /dev/null >> +++ b/xen/include/xen/stack_protector.h >> @@ -0,0 +1,30 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef XEN__STACK_PROTECTOR_H >> +#define XEN__STACK_PROTECTOR_H >> + >> +#ifdef CONFIG_STACKPROTECTOR >> + >> +#ifndef CONFIG_X86 >> +extern unsigned long __stack_chk_guard; > > Is this variable meant to change after boot? If not, then can you tag > it with __ro_after_init? > No, changing it after boot will lead to a random panic. So yes, it is good idea to make it RO.
diff --git a/Config.mk b/Config.mk index f1eab9a20a..c9fef4659f 100644 --- a/Config.mk +++ b/Config.mk @@ -190,7 +190,7 @@ endif APPEND_LDFLAGS += $(foreach i, $(APPEND_LIB), -L$(i)) APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i)) -EMBEDDED_EXTRA_CFLAGS := -fno-pie -fno-stack-protector -fno-stack-protector-all +EMBEDDED_EXTRA_CFLAGS := -fno-pie EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables XEN_EXTFILES_URL ?= https://xenbits.xen.org/xen-extfiles diff --git a/stubdom/Makefile b/stubdom/Makefile index 2a81af28a1..41424f6aca 100644 --- a/stubdom/Makefile +++ b/stubdom/Makefile @@ -54,6 +54,8 @@ TARGET_CFLAGS += $(CFLAGS) TARGET_CPPFLAGS += $(CPPFLAGS) $(call cc-options-add,TARGET_CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) +$(call cc-option-add,TARGET_CFLAGS,CC,-fno-stack-protector) + # Do not use host headers and libs GCC_INSTALL = $(shell LANG=C gcc -print-search-dirs | sed -n -e 's/install: \(.*\)/\1/p') TARGET_CPPFLAGS += -U __linux__ -U __FreeBSD__ -U __sun__ diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk index d3482c9ec4..b3f29556b7 100644 --- a/tools/firmware/Rules.mk +++ b/tools/firmware/Rules.mk @@ -15,6 +15,8 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) $(call cc-option-add,CFLAGS,CC,-fcf-protection=none) +$(call cc-option-add,CFLAGS,CC,-fno-stack-protector) + # Do not add the .note.gnu.property section to any of the firmware objects: it # breaks the rombios binary and is not useful for firmware anyway. $(call cc-option-add,CFLAGS,CC,-Wa$$(comma)-mx86-used-note=no) diff --git a/tools/tests/x86_emulator/testcase.mk b/tools/tests/x86_emulator/testcase.mk index fc95e24589..49a7a8dee9 100644 --- a/tools/tests/x86_emulator/testcase.mk +++ b/tools/tests/x86_emulator/testcase.mk @@ -4,6 +4,8 @@ include $(XEN_ROOT)/tools/Rules.mk $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS)) +$(call cc-option-add,CFLAGS,CC,-fno-stack-protector) + CFLAGS += -fno-builtin -g0 $($(TESTCASE)-cflags) LDFLAGS_DIRECT += $(shell { $(LD) -v --warn-rwx-segments; } >/dev/null 2>&1 && echo --no-warn-rwx-segments) diff --git a/xen/Makefile b/xen/Makefile index 2e1a925c84..0de0101fd0 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -432,6 +432,12 @@ else CFLAGS_UBSAN := endif +ifeq ($(CONFIG_STACK_PROTECTOR),y) +CFLAGS += -fstack-protector +else +CFLAGS += -fno-stack-protector +endif + ifeq ($(CONFIG_LTO),y) CFLAGS += -flto LDFLAGS-$(CONFIG_CC_IS_CLANG) += -plugin LLVMgold.so diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 90268d9249..0ffd825510 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -86,6 +86,9 @@ config HAS_UBSAN config HAS_VMAP bool +config HAS_STACK_PROTECTOR + bool + config MEM_ACCESS_ALWAYS_ON bool @@ -516,4 +519,14 @@ config TRACEBUFFER to be collected at run time for debugging or performance analysis. Memory and execution overhead when not active is minimal. +config STACK_PROTECTOR + bool "Stack protection" + depends on HAS_STACK_PROTECTOR + help + Use compiler's option -fstack-protector (supported both by GCC + and clang) to generate code that checks for corrupted stack + and halts the system in case of any problems. + + Please note that this option will impair performance. + endmenu diff --git a/xen/common/Makefile b/xen/common/Makefile index b279b09bfb..a9f2d05476 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -45,6 +45,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..de7c20f682 --- /dev/null +++ b/xen/common/stack_protector.c @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <xen/lib.h> +#include <xen/random.h> + +#ifndef CONFIG_X86 +/* + * GCC uses TLS to store stack canary value on x86. + * All other platforms use this global variable. + */ +unsigned long __stack_chk_guard; +#endif + +void __stack_chk_fail(void) +{ + panic("Detected stack corruption\n"); +} diff --git a/xen/include/xen/stack_protector.h b/xen/include/xen/stack_protector.h new file mode 100644 index 0000000000..97f1eb5ac0 --- /dev/null +++ b/xen/include/xen/stack_protector.h @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef XEN__STACK_PROTECTOR_H +#define XEN__STACK_PROTECTOR_H + +#ifdef CONFIG_STACKPROTECTOR + +#ifndef CONFIG_X86 +extern unsigned long __stack_chk_guard; +#endif + +/* + * This function should be always inlined. Also it should be called + * from a function that never returns. + */ +static inline void boot_stack_chk_guard_setup(void) +{ + __stack_chk_guard = get_random(); + if (BITS_PER_LONG == 64) + __stack_chk_guard |= ((unsigned long)get_random()) << 32; +} + +#else + +static inline void boot_stack_chk_guard_setup(void) {} + +#endif /* CONFIG_STACKPROTECTOR */ + +#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: - "-fno-stack-protector" is removed from global config - 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 Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- Config.mk | 2 +- stubdom/Makefile | 2 ++ tools/firmware/Rules.mk | 2 ++ tools/tests/x86_emulator/testcase.mk | 2 ++ xen/Makefile | 6 ++++++ xen/common/Kconfig | 13 ++++++++++++ xen/common/Makefile | 1 + xen/common/stack_protector.c | 16 +++++++++++++++ xen/include/xen/stack_protector.h | 30 ++++++++++++++++++++++++++++ 9 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 xen/common/stack_protector.c create mode 100644 xen/include/xen/stack_protector.h