1. Patchew
  2. Xen
  3. [PATCH 0/3] Add stack protector
  4. View patch

[PATCH 1/3] xen: common: add ability to enable stack protector

Volodymyr Babchuk posted 3 patches 1 month ago
There is a newer version of this series
[PATCH 1/3] xen: common: add ability to enable stack protector
Posted by Volodymyr Babchuk 1 month ago 
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

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 */
+
-- 
2.47.0
Re: [PATCH 1/3] xen: common: add ability to enable stack protector
Posted by Julien Grall 1 month ago 
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,

-- 
Julien Grall
Re: [PATCH 1/3] xen: common: add ability to enable stack protector
Posted by Volodymyr Babchuk 1 month ago 
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.

-- 
WBR, Volodymyr
Re: [PATCH 1/3] xen: common: add ability to enable stack protector
Posted by Andrew Cooper 1 month ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.