[PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

[PATCH v5 for-4.20(?) 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.

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 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         | 51 ++++++++++++++++++++++++++++
 xen/include/xen/stack-protector.h    | 14 ++++++++
 13 files changed, 99 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.47.1

[PATCH v5 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 v5 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.

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
---
 xen/Makefile                      |  4 +++
 xen/common/Kconfig                | 15 +++++++++
 xen/common/Makefile               |  1 +
 xen/common/stack-protector.c      | 51 +++++++++++++++++++++++++++++++
 xen/include/xen/stack-protector.h | 14 +++++++++
 5 files changed, 85 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..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__ */
-- 
2.47.1

[PATCH v5 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 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 v5 3/4] xen: arm: enable stack protector feature

[Volodymyr Babchuk]

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

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

---

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 v5 for-4.20(?) 0/4] Add/enable stack protector

[Andrew Cooper]

On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
> 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

Given the last minute observation on v4, I ran this through GitlabCI
with STACK_PROTECTOR forced on.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
is the full run.

https://gitlab.com/xen-project/people/andyhhp/xen/-/commit/b07b024f51907bc0f93d581287d36cf5bbfa98e2
is the patch to force things on, including some extra UBSAN because that
got missed.

This is relevant because the only 3 failures present are ARM32 UBSAN
failures in credit2.

Second, in all 3 failures, we've got an `ERROR-EOF!` interrupting the
backtrace, which I presume is something coming out of Expect.  Stefano,
any ideas?

My main observation is that there's no exterior way of telling whether
stack protector is actually active or not.  i.e. nothing printed during
setup.  However, all 4 builds did build common/stack-protector.o so I'm
assuming it was properly active.

If it was going to explode, it would have done before the UBSAN
failures, which are reasonably late on boot.

~Andrew

Re: [PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

[Volodymyr Babchuk]

Hi Andrew,

Andrew Cooper <andrew.cooper3@citrix.com> writes:

> On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
>> 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
>
> Given the last minute observation on v4, I ran this through GitlabCI
> with STACK_PROTECTOR forced on.
>
> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
> is the full run.

I noticed that you enabled both UBSAN and STACK_PROTECTOR for
arm32. Have you tried to run arm32 test with UBSAN only?

[...]

--
WBR, Volodymyr

Re: [PATCH v5 for-4.20(?) 0/4] Add/enable stack protector

[Andrew Cooper]

On 14/02/2025 12:34 am, Volodymyr Babchuk wrote:
> Hi Andrew,
>
> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> On 13/02/2025 10:00 pm, Volodymyr Babchuk wrote:
>>> 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
>> Given the last minute observation on v4, I ran this through GitlabCI
>> with STACK_PROTECTOR forced on.
>>
>> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1670468808
>> is the full run.
> I noticed that you enabled both UBSAN and STACK_PROTECTOR for
> arm32. Have you tried to run arm32 test with UBSAN only?

No, but I'm also confident that the UBSAN failure is unrelated to
STACK_PROTECTOR.

It turns out it's very gnarly to fix.

~Andrew

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

[Jan Beulich]

On 13.02.2025 23:00, 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>
> ---
>  Config.mk                            | 2 +-
>  stubdom/Makefile                     | 2 ++

Bzw, since on v4 the question was whether this series is ready to go in:
Strictly speaking you'd need a stubdom ack here as well.

Jan

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Julien Grall]

Hi Volodymyr,

On 13/02/2025 22:00, Volodymyr Babchuk wrote:
> Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
> platform. Initialize stack protector very early, at the very beginning
> of start_xen() function.

It would be worth explaining why this needs to be called very early 
given we have a default stack guard value. AFAIK, the only requirement 
is to have this enabled before we bring up any secondary CPUs.

This would be useful information if we decide to re-order the init code.

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

With the remark above:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Volodymyr Babchuk]

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.


-- 
WBR, Volodymyr