[PATCH v7 0/3] Add/enable stack protector

[PATCH v7 0/3] 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.

Changes in v7:
 - Patch "common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS"
   is taken into mainline
 - Updated CHANGELOG for v4.21
 - Updated stack-protector.h as per Jan's comments

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 (3):
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature
  CHANGELOG.md: Mention stack-protector feature

 CHANGELOG.md                      |  2 ++
 xen/Makefile                      |  4 ++++
 xen/arch/arm/Kconfig              |  1 +
 xen/arch/arm/setup.c              |  3 +++
 xen/common/Kconfig                | 15 ++++++++++++
 xen/common/Makefile               |  1 +
 xen/common/stack-protector.c      | 21 +++++++++++++++++
 xen/include/xen/stack-protector.h | 39 +++++++++++++++++++++++++++++++
 8 files changed, 86 insertions(+)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.48.1

[PATCH v7 2/3] 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 ffdff1f0a3..5d6870c817 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -15,6 +15,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 ffcae900d7..fa11e6be9f 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>
@@ -306,6 +307,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.48.1

[PATCH v7 1/3] 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 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

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__ */
-- 
2.48.1

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

[Jan Beulich]

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

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

[Nicola Vetrini]

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

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

[PATCH v7 3/3] 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>
Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

---

Changes in v7:
 - Moved the change to v4.21
 - Added Oleksii's acked-by tag

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7201c484f8..9605f670f6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - On x86:
    - Option to attempt to fixup p2m page-faults on PVH dom0.
    - Resizable BARs is supported for PVH dom0.
+ - On Arm:
+    - Ability to enable stack protector
 
 ### Removed
 
-- 
2.48.1