[PATCH v6 0/4] Add/enable stack protector

[PATCH v6 0/4] Add/enable stack protector

[Volodymyr Babchuk]

Both GCC and Clang support -fstack-protector feature, which add stack
canaries to functions where stack corruption is possible. This series
makes possible to use this feature in Xen. I tested this on ARM64 and
it is working as intended. Tested both with GCC and Clang. Also tested
with "-fstack-protector-all" compilation option to ensure that
initialization code works as expected.

It is hard to enable this feature on x86, as GCC stores stack canary
in %fs:40 by default, but Xen can't use %fs for various reasons. It is
possibly to change stack canary location new newer GCC versions, but
attempt to do this uncovered a whole host problems with GNU ld.
So, this series focus mostly on ARM.

Previous version of the series was acked for 4.20 release.

Changes in v6:

 - Moved stack guard initialization code to the header file
 - Expanded commit message for "[PATCH v6 3/4] xen: arm:
   enable stack protector feature"
 - Dropped couple of R-b tags
 - Added comment to "PATCH v6 4/4] CHANGELOG.md: Mention
   stack-protector feature", mentioning that it should be reworked
   if (almost certainly) it will not get into 4.20.
 - Tested with "-fstack-protector-all"


Changes in v5:

 - ARM code calls boot_stack_chk_guard_setup() from early C code
 - Bringed back stack-protector.h because C code needs to call
   boot_stack_chk_guard_setup()
 - Fixed formatting
 - Added Andrew's R-b tag

Changes in v4:

 - Added patch to CHANGELOG.md
 - Removed stack-protector.h because we dropped support for
   Xen's built-in RNG code and rely only on own implementation
 - Changes in individual patches are covered in their respect commit
 messages

Changes in v3:

 - Removed patch for riscv
 - Changes in individual patches are covered in their respect commit
 messages

Changes in v2:

 - Patch "xen: common: add ability to enable stack protector" was
   divided into two patches.
 - Rebase onto Andrew's patch that removes -fno-stack-protector-all
 - Tested on RISC-V thanks to Oleksii Kurochko
 - Changes in individual patches covered in their respect commit
 messages

Volodymyr Babchuk (4):
  common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature
  CHANGELOG.md: Mention stack-protector feature

 CHANGELOG.md                         |  1 +
 Config.mk                            |  2 +-
 stubdom/Makefile                     |  2 ++
 tools/firmware/Rules.mk              |  2 ++
 tools/tests/x86_emulator/testcase.mk |  2 +-
 xen/Makefile                         |  6 ++++
 xen/arch/arm/Kconfig                 |  1 +
 xen/arch/arm/setup.c                 |  3 ++
 xen/arch/x86/boot/Makefile           |  1 +
 xen/common/Kconfig                   | 15 ++++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 21 ++++++++++++++
 xen/include/xen/stack-protector.h    | 43 ++++++++++++++++++++++++++++
 13 files changed, 98 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.47.1

[PATCH v6 1/4] common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS

[Volodymyr Babchuk]

This patch is preparation for making stack protector
configurable. First step is to remove -fno-stack-protector flag from
EMBEDDED_EXTRA_CFLAGS so separate components (Hypervisor in this case)
can enable/disable this feature by themselves.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 Config.mk                            | 2 +-
 stubdom/Makefile                     | 2 ++
 tools/firmware/Rules.mk              | 2 ++
 tools/tests/x86_emulator/testcase.mk | 2 +-
 xen/Makefile                         | 2 ++
 xen/arch/x86/boot/Makefile           | 1 +
 6 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Config.mk b/Config.mk
index 1eb6ed04fe..4dd4b50fdf 100644
--- a/Config.mk
+++ b/Config.mk
@@ -198,7 +198,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
+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..9edcef6e99 100644
--- a/stubdom/Makefile
+++ b/stubdom/Makefile
@@ -14,6 +14,8 @@ export debug=y
 # Moved from config/StdGNU.mk
 CFLAGS += -O1 -fno-omit-frame-pointer
 
+CFLAGS += -fno-stack-protector
+
 ifeq (,$(findstring clean,$(MAKECMDGOALS)))
   ifeq ($(wildcard $(MINI_OS)/Config.mk),)
     $(error Please run 'make mini-os-dir' in top-level directory)
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index d3482c9ec4..be2692695d 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -11,6 +11,8 @@ ifneq ($(debug),y)
 CFLAGS += -DNDEBUG
 endif
 
+CFLAGS += -fno-stack-protector
+
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
 $(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
diff --git a/tools/tests/x86_emulator/testcase.mk b/tools/tests/x86_emulator/testcase.mk
index fc95e24589..7875b95d7c 100644
--- a/tools/tests/x86_emulator/testcase.mk
+++ b/tools/tests/x86_emulator/testcase.mk
@@ -4,7 +4,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
-CFLAGS += -fno-builtin -g0 $($(TESTCASE)-cflags)
+CFLAGS += -fno-builtin -fno-stack-protector -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 65b460e2b4..a0c774ab7d 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -435,6 +435,8 @@ else
 CFLAGS_UBSAN :=
 endif
 
+CFLAGS += -fno-stack-protector
+
 ifeq ($(CONFIG_LTO),y)
 CFLAGS += -flto
 LDFLAGS-$(CONFIG_CC_IS_CLANG) += -plugin LLVMgold.so
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index d457876659..ff0d61d7ac 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -17,6 +17,7 @@ obj32 := $(addprefix $(obj)/,$(obj32))
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
 CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
+CFLAGS_x86_32 += -fno-stack-protector
 CFLAGS_x86_32 += -nostdinc -include $(filter %/include/xen/config.h,$(XEN_CFLAGS))
 CFLAGS_x86_32 += $(filter -I% -O%,$(XEN_CFLAGS)) -D__XEN__
 
-- 
2.47.1

[PATCH v6 2/4] xen: common: add ability to enable stack protector

[Volodymyr Babchuk]

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 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 | 43 +++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

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..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..b758a8cb3d
--- /dev/null
+++ b/xen/include/xen/stack-protector.h
@@ -0,0 +1,43 @@
+#ifndef __XEN_STACK_PROTECTOR_H__
+#define __XEN_STACK_PROTECTOR_H__
+
+#ifdef CONFIG_STACK_PROTECTOR
+
+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 inline void 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;
+}
+
+#else
+
+static inline void boot_stack_chk_guard_setup(void) {};
+
+#endif
+
+#endif	/* __XEN_STACK_PROTECTOR_H__ */
-- 
2.47.1

[PATCH v6 4/4] CHANGELOG.md: Mention stack-protector feature

[Volodymyr Babchuk]

Stack protector is meant to be enabled on all architectures, but
currently it is tested (and enabled) only on ARM, so mention it in ARM
section.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

TODO: If this patch will not make into 4.20 - rework it by mentioning
a correct version.

Changes in v6:

 - Dropped Andrew's R-b tag because there is little chance that this
 series will be included in 4.20, so this patch should be reworked for
 4.21
---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1de1d1eca1..4cac4079f0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -23,6 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - Basic handling for SCMI requests over SMC using Shared Memory, by allowing
      forwarding the calls to EL3 FW if coming from hwdom.
    - Support for LLC (Last Level Cache) coloring.
+   - Ability to enable stack protector
  - On x86:
    - xl suspend/resume subcommands.
    - `wallclock` command line option to select time source.
-- 
2.47.1

[PATCH v6 3/4] xen: arm: enable stack protector feature

[Volodymyr Babchuk]

Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
platform. Initialize stack protector magic value very early, at the
very beginning of start_xen() function.

We want to do this early because prior to that
boot_stack_chk_guard_setup() call, default stack protector guard value
is used. While it is fine for general development and testing, it does
not provide highest security level, because potential attacker will
know the default value and can alter a payload, so correct stack
guard value will be placed in the correct position.

Apart from that argument, boot_stack_chk_guard_setup() should be
called prior to enabling secondary CPUs to avoid race with them.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Acked-by: Julien Grall <jgrall@amazon.com>

---

Changes in v6:

 - Expanded the commit message
 - Added Julien's A-b tag

Changes in v5:

 - Call boot_stack_chk_guard_setup() from start_xen()
   instead of early ASM
---
 xen/arch/arm/Kconfig | 1 +
 xen/arch/arm/setup.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index a26d3e1182..8f1a3c7d74 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -16,6 +16,7 @@ config ARM
 	select GENERIC_UART_INIT
 	select HAS_ALTERNATIVE if HAS_VMAP
 	select HAS_DEVICE_TREE
+	select HAS_STACK_PROTECTOR
 	select HAS_UBSAN
 
 config ARCH_DEFCONFIG
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c1f2d1b89d..0dca691207 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -30,6 +30,7 @@
 #include <xen/virtual_region.h>
 #include <xen/version.h>
 #include <xen/vmap.h>
+#include <xen/stack-protector.h>
 #include <xen/trace.h>
 #include <xen/libfdt/libfdt-xen.h>
 #include <xen/acpi.h>
@@ -305,6 +306,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
     struct domain *d;
     int rc, i;
 
+    boot_stack_chk_guard_setup();
+
     dcache_line_bytes = read_dcache_line_bytes();
 
     percpu_init_areas();
-- 
2.47.1

Re: [PATCH v6 2/4] xen: common: add ability to enable stack protector

[Jan Beulich]

On 17.02.2025 03:49, Volodymyr Babchuk wrote:> --- /dev/null
> +++ b/xen/include/xen/stack-protector.h
> @@ -0,0 +1,43 @@
> +#ifndef __XEN_STACK_PROTECTOR_H__
> +#define __XEN_STACK_PROTECTOR_H__
> +
> +#ifdef CONFIG_STACK_PROTECTOR
> +
> +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 inline void boot_stack_chk_guard_setup(void)

As was requested in v5 review, this needs to be always_inline. You cannot
chance the compiler deciding to make an out-of-line function.

> +{
> +    /*
> +     * 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;
> +}
> +
> +#else
> +
> +static inline void boot_stack_chk_guard_setup(void) {};
> +
> +#endif

Overall I think it would be neater (less redundancy) if the #ifdef lived inside
the body of the function. Having the decl of __stack_chk_guard always visible
isn't a major concern, imo.

Jan

Re: [PATCH v6 1/4] common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS

[Andrew Cooper]

On 17/02/2025 2:49 am, Volodymyr Babchuk wrote:
> This patch is preparation for making stack protector
> configurable. First step is to remove -fno-stack-protector flag from
> EMBEDDED_EXTRA_CFLAGS so separate components (Hypervisor in this case)
> can enable/disable this feature by themselves.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Now that 4.21 has opened, I've picked up this patch as it's ready, and
not overly intertwined with the subsequent patches.

~Andrew

Re: [PATCH v6 4/4] CHANGELOG.md: Mention stack-protector feature

[Oleksii Kurochko]

On 2/17/25 3:49 AM, Volodymyr Babchuk wrote:
> Stack protector is meant to be enabled on all architectures, but
> currently it is tested (and enabled) only on ARM, so mention it in ARM
> section.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> ---
>
> TODO: If this patch will not make into 4.20 - rework it by mentioning
> a correct version.
>
> Changes in v6:
>
>   - Dropped Andrew's R-b tag because there is little chance that this
>   series will be included in 4.20, so this patch should be reworked for
>   4.21
> ---
>   CHANGELOG.md | 1 +

Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

>   1 file changed, 1 insertion(+)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 1de1d1eca1..4cac4079f0 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -23,6 +23,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>      - Basic handling for SCMI requests over SMC using Shared Memory, by allowing
>        forwarding the calls to EL3 FW if coming from hwdom.
>      - Support for LLC (Last Level Cache) coloring.
> +   - Ability to enable stack protector
>    - On x86:
>      - xl suspend/resume subcommands.
>      - `wallclock` command line option to select time source.