[PATCH v2 0/4] Add stack protector

[PATCH v2 0/4] 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.

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
this will change minimal GCC requirement, which is also hard due to
various reasons. So, this series focus mostly on ARM and RISCV.

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
  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                   | 17 ++++++++++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 10 ++++++++++
 xen/include/xen/stack-protector.h    | 29 ++++++++++++++++++++++++++++
 13 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.47.1

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

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

---

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                | 17 +++++++++++++++++
 xen/common/Makefile               |  1 +
 xen/common/stack-protector.c      | 10 ++++++++++
 xen/include/xen/stack-protector.h | 29 +++++++++++++++++++++++++++++
 5 files changed, 61 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 34ed8c0fc7..0de0101fd0 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -432,7 +432,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 90268d9249..64fd04f805 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
 
@@ -213,6 +216,20 @@ config SPECULATIVE_HARDEN_LOCK
 
 endmenu
 
+menu "Compiler options"
+
+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
+
 config DIT_DEFAULT
 	bool "Data Independent Timing default"
 	depends on HAS_DIT
diff --git a/xen/common/Makefile b/xen/common/Makefile
index b279b09bfb..ceb5b2f32b 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..b258590d3a
--- /dev/null
+++ b/xen/common/stack-protector.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <xen/lib.h>
+#include <xen/random.h>
+
+unsigned long __ro_after_init __stack_chk_guard;
+
+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..779d7cf9ec
--- /dev/null
+++ b/xen/include/xen/stack-protector.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef XEN__STACK_PROTECTOR_H
+#define XEN__STACK_PROTECTOR_H
+
+#ifdef CONFIG_STACKPROTECTOR
+
+extern unsigned long __stack_chk_guard;
+
+/*
+ * This function should be always inlined. Also it should be called
+ * from a function that never returns or a function that with
+ * stack-protector disabled.
+ */
+static always_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.1

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

[Jan Beulich]

On 30.11.2024 02:10, Volodymyr Babchuk wrote:
> --- /dev/null
> +++ b/xen/common/stack-protector.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <xen/lib.h>
> +#include <xen/random.h>
> +
> +unsigned long __ro_after_init __stack_chk_guard;
> +
> +void __stack_chk_fail(void)
> +{
> +    panic("Detected stack corruption\n");
> +}

That's very little information that'll end up in the log to understand
what has gone wrong.

> --- /dev/null
> +++ b/xen/include/xen/stack-protector.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN__STACK_PROTECTOR_H
> +#define XEN__STACK_PROTECTOR_H
> +
> +#ifdef CONFIG_STACKPROTECTOR
> +
> +extern unsigned long __stack_chk_guard;
> +
> +/*
> + * This function should be always inlined. Also it should be called
> + * from a function that never returns or a function that with
> + * stack-protector disabled.
> + */
> +static always_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;
> +}

The hard tabs here make it look like Linux style, when - unless there's a
specific reason - new files want to be Xen style.

Jan

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

[Andrew Cooper]

On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 90268d9249..64fd04f805 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -213,6 +216,20 @@ config SPECULATIVE_HARDEN_LOCK
>  
>  endmenu
>  
> +menu "Compiler options"
> +
> +config STACK_PROTECTOR
> +	bool "Stack protection"

Call this "Stack Protector".  There is no point deviating from the name
most people know.

> +	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.

This final sentence isn't interesting.  All hardening options come with
a cost, and stack protector is small compared to some we have in Xen. 
Furthermore, the audience you need to write for is the curious power
user, not a developer.

How about this:

"Enable the Stack Protector compiler hardening option.  This inserts a
canary value in the stack frame of functions, and performs an integrity
check on exit."

> diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c
> new file mode 100644
> index 0000000000..b258590d3a
> --- /dev/null
> +++ b/xen/common/stack-protector.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <xen/lib.h>
> +#include <xen/random.h>
> +
> +unsigned long __ro_after_init __stack_chk_guard;
> +
> +void __stack_chk_fail(void)

asmlinkage.  This MISRA check is now blocking in Eclair.

> +{
> +    panic("Detected stack corruption\n");

At a bare minimum, "Stack Protector integrity violation identified in
%ps\n", __builtin_return_address(0)

It's a little awkward because ending up here means a sibling call from
the same function ended up corrupting the stack, but there's no way of
tracking down which.

> +}
> diff --git a/xen/include/xen/stack-protector.h b/xen/include/xen/stack-protector.h
> new file mode 100644
> index 0000000000..779d7cf9ec
> --- /dev/null
> +++ b/xen/include/xen/stack-protector.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN__STACK_PROTECTOR_H
> +#define XEN__STACK_PROTECTOR_H
> +
> +#ifdef CONFIG_STACKPROTECTOR

This is the header needing to include random.h, or it won't compile in
isolation.

~Andrew

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

[Volodymyr Babchuk]

Hi Andrew,

I addressed almost all your comments, but didn't get this one:

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

> On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:

[...]


>> diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c
>> new file mode 100644
>> index 0000000000..b258590d3a
>> --- /dev/null
>> +++ b/xen/common/stack-protector.c
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +#include <xen/lib.h>
>> +#include <xen/random.h>
>> +
>> +unsigned long __ro_after_init __stack_chk_guard;
>> +
>> +void __stack_chk_fail(void)
>
> asmlinkage.  This MISRA check is now blocking in Eclair.

I can see what we might want to mark it as "noreturn", but I don't
understand why you want to use asmlinkage. It is not called from
assembly.


-- 
WBR, Volodymyr

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

[Andrew Cooper]

On 05/12/2024 3:34 am, Volodymyr Babchuk wrote:
>>> diff --git a/xen/common/stack-protector.c b/xen/common/stack-protector.c
>>> new file mode 100644
>>> index 0000000000..b258590d3a
>>> --- /dev/null
>>> +++ b/xen/common/stack-protector.c
>>> @@ -0,0 +1,10 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +#include <xen/lib.h>
>>> +#include <xen/random.h>
>>> +
>>> +unsigned long __ro_after_init __stack_chk_guard;
>>> +
>>> +void __stack_chk_fail(void)
>> asmlinkage.  This MISRA check is now blocking in Eclair.
> I can see what we might want to mark it as "noreturn", but I don't
> understand why you want to use asmlinkage. It is not called from
> assembly.

It's not really about "called from assembly", as "called from something
Eclair can't see".

There's no declaration of __stack_chk_fail() (Rule 8.4 violation), and
no visible callsites (Rule 2.1 violation).

asmlinkage is an annotation that Eclair knows about, for the purpose of
identifying C construct such as this.

~Andrew

[PATCH v2 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 projects (Hypervisor in this case)
can enable/disable this feature by themselves.

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

---

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 ++
 5 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Config.mk b/Config.mk
index fa0414055b..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
+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..34ed8c0fc7 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -432,6 +432,8 @@ else
 CFLAGS_UBSAN :=
 endif
 
+CFLAGS += -fno-stack-protector
+
 ifeq ($(CONFIG_LTO),y)
 CFLAGS += -flto
 LDFLAGS-$(CONFIG_CC_IS_CLANG) += -plugin LLVMgold.so
-- 
2.47.1

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

[Jan Beulich]

On 30.11.2024 02:10, 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 projects (Hypervisor in this case)
> can enable/disable this feature by themselves.

s/projects/components/ ?

> --- 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__
> --- 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)
> --- 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)

Is use of cc-option-add really necessary throughout here, when ...

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -432,6 +432,8 @@ else
>  CFLAGS_UBSAN :=
>  endif
>  
> +CFLAGS += -fno-stack-protector

... is isn't needed here? Iirc the compiler version ranges supported don't
vary between components. Then again afaics $(EMBEDDED_EXTRA_CFLAGS) is used
by x86 only right now, and with cc-options-add, so perhaps it (a) needs
using cc-options-add here, too, and (b) it wants explaining why this needs
generalizing from x86 to all architectures. Quite possibly hypervisor use
of $(EMBEDDED_EXTRA_CFLAGS) may want generalizing separately, up front?

Jan

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

[Andrew Cooper]

On 02/12/2024 8:06 am, Jan Beulich wrote:
> On 30.11.2024 02:10, 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 projects (Hypervisor in this case)
>> can enable/disable this feature by themselves.
> s/projects/components/ ?
>
>> --- 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__
>> --- 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)
>> --- 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)
> Is use of cc-option-add really necessary throughout here, when ...
>
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -432,6 +432,8 @@ else
>>  CFLAGS_UBSAN :=
>>  endif
>>  
>> +CFLAGS += -fno-stack-protector
> ... is isn't needed here? Iirc the compiler version ranges supported don't
> vary between components. Then again afaics $(EMBEDDED_EXTRA_CFLAGS) is used
> by x86 only right now, and with cc-options-add, so perhaps it (a) needs
> using cc-options-add here, too, and (b) it wants explaining why this needs
> generalizing from x86 to all architectures. Quite possibly hypervisor use
> of $(EMBEDDED_EXTRA_CFLAGS) may want generalizing separately, up front?

EMBEDDED_EXTRA_CFLAGS uses cc-*-add because some options are (/were) not
accepted by compilers.  Notably -fno-stack-protector-all (found from v1
of this series), and prior to that, -no-pie which as I recall is an LD
option not a CC option.

All supported compilers know -fno-stack-protector (found when checking
-fno-stack-protector-all) so it can be added to plain CFLAGS everywhere,
not only in xen/

~Andrew

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

[Jan Beulich]

On 02.12.2024 09:06, Jan Beulich wrote:
> On 30.11.2024 02:10, 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 projects (Hypervisor in this case)
>> can enable/disable this feature by themselves.
> 
> s/projects/components/ ?
> 
>> --- 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__
>> --- 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)
>> --- 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)
> 
> Is use of cc-option-add really necessary throughout here, when ...
> 
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -432,6 +432,8 @@ else
>>  CFLAGS_UBSAN :=
>>  endif
>>  
>> +CFLAGS += -fno-stack-protector
> 
> ... is isn't needed here? Iirc the compiler version ranges supported don't
> vary between components. Then again afaics $(EMBEDDED_EXTRA_CFLAGS) is used
> by x86 only right now, and with cc-options-add, so perhaps it (a) needs
> using cc-options-add here, too, and (b) it wants explaining why this needs
> generalizing from x86 to all architectures. Quite possibly hypervisor use
> of $(EMBEDDED_EXTRA_CFLAGS) may want generalizing separately, up front?

Correction: Except for PPC all architectures consume $(EMBEDDED_EXTRA_CFLAGS)
right now. So the moving is less of a generalization than I first thought. I
still need to get used to passing -R (rather than -r) to grep, to find all
instances I'm after ...

Jan

[PATCH v2 3/4] 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>

---

In v2:
 - Reordered Kconfig entry
---
 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 23bbc91aad..a24c88c327 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -16,6 +16,7 @@ config ARM
 	select HAS_ALTERNATIVE if HAS_VMAP
 	select HAS_DEVICE_TREE
 	select HAS_PASSTHROUGH
+	select HAS_STACK_PROTECTOR
 	select HAS_UBSAN
 	select IOMMU_FORCE_PT_SHARE
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2e27af4560..f855e97e25 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.1

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

[Julien Grall]

Hi Volodymyr,

On 30/11/2024 01:10, Volodymyr Babchuk wrote:
> 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.

It would be good to explain how you decided to call...

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> In v2:
>   - Reordered Kconfig entry
> ---
>   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 23bbc91aad..a24c88c327 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -16,6 +16,7 @@ config ARM
>   	select HAS_ALTERNATIVE if HAS_VMAP
>   	select HAS_DEVICE_TREE
>   	select HAS_PASSTHROUGH
> +	select HAS_STACK_PROTECTOR
>   	select HAS_UBSAN
>   	select IOMMU_FORCE_PT_SHARE
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2e27af4560..f855e97e25 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();

... the function here. If I am not mistaken, at this point, cpu_khz 
(used by NOW() in get_random()) would be zero. It is only initialized by 
preinit_xen_time() which happens later.

So I think it should be called a bit further down and gain a comment to 
about the call dependencies.

Cheers,

-- 
Julien Grall

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

[Andrew Cooper]

On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2e27af4560..f855e97e25 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -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");

I still think that __stack_chk_guard wants setting up in ASM before
entering C.

The only reason this call is so late is because Xen's get_random()
sequence is less than helpful.  That wants rewriting somewhat, but maybe
now isn't the best time.

Even if you initialise __stack_chk_guard it to -1 rather than 0, it's
still got a better chance of catching errors during very early boot; the
instrumentation is present, but is using 0 as the canary value.

On x86, dumping the current TSC value into __stack_chk_guard would be
far better than using -1.  Even if it skewed to a lower number, it's
unpredictable and not going to reoccur by accident during a stack overrun.

Surely ARM has something similar it could use?

[edit] Yes, get_cycles(), which every architecture seems to have.  In
fact, swapping get_random() from NOW() to get_cycles() would be good
enough to get it usable from early assembly.

~Andrew

A better option for get_random() would be to use a proven PRNG (e.g. one
of the xorshift family), seeded with get_cycles() and then re-seeded
with a real RDRAND/etc instruction if such a capability is found to
exist on the hardware.

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

[Julien Grall]

On Tue, 3 Dec 2024 at 22:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 2e27af4560..f855e97e25 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -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");
>
> I still think that __stack_chk_guard wants setting up in ASM before
> entering C.
>
> The only reason this call is so late is because Xen's get_random()
> sequence is less than helpful.  That wants rewriting somewhat, but maybe
> now isn't the best time.
>
> Even if you initialise __stack_chk_guard it to -1 rather than 0, it's
> still got a better chance of catching errors during very early boot; the
> instrumentation is present, but is using 0 as the canary value.
>
> On x86, dumping the current TSC value into __stack_chk_guard would be
> far better than using -1.  Even if it skewed to a lower number, it's
> unpredictable and not going to reoccur by accident during a stack overrun.
>
> Surely ARM has something similar it could use?

There is a optional system register to read a random number.

>
> [edit] Yes, get_cycles(), which every architecture seems to have.  In
> fact, swapping get_random() from NOW() to get_cycles() would be good
> enough to get it usable from early assembly.

Not quite. Technically we can't rely on the timer counter until
platform_init_time() is called.
This was to cater an issue on the exynos we used in OssTest. But
arguably this is the exception
rather than the norm because the firmware ought to properly initialize
the timer...

I haven't checked recent firmware. But I could be convinced to access
the counter before
hand if we really think that setting __stack_chk_guard from ASM is much better.

Cheers,

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

[Andrew Cooper]

On 03/12/2024 11:16 pm, Julien Grall wrote:
> On Tue, 3 Dec 2024 at 22:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 2e27af4560..f855e97e25 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -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");
>> I still think that __stack_chk_guard wants setting up in ASM before
>> entering C.
>>
>> The only reason this call is so late is because Xen's get_random()
>> sequence is less than helpful.  That wants rewriting somewhat, but maybe
>> now isn't the best time.
>>
>> Even if you initialise __stack_chk_guard it to -1 rather than 0, it's
>> still got a better chance of catching errors during very early boot; the
>> instrumentation is present, but is using 0 as the canary value.
>>
>> On x86, dumping the current TSC value into __stack_chk_guard would be
>> far better than using -1.  Even if it skewed to a lower number, it's
>> unpredictable and not going to reoccur by accident during a stack overrun.
>>
>> Surely ARM has something similar it could use?
> There is a optional system register to read a random number.

Only in v8.5 as far as I can see, and even then it's not required. 
Also, it suffers from the same problem as RDRAND on x86; we need to boot
to at least feature detection before we can safely use it if it's available.

>
>> [edit] Yes, get_cycles(), which every architecture seems to have.  In
>> fact, swapping get_random() from NOW() to get_cycles() would be good
>> enough to get it usable from early assembly.
> Not quite. Technically we can't rely on the timer counter until
> platform_init_time() is called.
> This was to cater an issue on the exynos we used in OssTest. But
> arguably this is the exception
> rather than the norm because the firmware ought to properly initialize
> the timer...
>
> I haven't checked recent firmware. But I could be convinced to access
> the counter before
> hand if we really think that setting __stack_chk_guard from ASM is much better.

The C instrumentation is always present, right from the very start of
start_xen().

Even working with a canary of 0, there's some value.  It will spot
clobbering with a non-zero value, but it won't spot e.g. an overly-long
memset(, 0).

During boot, we're not defending against a malicious entity.  Simply
defending against bad developer expectations.

Therefore, anything to get a non-zero value prior to entering C will be
an improvement.  Best-effort is fine, and if there's one platform with
an errata that causes it to miss out, then oh well.  Any other platform
which manifests a crash will still lead to the problem being fixed.

I suppose taking this argument to it's logical conclusion, we could
initialise __stack_chk_guard with a poison pattern, although not one
shared by any other poison pattern in Xen.  That alone would be better
than using 0 in early boot.

~Andrew

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

[Volodymyr Babchuk]

Hi Andrew,

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

> On 03/12/2024 11:16 pm, Julien Grall wrote:
>> On Tue, 3 Dec 2024 at 22:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index 2e27af4560..f855e97e25 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -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");
>>> I still think that __stack_chk_guard wants setting up in ASM before
>>> entering C.
>>>
>>> The only reason this call is so late is because Xen's get_random()
>>> sequence is less than helpful.  That wants rewriting somewhat, but maybe
>>> now isn't the best time.
>>>
>>> Even if you initialise __stack_chk_guard it to -1 rather than 0, it's
>>> still got a better chance of catching errors during very early boot; the
>>> instrumentation is present, but is using 0 as the canary value.
>>>
>>> On x86, dumping the current TSC value into __stack_chk_guard would be
>>> far better than using -1.  Even if it skewed to a lower number, it's
>>> unpredictable and not going to reoccur by accident during a stack overrun.
>>>
>>> Surely ARM has something similar it could use?
>> There is a optional system register to read a random number.
>
> Only in v8.5 as far as I can see, and even then it's not required. 
> Also, it suffers from the same problem as RDRAND on x86; we need to boot
> to at least feature detection before we can safely use it if it's available.
>
>>
>>> [edit] Yes, get_cycles(), which every architecture seems to have.  In
>>> fact, swapping get_random() from NOW() to get_cycles() would be good
>>> enough to get it usable from early assembly.
>> Not quite. Technically we can't rely on the timer counter until
>> platform_init_time() is called.
>> This was to cater an issue on the exynos we used in OssTest. But
>> arguably this is the exception
>> rather than the norm because the firmware ought to properly initialize
>> the timer...
>>
>> I haven't checked recent firmware. But I could be convinced to access
>> the counter before
>> hand if we really think that setting __stack_chk_guard from ASM is much better.
>
> The C instrumentation is always present, right from the very start of
> start_xen().
>
> Even working with a canary of 0, there's some value.  It will spot
> clobbering with a non-zero value, but it won't spot e.g. an overly-long
> memset(, 0).
>
> During boot, we're not defending against a malicious entity.  Simply
> defending against bad developer expectations.
>
> Therefore, anything to get a non-zero value prior to entering C will be
> an improvement.  Best-effort is fine, and if there's one platform with
> an errata that causes it to miss out, then oh well.  Any other platform
> which manifests a crash will still lead to the problem being fixed.
>
> I suppose taking this argument to it's logical conclusion, we could
> initialise __stack_chk_guard with a poison pattern, although not one
> shared by any other poison pattern in Xen.  That alone would be better
> than using 0 in early boot.

Okay, so I come with three-stage initialization:

1. Static poison pattern
2. Time-based early value
3. Random-number based long-term value

So, apart from already present

static always_inline void boot_stack_chk_guard_setup(void);

I did this:

/*
 * Initial value is chosen by fair dice roll.
 * It will be updated during boot process.
 */
#if BITS_PER_LONG == 32
unsigned long __ro_after_init __stack_chk_guard = 0xdd2cc927;
#else
unsigned long __ro_after_init __stack_chk_guard = 0x2d853605a4d9a09c;
#endif

/* This function should be called from ASM only */
void __init asmlinkage boot_stack_chk_guard_setup_early(void)
{
    /*
     * Linear congruent generator. 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 = 2891336453;
#else
    const unsigned long a = 2862933555777941757;
#endif
    const unsigned c = 1;
    unsigned long cycles = get_cycles();

    if ( !cycles )
        return;

    __stack_chk_guard = cycles * a + c;
}

boot_stack_chk_guard_setup_early() is being called by ASM code during
early boot stages.

Then, later, boot_stack_chk_guard_setup() is called.

If you are okay with this approach, I'll send the next version.

-- 
WBR, Volodymyr

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

[Jan Beulich]

On 06.12.2024 05:46, Volodymyr Babchuk wrote:
> So, apart from already present
> 
> static always_inline void boot_stack_chk_guard_setup(void);
> 
> I did this:
> 
> /*
>  * Initial value is chosen by fair dice roll.
>  * It will be updated during boot process.
>  */
> #if BITS_PER_LONG == 32
> unsigned long __ro_after_init __stack_chk_guard = 0xdd2cc927;

At least this and ...

> #else
> unsigned long __ro_after_init __stack_chk_guard = 0x2d853605a4d9a09c;
> #endif
> 
> /* This function should be called from ASM only */
> void __init asmlinkage boot_stack_chk_guard_setup_early(void)
> {
>     /*
>      * Linear congruent generator. 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 = 2891336453;

... this will need a UL suffix for Misra. Probably best to add UL on all
four constants.

As to the comment, please adhere to ./CODING_STYLE.

> #else
>     const unsigned long a = 2862933555777941757;
> #endif
>     const unsigned c = 1;

I'm having a hard time seeing why this need to be a static variable. Its
sole use is ...

>     unsigned long cycles = get_cycles();
> 
>     if ( !cycles )
>         return;
> 
>     __stack_chk_guard = cycles * a + c;

... here, where you can as well write a literal 1.

Jan

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

[Volodymyr Babchuk]

Hi Jan,

Jan Beulich <jbeulich@suse.com> writes:

[...]


>
>> #else
>>     const unsigned long a = 2862933555777941757;
>> #endif
>>     const unsigned c = 1;
>
> I'm having a hard time seeing why this need to be a static variable. Its
> sole use is ...

It's a constant in a hope that compiler is smart enough to optimize it out.

>>     unsigned long cycles = get_cycles();
>> 
>>     if ( !cycles )
>>         return;
>> 
>>     __stack_chk_guard = cycles * a + c;
>
> ... here, where you can as well write a literal 1.

For readability. Formula for LCG is X_n+1 = (X_n * a + c) mod m.

-- 
WBR, Volodymyr

[PATCH v2 4/4] 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>
Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

---

In v2:
 - Reordered Kconfig entry
 - Added Oleksii's Tested-by tag
---
 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..79b3b68754 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -4,6 +4,7 @@ config RISCV
 	select GENERIC_BUG_FRAME
 	select HAS_DEVICE_TREE
 	select HAS_PMAP
+	select HAS_STACK_PROTECTOR
 	select HAS_VMAP
 
 config RISCV_64
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9680332fee..59eddb465a 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 <xen/vmap.h>
 
 #include <public/version.h>
@@ -57,6 +58,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.1

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

[Jan Beulich]

On 30.11.2024 02:10, 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>
> Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Isn't this premature? For ...

> @@ -57,6 +58,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();

... this function's use of get_random(), either arch_get_random() needs
to be implemented, or (as Julien also pointed out for Arm64) NOW() needs
to work. Yet get_s_time() presently expands to just BUG_ON(). Given this
it's not even clear to me how Oleksii managed to actually test this.

Jan

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

[oleksii.kurochko]

On Mon, 2024-12-02 at 09:12 +0100, Jan Beulich wrote:
> On 30.11.2024 02:10, 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>
> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Isn't this premature? For ...
> 
> > @@ -57,6 +58,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();
> 
> ... this function's use of get_random(), either arch_get_random()
> needs
> to be implemented, or (as Julien also pointed out for Arm64) NOW()
> needs
> to work. Yet get_s_time() presently expands to just BUG_ON(). Given
> this
> it's not even clear to me how Oleksii managed to actually test this.
I will double check that but it worked for me ( I didn't face BUG_ON()
).

~ Oleksii

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

[oleksii.kurochko]

On Mon, 2024-12-02 at 09:12 +0100, Jan Beulich wrote:
> On 30.11.2024 02:10, 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>
> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Isn't this premature? For ...
> 
> > @@ -57,6 +58,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();
> 
> ... this function's use of get_random(), either arch_get_random()
> needs
> to be implemented, or (as Julien also pointed out for Arm64) NOW()
> needs
> to work. Yet get_s_time() presently expands to just BUG_ON(). Given
> this
> it's not even clear to me how Oleksii managed to actually test this.
Okay, I think I found what is the problem and why my testing on staging
wasn't really correct.

In xen/include/xen/stack_protector.h (
https://patchew.org/Xen/20241130010954.36057-1-volodymyr._5Fbabchuk@epam.com/20241130010954.36057-3-volodymyr._5Fbabchuk@epam.com/
) CONFIG_STACKPROTECTOR is used for ifdef-ing so the stub version of 
boot_stack_chk_guard_setup() is used and there is no need for
get_random() implementation:
1. Shouldn't it be /s/CONFIG_STACKPROTECTOR/CONFIG_STACK_PROTECTOR ?
2. CONFIG_STACK_PROTECTOR isn't set in case of RISC-V, at least:
      grep -Rni "STACK_PROTECTOR" xen/.config 
      38:CONFIG_HAS_STACK_PROTECTOR=y
      76:# CONFIG_STACK_PROTECTOR is not set
   
   Shouldn't it be default HAS_STACK_PROTECTOR ( or something similar )
   for 'config STACK_PROTECTOR':
      diff --git a/xen/common/Kconfig b/xen/common/Kconfig
      index 0ffd825510..f3156dbb9a 100644
      --- a/xen/common/Kconfig
      +++ b/xen/common/Kconfig
      @@ -521,6 +521,7 @@ config TRACEBUFFER
       
       config STACK_PROTECTOR
              bool "Stack protection"
      +       default HAS_STACK_PROTECTOR
              depends on HAS_STACK_PROTECTOR
              help
                Use compiler's option -fstack-protector (supported both by
      GCC
      
With these changes, I can confirm Jan's statement that the BUG_ON()
occurs due to the absence of the get_random()/NOW() implementation.

My second test (on my downstream branch) wasn't relevant because
get_s_time() and NOW() are implemented there.


Aside from the changes I mentioned above, I can re-send this patch when
I submit the patch enabling get_s_time() and NOW() for RISC-V. If you,
Volodymyr, are okay with that, we can proceed in this way.

Best regards,
 Oleksii

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

[Volodymyr Babchuk]

Hello Oleksii,


oleksii.kurochko@gmail.com writes:

> On Mon, 2024-12-02 at 09:12 +0100, Jan Beulich wrote:
>> On 30.11.2024 02:10, 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>
>> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>
>> Isn't this premature? For ...
>>
>> > @@ -57,6 +58,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();
>>
>> ... this function's use of get_random(), either arch_get_random()
>> needs
>> to be implemented, or (as Julien also pointed out for Arm64) NOW()
>> needs
>> to work. Yet get_s_time() presently expands to just BUG_ON(). Given
>> this
>> it's not even clear to me how Oleksii managed to actually test this.
> Okay, I think I found what is the problem and why my testing on staging
> wasn't really correct.
>
> In xen/include/xen/stack_protector.h (
> https://patchew.org/Xen/20241130010954.36057-1-volodymyr._5Fbabchuk@epam.com/20241130010954.36057-3-volodymyr._5Fbabchuk@epam.com/
> ) CONFIG_STACKPROTECTOR is used for ifdef-ing so the stub version of
> boot_stack_chk_guard_setup() is used and there is no need for
> get_random() implementation:
> 1. Shouldn't it be /s/CONFIG_STACKPROTECTOR/CONFIG_STACK_PROTECTOR ?
> 2. CONFIG_STACK_PROTECTOR isn't set in case of RISC-V, at least:
>       grep -Rni "STACK_PROTECTOR" xen/.config
>       38:CONFIG_HAS_STACK_PROTECTOR=y
>       76:# CONFIG_STACK_PROTECTOR is not set
>
>    Shouldn't it be default HAS_STACK_PROTECTOR ( or something similar )
>    for 'config STACK_PROTECTOR':
>       diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>       index 0ffd825510..f3156dbb9a 100644
>       --- a/xen/common/Kconfig
>       +++ b/xen/common/Kconfig
>       @@ -521,6 +521,7 @@ config TRACEBUFFER
>
>        config STACK_PROTECTOR
>               bool "Stack protection"
>       +       default HAS_STACK_PROTECTOR
>               depends on HAS_STACK_PROTECTOR
>               help
>                 Use compiler's option -fstack-protector (supported both by
>       GCC
>
> With these changes, I can confirm Jan's statement that the BUG_ON()
> occurs due to the absence of the get_random()/NOW() implementation.
>
> My second test (on my downstream branch) wasn't relevant because
> get_s_time() and NOW() are implemented there.
>
>
> Aside from the changes I mentioned above, I can re-send this patch when
> I submit the patch enabling get_s_time() and NOW() for RISC-V. If you,
> Volodymyr, are okay with that, we can proceed in this way.

Thank you for testing this once more. I'll drop this patch from v3 then, so
you'll be able to include it into your series.

--
WBR, Volodymyr