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

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

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/arm64/head.S            |  3 ++
 xen/arch/x86/boot/Makefile           |  1 +
 xen/common/Kconfig                   | 15 ++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 51 ++++++++++++++++++++++++++++
 12 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c

-- 
2.47.1

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

---

Changes in v4:
 - Removed third phase of initialization (it was using Xen's RNG)
 - remove stack-protector.h because it is not required anymore
 - Reworded comments
 - __stack_chk_fail() now dumps execution state before calling panic()
 - "Compiler option" Kconfig entry renamed to "Other hardening"

Changes in v3:
 - Fixed coding style in stack-protector.h
 - Extended panic() message
 - Included missed random.h
 - Renamed Kconfig option
 - Used Andrew's suggestion for the Kconfig help text
 - Added "asmlinkage" attribute to __stack_chk_fail() to make Eclair
 happy
 - Initial stack guard value is random
 - Added LCG to generate stack guard value at early boot stages
 - Added comment to asm-generic/random.h about dependencies
 - Extended the commit message

Changes in v2:
 - Moved changes to EMBEDDED_EXTRA_CFLAGS into separate patch
 - Renamed stack_protector.c to stack-protector.c
 - Renamed stack_protector.h to stack-protector.h
 - Removed #ifdef CONFIG_X86 in stack-protector.h
 - Updated comment in stack-protector.h
   (also, we can't call boot_stack_chk_guard_setup() from asm code in
   general case, because it calls get_random() and get_random() may
   depend in per_cpu infrastructure, which is initialized later)
 - Fixed coding style
 - Moved CONFIG_STACK_PROTECTOR into newly added "Compiler options"
 submenu
 - Marked __stack_chk_guard as __ro_after_init
---
 xen/Makefile                 |  4 +++
 xen/common/Kconfig           | 15 +++++++++++
 xen/common/Makefile          |  1 +
 xen/common/stack-protector.c | 51 ++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 xen/common/stack-protector.c

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..8fa9f6147f
--- /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");
+}
-- 
2.47.1

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

---

Changes in v4:
 - Removed stray hunk
 - Added x86_32 CFLAG
 - Added Jan's R-b tag
Changes in v3:
 - Reword commit message
 - Use CFLAGS += instead of cc-optios-add
Changes in v2:
 - New in v2
---
 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 v4 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>
---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8507e6556a..62e6c26aaf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -22,6 +22,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.
 
-- 
2.47.1

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

[Volodymyr Babchuk]

Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
platform. We initialize stack protector very early, in head.S using
boot_stack_chk_guard_setup. This ensures that all C code from the very
beginning can use stack protector.

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

---
In v4:
 - setup.c does not call boot_stack_chk_guard_setup() anymore, because
   the original implementation was removed and
   boot_stack_chk_guard_setup_early was renamed to boot_stack_chk_guard_setup
In v3:
 - Call boot_stack_chk_guard_setup_early from head.S to ensure
   that stack is protected from early boot stages
 - Call boot_stack_chk_guard_setup() later, when time subsystem is
   sufficiently initialized to provide values for the random number
   generator.
In v2:
 - Reordered Kconfig entry
---
 xen/arch/arm/Kconfig      | 1 +
 xen/arch/arm/arm64/head.S | 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/arm64/head.S b/xen/arch/arm/arm64/head.S
index 72c7b24498..5cbd62af86 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -250,6 +250,9 @@ real_start_efi:
 #endif
         PRINT("- Boot CPU booting -\r\n")
 
+#ifdef CONFIG_STACK_PROTECTOR
+        bl    boot_stack_chk_guard_setup
+#endif
         bl    check_cpu_mode
         bl    cpu_init
 
-- 
2.47.1

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

[Andrew Cooper]

On 14/01/2025 4:25 am, Volodymyr Babchuk wrote:
> diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c
> new file mode 100644
> index 0000000000..8fa9f6147f
> --- /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;

Indentation.  Can be fixed on commit.

Everything else LGTM.

~Andrew

Re: [PATCH v4 0/4] Add/enable stack protector

[Andrew Cooper]

On 14/01/2025 4:25 am, 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

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

There's one minor formatting error which can be fixed on commit.

~Andrew

Re: [PATCH v4 0/4] Add/enable stack protector

[Volodymyr Babchuk]

Hi Andrew,

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

> On 14/01/2025 4:25 am, 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
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> There's one minor formatting error which can be fixed on commit.
>
> ~Andrew

Thanks for the review. I noticed that this series is not committed. Is
there anything else required from my side?

-- 
WBR, Volodymyr

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

[Andrew Cooper]

On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
> Hi Andrew,
>
> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>
>> On 14/01/2025 4:25 am, 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
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> There's one minor formatting error which can be fixed on commit.
>>
>> ~Andrew
> Thanks for the review. I noticed that this series is not committed. Is
> there anything else required from my side?
>

You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
enough.

And at this point at rc4, you'll need to persuade Oleksii to take it for
4.20.

Personally I think it's low risk and worthwhile to take for 4.20, and it
was technically completed in time - it just fell between the cracks.

~Andrew

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]


On 1/14/25 5:25 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>

Reviewed-By: Oleksii Kurochko<oleksii.kurochko@gmail.com>

Thanks.

~ Oleksii

> ---
>   CHANGELOG.md | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 8507e6556a..62e6c26aaf 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -22,6 +22,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.
>   

[-- Attachment #2: Type: text/html, Size: 1721 bytes --]

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

[Julien Grall]

Hi,

On 14/01/2025 04:25, Volodymyr Babchuk wrote:
> Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
> platform. We initialize stack protector very early, in head.S using
> boot_stack_chk_guard_setup. This ensures that all C code from the very
> beginning can use stack protector.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> In v4:
>   - setup.c does not call boot_stack_chk_guard_setup() anymore, because
>     the original implementation was removed and
>     boot_stack_chk_guard_setup_early was renamed to boot_stack_chk_guard_setup
> In v3:
>   - Call boot_stack_chk_guard_setup_early from head.S to ensure
>     that stack is protected from early boot stages
>   - Call boot_stack_chk_guard_setup() later, when time subsystem is
>     sufficiently initialized to provide values for the random number
>     generator.
> In v2:
>   - Reordered Kconfig entry
> ---
>   xen/arch/arm/Kconfig      | 1 +
>   xen/arch/arm/arm64/head.S | 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

This is select stack protection for both 32-bit and 64-bit. Yet...

>   	select HAS_UBSAN
>   
>   config ARCH_DEFCONFIG
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S

... you only modify arm64. Why?

> index 72c7b24498..5cbd62af86 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -250,6 +250,9 @@ real_start_efi:
>   #endif
>           PRINT("- Boot CPU booting -\r\n")
>   
> +#ifdef CONFIG_STACK_PROTECTOR
> +        bl    boot_stack_chk_guard_setup
> +#endif

I don't think you can call a C function this early:
   * The stack is not set  (it is not clear why would the function not 
use it)
   * The MMU is not turned on
   * We don't follow the AAPCS

If you want to call from assembly, then I think it just needs to be 
called before launch.

Cheers,

-- 
Julien Grall

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]


On 2/13/25 3:07 PM, Andrew Cooper wrote:
> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>> Hi Andrew,
>>
>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>
>>> On 14/01/2025 4:25 am, 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
>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>
>>> There's one minor formatting error which can be fixed on commit.
>>>
>>> ~Andrew
>> Thanks for the review. I noticed that this series is not committed. Is
>> there anything else required from my side?
>>
> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
> enough.
>
> And at this point at rc4, you'll need to persuade Oleksii to take it for
> 4.20.
>
> Personally I think it's low risk and worthwhile to take for 4.20, and it
> was technically completed in time - it just fell between the cracks.

I think the same it's low risk patch series, so we can take it for 4.20:
  Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>

Thanks.

~ Oleksii

>
> ~Andrew

[-- Attachment #2: Type: text/html, Size: 2423 bytes --]

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

[Julien Grall]

Hi,

On 13/02/2025 14:21, Oleksii Kurochko wrote:
> 
> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>> Hi Andrew,
>>>
>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>
>>>> On 14/01/2025 4:25 am, 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
>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>
>>>> There's one minor formatting error which can be fixed on commit.
>>>>
>>>> ~Andrew
>>> Thanks for the review. I noticed that this series is not committed. Is
>>> there anything else required from my side?
>>>
>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>> enough.

I beg to differ. For low level code, you really ought to have Arm folks 
to confirm this is correct. In fact, I don't think patch #3 it is. So ...

>>
>> And at this point at rc4, you'll need to persuade Oleksii to take it for
>> 4.20.
>>
>> Personally I think it's low risk and worthwhile to take for 4.20, and it
>> was technically completed in time - it just fell between the cracks.
> 
> I think the same it's low risk patch series, so we can take it for 4.20:
>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>

... I should not go to 4.20 as-is.

And before someone ask why it wasn't answered early. I can't comment for 
the other Arm maintainers, but I have been away for the past two months. 
So still catching up on my emails.

Cheers,

-- 
Julien Grall

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]


On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>
>
> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>> Hi Andrew,
>>>
>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>
>>>> On 14/01/2025 4:25 am, 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
>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>
>>>> There's one minor formatting error which can be fixed on commit.
>>>>
>>>> ~Andrew
>>> Thanks for the review. I noticed that this series is not committed. Is
>>> there anything else required from my side?
>>>
>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>> enough.

Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

>> And at this point at rc4, you'll need to persuade Oleksii to take it for
>> 4.20.
>>
>> Personally I think it's low risk and worthwhile to take for 4.20, and it
>> was technically completed in time - it just fell between the cracks.
> I think the same it's low risk patch series, so we can take it for 4.20:
>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>
>
> Thanks.
>
> ~ Oleksii
>> ~Andrew

[-- Attachment #2: Type: text/html, Size: 3372 bytes --]

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

[Oleksii Kurochko]

[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]


On 2/13/25 3:24 PM, Julien Grall wrote:
> Hi,
>
> On 13/02/2025 14:21, Oleksii Kurochko wrote:
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
>
> I beg to differ. For low level code, you really ought to have Arm 
> folks to confirm this is correct. In fact, I don't think patch #3 it 
> is. So ...
>
>>>
>>> And at this point at rc4, you'll need to persuade Oleksii to take it 
>>> for
>>> 4.20.
>>>
>>> Personally I think it's low risk and worthwhile to take for 4.20, 
>>> and it
>>> was technically completed in time - it just fell between the cracks.
>>
>> I think the same it's low risk patch series, so we can take it for 4.20:
>>   Release-Acked-by: Oleksii Kurochko<olekskii.kurochko@gmail.com>
>
> ... I should not go to 4.20 as-is.
>
> And before someone ask why it wasn't answered early. I can't comment 
> for the other Arm maintainers, but I have been away for the past two 
> months. So still catching up on my emails.

Agree, I wrote that in follow-up reply to my initial reply.

So if the proper Ack will be received I still think we can consider to have it in 4.20.

~ Oleksii

>

[-- Attachment #2: Type: text/html, Size: 3894 bytes --]

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

[Jan Beulich]

On 13.02.2025 15:26, Oleksii Kurochko wrote:
> 
> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
> 
> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

At a guess Andrew found Volodymyr in the ARM section of maintainers, but
then didn't pay close attention to the R: (rather than M:).

Jan

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

[Andrew Cooper]

On 13/02/2025 2:26 pm, Oleksii Kurochko wrote:
>
>
> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>
>>
>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>> Hi Andrew,
>>>>
>>>> Andrew Cooper <andrew.cooper3@citrix.com> writes:
>>>>
>>>>> On 14/01/2025 4:25 am, 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
>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>
>>>>> ~Andrew
>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>> there anything else required from my side?
>>>>
>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>> enough.
> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.

Volodymyr is an ARM maintainer (so qualifies for the ARM requirement),
and my R-by covers the "looked over by any other person" requirement.

~Andrew

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

[Julien Grall]

On 13/02/2025 14:20, Julien Grall wrote:
> Hi,
> 
> On 14/01/2025 04:25, Volodymyr Babchuk wrote:
>> Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
>> platform. We initialize stack protector very early, in head.S using
>> boot_stack_chk_guard_setup. This ensures that all C code from the very
>> beginning can use stack protector.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>> ---
>> In v4:
>>   - setup.c does not call boot_stack_chk_guard_setup() anymore, because
>>     the original implementation was removed and
>>     boot_stack_chk_guard_setup_early was renamed to 
>> boot_stack_chk_guard_setup
>> In v3:
>>   - Call boot_stack_chk_guard_setup_early from head.S to ensure
>>     that stack is protected from early boot stages
>>   - Call boot_stack_chk_guard_setup() later, when time subsystem is
>>     sufficiently initialized to provide values for the random number
>>     generator.
>> In v2:
>>   - Reordered Kconfig entry
>> ---
>>   xen/arch/arm/Kconfig      | 1 +
>>   xen/arch/arm/arm64/head.S | 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
> 
> This is select stack protection for both 32-bit and 64-bit. Yet...
> 
>>       select HAS_UBSAN
>>   config ARCH_DEFCONFIG
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> 
> ... you only modify arm64. Why?
> 
>> index 72c7b24498..5cbd62af86 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -250,6 +250,9 @@ real_start_efi:
>>   #endif
>>           PRINT("- Boot CPU booting -\r\n")
>> +#ifdef CONFIG_STACK_PROTECTOR
>> +        bl    boot_stack_chk_guard_setup
>> +#endif
> 
> I don't think you can call a C function this early:
>    * The stack is not set  (it is not clear why would the function not 
> use it)
>    * The MMU is not turned on

Just to expand, this is a problem because Xen may not be loaded with VA 
== PA. So depending on how the GCC generated boot_stack_chk_guard_setup, 
the global variable may be accessed with an absolution address (this 
would be the VA).

This means we could overwrite "random" memory.

>    * We don't follow the AAPCS
> 
> If you want to call from assembly, then I think it just needs to be 
> called before launch.

Actually we only setup the stack in launch. So I guess some re-ordering 
is needed to setup the stack earlier.

Cheers,

-- 
Julien Grall

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

[Andrew Cooper]

On 13/02/2025 2:30 pm, Jan Beulich wrote:
> On 13.02.2025 15:26, Oleksii Kurochko wrote:
>> On 2/13/25 3:21 PM, Oleksii Kurochko wrote:
>>>
>>> On 2/13/25 3:07 PM, Andrew Cooper wrote:
>>>> On 13/02/2025 1:54 pm, Volodymyr Babchuk wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> Andrew Cooper<andrew.cooper3@citrix.com> writes:
>>>>>
>>>>>> On 14/01/2025 4:25 am, 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
>>>>>> Reviewed-by: Andrew Cooper<andrew.cooper3@citrix.com>
>>>>>>
>>>>>> There's one minor formatting error which can be fixed on commit.
>>>>>>
>>>>>> ~Andrew
>>>>> Thanks for the review. I noticed that this series is not committed. Is
>>>>> there anything else required from my side?
>>>>>
>>>> You need an ARM Ack on patch 3.  [EDIT], no you don't, my R-by is good
>>>> enough.
>> Andrew, why it is enough your R-by for patch 3? It seems like it is fully Arm related patch
>> and I expect to see Ack from Arm maintainers. Also, there is some comments from Julien.
> At a guess Andrew found Volodymyr in the ARM section of maintainers, but
> then didn't pay close attention to the R: (rather than M:).

My apologies.  Yes, I did fail to read the small print.

~Andrew