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

[PATCH 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:

 - "-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

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

My aim was to enable it on x86 also, but it appears that on x86 GCC
stores canary value in TLS, exactly at fs:40, which is hardcoded. As
Xen does not setup fs register for itself, any attempt to enable stack
protector leads to paging abort.

I also tested build-ability for RISCV platform, but didn't tested that
it does not break anything, so we will need RISCV maintainer's
approval.

Volodymyr Babchuk (3):
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature
  xen: riscv: enable stack protector feature

 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/riscv/Kconfig               |  1 +
 xen/arch/riscv/setup.c               |  3 +++
 xen/common/Kconfig                   | 13 ++++++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack_protector.c         | 16 +++++++++++++++
 xen/include/xen/stack_protector.h    | 30 ++++++++++++++++++++++++++++
 13 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/stack_protector.c
 create mode 100644 xen/include/xen/stack_protector.h

-- 
2.47.0

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

[Volodymyr Babchuk]

Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
platform. Here we can call boot_stack_chk_guard_setup() in start_xen()
function, because it never returns, so stack protector code will not
be triggered because of changed canary.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 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 15b2e4a227..8fbb31bc07 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -17,6 +17,7 @@ config ARM
 	select HAS_PASSTHROUGH
 	select HAS_UBSAN
 	select IOMMU_FORCE_PT_SHARE
+	select HAS_STACK_PROTECTOR
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca..2bd3caf90b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -13,6 +13,7 @@
 #include <xen/domain_page.h>
 #include <xen/grant_table.h>
 #include <xen/types.h>
+#include <xen/stack_protector.h>
 #include <xen/string.h>
 #include <xen/serial.h>
 #include <xen/sched.h>
@@ -341,6 +342,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
      */
     system_state = SYS_STATE_boot;
 
+    boot_stack_chk_guard_setup();
+
     if ( acpi_disabled )
     {
         printk("Booting using Device Tree\n");
-- 
2.47.0

[PATCH 3/3] xen: riscv: enable stack protector feature

[Volodymyr Babchuk]

Enable previously added CONFIG_STACK_PROTECTOR feature for RISC-V
platform. Here we can call boot_stack_chk_guard_setup() in start_xen()
function, because it never returns, so stack protector code will not
be triggered because of changed canary.

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

---

I tested this patch for buildability, but didn't verified that Xen
really boots on RISC-V when this feature is enabled. So I am asking
RISC-V maintainers to provide feedback on it.
---
 xen/arch/riscv/Kconfig | 1 +
 xen/arch/riscv/setup.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 1858004676..1481f23b66 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -5,6 +5,7 @@ config RISCV
 	select HAS_DEVICE_TREE
 	select HAS_PMAP
 	select HAS_VMAP
+	select HAS_STACK_PROTECTOR
 
 config RISCV_64
 	def_bool y
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index e29bd75d7c..cd71748d2c 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -7,6 +7,7 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/shutdown.h>
+#include <xen/stack_protector.h>
 
 #include <public/version.h>
 
@@ -55,6 +56,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
     if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
         BUG();
 
+    boot_stack_chk_guard_setup();
+
     cmdline = boot_fdt_cmdline(device_tree_flattened);
     printk("Command line: %s\n", cmdline);
     cmdline_parse(cmdline);
-- 
2.47.0

Re: [PATCH 0/3] Add stack protector

[Andrew Cooper]

On 22/11/2024 9:07 pm, Volodymyr Babchuk wrote:
> 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.
>
> My aim was to enable it on x86 also, but it appears that on x86 GCC
> stores canary value in TLS, exactly at fs:40, which is hardcoded. As
> Xen does not setup fs register for itself, any attempt to enable stack
> protector leads to paging abort.

It's rather worse than a #PF, if a devious PV guest decides to map
memory at 0.

There's a long and complicated history with stack-protector on x86.

It is %fs:40 by default, but this becomes %gs:40 with -mcmodel=kernel
(which Xen doesn't use, but is necessary to know if anyone wants to look
at how Linux does this.)

Xen on x86 uses neither %fs nor %gs for per-cpu variables, because PV
guests do.  This happens to have saved us from a number of privilege
escalation corner cases that exist in the x86 architecture.  I'm very
reluctant to open ourselves up to such problems just to support
stack-protector.


With GCC 8.1, we get -mstack-protector-{guard,reg,offset} to fine-tune
things a whole lot more.  I am not sure when these got into Clang.

In particular, -mstack-protector-guard=global lets us have a scheme that
seems to be the same as the ARM/RISCV, which is probably good enough to
start with, although it involves restarting the toolchain debate and is
probably not something you want to do in this series.

See
https://lore.kernel.org/lkml/20241105155801.1779119-1-brgerst@gmail.com/T/#u
for most of the gory details.

> I also tested build-ability for RISCV platform, but didn't tested that
> it does not break anything, so we will need RISCV maintainer's
> approval.

There's a RISC-V smoke test in gitlab CI, but you can run it locally too.

~Andrew

Re: [PATCH 3/3] xen: riscv: enable stack protector feature

[Jan Beulich]

On 22.11.2024 22:07, Volodymyr Babchuk wrote:
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -5,6 +5,7 @@ config RISCV
>  	select HAS_DEVICE_TREE
>  	select HAS_PMAP
>  	select HAS_VMAP
> +	select HAS_STACK_PROTECTOR

Please maintain alphabetic sorting here (perhaps similarly in the Arm change).

Jan

Re: [PATCH 3/3] xen: riscv: enable stack protector feature

[oleksii.kurochko]

On Fri, 2024-11-22 at 21:07 +0000, Volodymyr Babchuk wrote:
> Enable previously added CONFIG_STACK_PROTECTOR feature for RISC-V
> platform. Here we can call boot_stack_chk_guard_setup() in
> start_xen()
> function, because it never returns, so stack protector code will not
> be triggered because of changed canary.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> I tested this patch for buildability, but didn't verified that Xen
> really boots on RISC-V when this feature is enabled. So I am asking
> RISC-V maintainers to provide feedback on it.
> ---
I've tested on staging and on downstream branches everything is fine:
Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

>  xen/arch/riscv/Kconfig | 1 +
>  xen/arch/riscv/setup.c | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> index 1858004676..1481f23b66 100644
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -5,6 +5,7 @@ config RISCV
>  	select HAS_DEVICE_TREE
>  	select HAS_PMAP
>  	select HAS_VMAP
> +	select HAS_STACK_PROTECTOR
>  
>  config RISCV_64
>  	def_bool y
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index e29bd75d7c..cd71748d2c 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -7,6 +7,7 @@
>  #include <xen/init.h>
>  #include <xen/mm.h>
>  #include <xen/shutdown.h>
> +#include <xen/stack_protector.h>
>  
>  #include <public/version.h>
>  
> @@ -55,6 +56,8 @@ void __init noreturn start_xen(unsigned long
> bootcpu_id,
>      if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
>          BUG();
>  
> +    boot_stack_chk_guard_setup();
> +
>      cmdline = boot_fdt_cmdline(device_tree_flattened);
>      printk("Command line: %s\n", cmdline);
>      cmdline_parse(cmdline);

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

[Andrew Cooper]

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

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

[Julien Grall]

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

[Volodymyr Babchuk]

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