[PATCH v3 0/3] Add stack protector

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

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 v3:

 - Removed patch for riscv
 - Changes in individual patches are covered in their respect commit
 messages

Changes in v2:

 - Patch "xen: common: add ability to enable stack protector" was
   divided into two patches.
 - Rebase onto Andrew's patch that removes -fno-stack-protector-all
 - Tested on RISC-V thanks to Oleksii Kurochko
 - Changes in individual patches covered in their respect commit
 messages


Volodymyr Babchuk (3):
  common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS
  xen: common: add ability to enable stack protector
  xen: arm: enable stack protector feature

 Config.mk                            |  2 +-
 stubdom/Makefile                     |  3 ++
 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/arm/setup.c                 |  3 ++
 xen/common/Kconfig                   | 15 +++++++++
 xen/common/Makefile                  |  1 +
 xen/common/stack-protector.c         | 47 ++++++++++++++++++++++++++++
 xen/include/asm-generic/random.h     |  5 +++
 xen/include/xen/stack-protector.h    | 30 ++++++++++++++++++
 13 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100644 xen/common/stack-protector.c
 create mode 100644 xen/include/xen/stack-protector.h

-- 
2.47.1

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

[Volodymyr Babchuk]

Both GCC and Clang support -fstack-protector feature, which add stack
canaries to functions where stack corruption is possible. This patch
makes general preparations to enable this feature on different
supported architectures:

 - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture
   can enable this feature individually
 - Added user-selectable CONFIG_STACK_PROTECTOR option
 - Implemented code that sets up random stack canary and a basic
   handler for stack protector failures

Stack guard value is initialized in three phases:

1. Pre-defined randomly-selected value.

2. Early use of 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. Even when
get_cycles() is available, it's return value may be easily predicted,
especially on embedded systems, where boot time is quite consistent.

3. After hypervisor is sufficiently initialized, stack guard can be
set-up with get_random() function, which is expected to provide better
randomness.

Also this patch adds comment to asm-generic/random.h about stack
protector dependency on it.

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

---

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      | 47 +++++++++++++++++++++++++++++++
 xen/include/asm-generic/random.h  |  5 ++++
 xen/include/xen/stack-protector.h | 30 ++++++++++++++++++++
 6 files changed, 102 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..5676339a66 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,18 @@ config SPECULATIVE_HARDEN_LOCK
 
 endmenu
 
+menu "Compiler options"
+
+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 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 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..922511555f
--- /dev/null
+++ b/xen/common/stack-protector.c
@@ -0,0 +1,47 @@
+// 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 ASM only */
+void __init asmlinkage boot_stack_chk_guard_setup_early(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)
+{
+    panic("Stack Protector integrity violation identified in %ps\n",
+	  __builtin_return_address(0));
+}
diff --git a/xen/include/asm-generic/random.h b/xen/include/asm-generic/random.h
index d0d35dd217..7f6d8790c4 100644
--- a/xen/include/asm-generic/random.h
+++ b/xen/include/asm-generic/random.h
@@ -2,6 +2,11 @@
 #ifndef __ASM_GENERIC_RANDOM_H__
 #define __ASM_GENERIC_RANDOM_H__
 
+/*
+ * When implementing arch_get_random(), please make sure that
+ * it can provide random data before stack protector is initialized
+ * (i.e. before boot_stack_chk_guard_setup() is called).
+ */
 static inline unsigned int arch_get_random(void)
 {
     return 0;
diff --git a/xen/include/xen/stack-protector.h b/xen/include/xen/stack-protector.h
new file mode 100644
index 0000000000..bd324d9003
--- /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_STACK_PROTECTOR
+
+#include <xen/random.h>
+
+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 has
+ * 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_STACK_PROTECTOR  */
+
+#endif /* XEN__STACK_PROTECTOR_H */
-- 
2.47.1

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

[Jan Beulich]

On 11.12.2024 03:04, Volodymyr Babchuk wrote:
> Both GCC and Clang support -fstack-protector feature, which add stack
> canaries to functions where stack corruption is possible. This patch
> makes general preparations to enable this feature on different
> supported architectures:
> 
>  - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture
>    can enable this feature individually
>  - Added user-selectable CONFIG_STACK_PROTECTOR option
>  - Implemented code that sets up random stack canary and a basic
>    handler for stack protector failures
> 
> Stack guard value is initialized in three phases:
> 
> 1. Pre-defined randomly-selected value.
> 
> 2. Early use of 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. Even when
> get_cycles() is available, it's return value may be easily predicted,
> especially on embedded systems, where boot time is quite consistent.
> 
> 3. After hypervisor is sufficiently initialized, stack guard can be
> set-up with get_random() function, which is expected to provide better
> randomness.
> 
> Also this patch adds comment to asm-generic/random.h about stack
> protector dependency on it.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> 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      | 47 +++++++++++++++++++++++++++++++
>  xen/include/asm-generic/random.h  |  5 ++++
>  xen/include/xen/stack-protector.h | 30 ++++++++++++++++++++
>  6 files changed, 102 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

Personally I'd prefer if we consistently used the list approach we use
in various places, whenever possible:

CFLAGS-stack-protector-y := -fno-stack-protector
CFLAGS-stack-protector-$(CONFIG_STACK_PROTECTOR) := -fstack-protector
CFLAGS += $(CFLAGS-stack-protector-y)

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -86,6 +86,9 @@ config HAS_UBSAN
>  config HAS_VMAP
>  	bool
>  
> +config HAS_STACK_PROTECTOR
> +	bool

Please obey to alphabetic sorting in this region of the file.

> @@ -213,6 +216,18 @@ config SPECULATIVE_HARDEN_LOCK
>  
>  endmenu
>  
> +menu "Compiler options"
> +
> +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 exit.
> +
> +endmenu

"Compiler options" reads a little odd to me as a menu title. The preceding one
is "Speculative hardening"; how about making this one "Other hardening" or some
such?

> --- /dev/null
> +++ b/xen/common/stack-protector.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Nit: I don't think we permit C++ comments as per our style.

> +#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 ASM only */

And with no (stack-protector enabled) C functions up the call stack. This
may be as easy to express in the comment as by simply adding "early".
However, considering the so far hypothetical case of offering the feature
also on x86: What about xen.efi, which from the very start uses C code?

> +void __init asmlinkage boot_stack_chk_guard_setup_early(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;

Nit: Indentation (no hard tabs please).

> +    __stack_chk_guard = cycles * a + c;
> +}
> +
> +void asmlinkage __stack_chk_fail(void)
> +{
> +    panic("Stack Protector integrity violation identified in %ps\n",
> +	  __builtin_return_address(0));

Again.

Is panic() really the right construct to use here, though?
__builtin_return_address() will merely identify the immediate caller. A
full stack trace (from BUG()) would likely be more useful in identifying
the offender.

> --- a/xen/include/asm-generic/random.h
> +++ b/xen/include/asm-generic/random.h
> @@ -2,6 +2,11 @@
>  #ifndef __ASM_GENERIC_RANDOM_H__
>  #define __ASM_GENERIC_RANDOM_H__
>  
> +/*
> + * When implementing arch_get_random(), please make sure that
> + * it can provide random data before stack protector is initialized
> + * (i.e. before boot_stack_chk_guard_setup() is called).
> + */
>  static inline unsigned int arch_get_random(void)
>  {
>      return 0;

What exactly will go (entirely) wrong if the comment isn't followed?
(I'm afraid anyway that the comment living here is easy to miss.)

> --- /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_STACK_PROTECTOR
> +
> +#include <xen/random.h>
> +
> +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 has
> + * stack-protector disabled.
> + */

As to the latter - that's not just "a function" but an entire call
stack that would need to have stack protector disabled.

> +static always_inline void boot_stack_chk_guard_setup(void)
> +{
> +    __stack_chk_guard = get_random();
> +    if (BITS_PER_LONG == 64)

Nit: Style (missing blanks).

> +        __stack_chk_guard |= ((unsigned long)get_random()) << 32;

Nit: No real need for the outer pair of parentheses.

Jan

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

[Volodymyr Babchuk]

Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 11.12.2024 03:04, Volodymyr Babchuk wrote:

[...]

>
>> @@ -213,6 +216,18 @@ config SPECULATIVE_HARDEN_LOCK
>>  
>>  endmenu
>>  
>> +menu "Compiler options"
>> +
>> +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 exit.
>> +
>> +endmenu
>
> "Compiler options" reads a little odd to me as a menu title. The preceding one
> is "Speculative hardening"; how about making this one "Other hardening" or some
> such?

It was on of the Andrew's suggestion. Other was "Hardening". So yes, I
can rename it to "Other hardening", hope Andrew will be okay with this.

[...]

>> +/* This function should be called from ASM only */
>
> And with no (stack-protector enabled) C functions up the call stack. This
> may be as easy to express in the comment as by simply adding "early".

Like "This function should be called from early ASM only" ?

> However, considering the so far hypothetical case of offering the feature
> also on x86: What about xen.efi, which from the very start uses C code?
>

It depends on what other services are available. If RNG can be used
already, we don't need to call this function at all and can use
boot_stack_chk_guard_setup() right away.

>> +void __init asmlinkage boot_stack_chk_guard_setup_early(void)
>> +{

[...]

>> +void asmlinkage __stack_chk_fail(void)
>> +{
>> +    panic("Stack Protector integrity violation identified in %ps\n",
>> +	  __builtin_return_address(0));
>
> Again.
>
> Is panic() really the right construct to use here, though?
> __builtin_return_address() will merely identify the immediate caller. A
> full stack trace (from BUG()) would likely be more useful in identifying
> the offender.

Okay, I'll put just plain BUG(); here.

>
>> --- a/xen/include/asm-generic/random.h
>> +++ b/xen/include/asm-generic/random.h
>> @@ -2,6 +2,11 @@
>>  #ifndef __ASM_GENERIC_RANDOM_H__
>>  #define __ASM_GENERIC_RANDOM_H__
>>  
>> +/*
>> + * When implementing arch_get_random(), please make sure that
>> + * it can provide random data before stack protector is initialized
>> + * (i.e. before boot_stack_chk_guard_setup() is called).
>> + */
>>  static inline unsigned int arch_get_random(void)
>>  {
>>      return 0;
>
> What exactly will go (entirely) wrong if the comment isn't followed?

This will not cause immediate harm, but it will give false confidence to
anyone who enables stack protector.

I'd prefer more substantial protection, but we can't even check if
random generator is available in runtime. Taking into account that we
potential can get 0 as result of RNG, we can't even put
WARN_ON(!arch_get_random()) check.

> (I'm afraid anyway that the comment living here is easy to miss.)

I didn't found a better place for it. Maybe you can suggest one?


[...]

Thank you for the review. I'll address all your other comments.

-- 
WBR, Volodymyr

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

[Jan Beulich]

On 12.12.2024 01:47, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>> --- a/xen/include/asm-generic/random.h
>>> +++ b/xen/include/asm-generic/random.h
>>> @@ -2,6 +2,11 @@
>>>  #ifndef __ASM_GENERIC_RANDOM_H__
>>>  #define __ASM_GENERIC_RANDOM_H__
>>>  
>>> +/*
>>> + * When implementing arch_get_random(), please make sure that
>>> + * it can provide random data before stack protector is initialized
>>> + * (i.e. before boot_stack_chk_guard_setup() is called).
>>> + */
>>>  static inline unsigned int arch_get_random(void)
>>>  {
>>>      return 0;
>>
>> What exactly will go (entirely) wrong if the comment isn't followed?
> 
> This will not cause immediate harm, but it will give false confidence to
> anyone who enables stack protector.
> 
> I'd prefer more substantial protection, but we can't even check if
> random generator is available in runtime. Taking into account that we
> potential can get 0 as result of RNG, we can't even put
> WARN_ON(!arch_get_random()) check.

Right, and hence 0 isn't strictly something that can be called "bad".
With at least some randomness one will of course observe a possible
problem at least across two or more runs. However, you don't call
arch_get_random() directly anyway, and get_random() has fallback code,
which is no more likely to return 0 than arch_get_random() is.

In fact this fallback code means get_random() will also use it when
arch_get_random() returns 0 as coming from an actual RNG. Which can
be considered bogus, as for a good random number source _every_
possible value ought to have equal probability of being returned.
Plus if we special-case 0, why not also special-case ~0? Or any other
number with only very few bits set/clear?

For the purpose of stack protector we may want to consider using a
mix of static pattern (not used elsewhere in the codebase) and random
number. The static pattern part would then want arranging such that
at least any value representing a valid address (within Xen alone)
won't match possible canary values.

>> (I'm afraid anyway that the comment living here is easy to miss.)
> 
> I didn't found a better place for it. Maybe you can suggest one?

I'm of two minds here really. Part of me wants this simply dropped. Yet
then some may deem this worthwhile information, except that then it
needs to live is a suitably exposed place. Just that I can't think of
any that would really fit.

Jan

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

[Andrew Cooper]

On 11/12/2024 8:16 am, Jan Beulich wrote:
> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>> Both GCC and Clang support -fstack-protector feature, which add stack
>> canaries to functions where stack corruption is possible. This patch
>> makes general preparations to enable this feature on different
>> supported architectures:
>>
>>  - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture
>>    can enable this feature individually
>>  - Added user-selectable CONFIG_STACK_PROTECTOR option
>>  - Implemented code that sets up random stack canary and a basic
>>    handler for stack protector failures
>>
>> Stack guard value is initialized in three phases:
>>
>> 1. Pre-defined randomly-selected value.
>>
>> 2. Early use of 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. Even when
>> get_cycles() is available, it's return value may be easily predicted,
>> especially on embedded systems, where boot time is quite consistent.
>>
>> 3. After hypervisor is sufficiently initialized, stack guard can be
>> set-up with get_random() function, which is expected to provide better
>> randomness.
>>
>> Also this patch adds comment to asm-generic/random.h about stack
>> protector dependency on it.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>> ---
>>
>> 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      | 47 +++++++++++++++++++++++++++++++
>>  xen/include/asm-generic/random.h  |  5 ++++
>>  xen/include/xen/stack-protector.h | 30 ++++++++++++++++++++
>>  6 files changed, 102 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
> Personally I'd prefer if we consistently used the list approach we use
> in various places, whenever possible:
>
> CFLAGS-stack-protector-y := -fno-stack-protector
> CFLAGS-stack-protector-$(CONFIG_STACK_PROTECTOR) := -fstack-protector
> CFLAGS += $(CFLAGS-stack-protector-y)

No - please stop this antipattern.

It saves 2 lines of code and makes the logic complete unintelligible.

I have a very strong preference for this patch to happen as Volodymyr
presented, and without the double := replacing the more-legible ifeq.

>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -86,6 +86,9 @@ config HAS_UBSAN
>>  config HAS_VMAP
>>  	bool
>>  
>> +config HAS_STACK_PROTECTOR
>> +	bool
> Please obey to alphabetic sorting in this region of the file.
>
>> @@ -213,6 +216,18 @@ config SPECULATIVE_HARDEN_LOCK
>>  
>>  endmenu
>>  
>> +menu "Compiler options"
>> +
>> +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 exit.

I'd be tempted to say "on function exit" to be a little more specific.

>> +
>> +endmenu
> "Compiler options" reads a little odd to me as a menu title. The preceding one
> is "Speculative hardening"; how about making this one "Other hardening" or some
> such?

In an ideal world, we'd have "Code hardening", with speculative just
being one item in the list beside stack protector, trivial auto var
init, FineIBT, etc.

>
>> --- /dev/null
>> +++ b/xen/common/stack-protector.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
> Nit: I don't think we permit C++ comments as per our style.
>
>> +#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 ASM only */
> And with no (stack-protector enabled) C functions up the call stack. This
> may be as easy to express in the comment as by simply adding "early".
> However, considering the so far hypothetical case of offering the feature
> also on x86: What about xen.efi, which from the very start uses C code?

The necessary property is "with no functions to unwind that have an
active canary".

This is why it ends up as "from early assembly, or a noreturn function",
where really the latter is even really "has an exit which escapes canary
tracking, such as Xen's reset_stack_and_jmp()".

>
>> +void __init asmlinkage boot_stack_chk_guard_setup_early(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;
> Nit: Indentation (no hard tabs please).
>
>> +    __stack_chk_guard = cycles * a + c;
>> +}

This is much much nicer.

That said, I don't think we two different setup phases.  I'd suggest
having only this get_cycles implementation and ignore the later
get_random() setup.

It's definitely good enough for a v1, (and frankly, get_random() wants
some separate improvements anyway.)


>> +
>> +void asmlinkage __stack_chk_fail(void)
>> +{
>> +    panic("Stack Protector integrity violation identified in %ps\n",
>> +	  __builtin_return_address(0));
> Again.
>
> Is panic() really the right construct to use here, though?
> __builtin_return_address() will merely identify the immediate caller. A
> full stack trace (from BUG()) would likely be more useful in identifying
> the offender.

Well - we have to be careful, because the backtrace from here is
specifically misleading in this case.

When this trips, it's either the caller itself that broke, or some
sibling call tree which is rubble under the active stack now.

BUG() also comes with 0 information.

So, maybe we want a dump_execution_state() (to get the backtrace), and
then this panic() which states it was a Stack Protection violation,
which hopefully is enough of a hint to people to look in the sibling
call tree.

~Andrew

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

[Jan Beulich]

On 12.12.2024 01:52, Andrew Cooper wrote:
> On 11/12/2024 8:16 am, Jan Beulich wrote:
>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>> --- 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
>> Personally I'd prefer if we consistently used the list approach we use
>> in various places, whenever possible:
>>
>> CFLAGS-stack-protector-y := -fno-stack-protector
>> CFLAGS-stack-protector-$(CONFIG_STACK_PROTECTOR) := -fstack-protector
>> CFLAGS += $(CFLAGS-stack-protector-y)
> 
> No - please stop this antipattern.
> 
> It saves 2 lines of code and makes the logic complete unintelligible.
> 
> I have a very strong preference for this patch to happen as Volodymyr
> presented, and without the double := replacing the more-legible ifeq.

Why "antipattern"? Surely there are cases where the list approach is
preferable. Surely there are cases where it ends up less legible, and
this may indeed be one such case. Yet then - where do you suggest to
draw the boundary?

>>> +void asmlinkage __stack_chk_fail(void)
>>> +{
>>> +    panic("Stack Protector integrity violation identified in %ps\n",
>>> +	  __builtin_return_address(0));
>> Again.
>>
>> Is panic() really the right construct to use here, though?
>> __builtin_return_address() will merely identify the immediate caller. A
>> full stack trace (from BUG()) would likely be more useful in identifying
>> the offender.
> 
> Well - we have to be careful, because the backtrace from here is
> specifically misleading in this case.
> 
> When this trips, it's either the caller itself that broke, or some
> sibling call tree which is rubble under the active stack now.
> 
> BUG() also comes with 0 information.

Not quite, the more that you suggest an alternative way to ...

> So, maybe we want a dump_execution_state() (to get the backtrace), and
> then this panic() which states it was a Stack Protection violation,
> which hopefully is enough of a hint to people to look in the sibling
> call tree.

... get register state and stack trace out. Which I'd be entirely fine
with.

Jan

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

[Yann Dirson]

On 12/11/24 03:04, Volodymyr Babchuk wrote:
> +menu "Compiler options"
> +
> +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 exit.
> +
> +endmenu
> +

Since the previous patch lets room for other components to add Stack 
Protector support, wouldn't it be useful to make it more obvious what 
the scope of this option is?



Yann Dirson | Vates Platform Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

---

Changes in v3:
 - Reword commit message
 - Use CFLAGS += instead of cc-optios-add
Changes in v2:
 - New in v2
---
 Config.mk                            | 2 +-
 stubdom/Makefile                     | 3 +++
 tools/firmware/Rules.mk              | 2 ++
 tools/tests/x86_emulator/testcase.mk | 2 +-
 xen/Makefile                         | 2 ++
 5 files changed, 9 insertions(+), 2 deletions(-)

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..4c9186499d 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)
@@ -54,6 +56,7 @@ TARGET_CFLAGS += $(CFLAGS)
 TARGET_CPPFLAGS += $(CPPFLAGS)
 $(call cc-options-add,TARGET_CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
+
 # 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..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 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 v3 1/3] common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS

[Jan Beulich]

On 11.12.2024 03:04, Volodymyr Babchuk wrote:
> This patch is preparation for making stack protector
> configurable. First step is to remove -fno-stack-protector flag from
> EMBEDDED_EXTRA_CFLAGS so separate components (Hypervisor in this case)
> can enable/disable this feature by themselves.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with ...

> --- 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)
> @@ -54,6 +56,7 @@ TARGET_CFLAGS += $(CFLAGS)
>  TARGET_CPPFLAGS += $(CPPFLAGS)
>  $(call cc-options-add,TARGET_CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  
> +
>  # 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__

... this stray (and wrong) hunk dropped. Can likely be done while committing.

Jan

Re: [PATCH v3 1/3] common: remove -fno-stack-protector from EMBEDDED_EXTRA_CFLAGS

[Andrew Cooper]

On 11/12/2024 2:04 am, Volodymyr Babchuk wrote:
> This patch is preparation for making stack protector
> configurable. First step is to remove -fno-stack-protector flag from
> EMBEDDED_EXTRA_CFLAGS so separate components (Hypervisor in this case)
> can enable/disable this feature by themselves.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

This doesn't build on x86.

You need this hunk too,

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index d45787665907..ff0d61d7ac39 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__
 
because CFLAGS_x86_32 really was using -fno-stack-protector to override
the compilers inbuilt choice.

~Andrew

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

[Volodymyr Babchuk]

Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
platform. We initialize stack protector in two stages: from head.S
using boot_stack_chk_guard_setup_early() function and from start_xen()
using  boot_stack_chk_guard_setup(). This ensures that all C code from
the very beginning can use stack protector.

We call boot_stack_chk_guard_setup() only after time subsystem was
initialized to make sure that generic random number generator will
be working properly.

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

---

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 +++
 xen/arch/arm/setup.c      | 3 +++
 3 files changed, 7 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/arm64/head.S b/xen/arch/arm/arm64/head.S
index 72c7b24498..535969e9c0 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_early
+#endif
         bl    check_cpu_mode
         bl    cpu_init
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 2e27af4560..3587baab21 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>
@@ -359,6 +360,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
 
     preinit_xen_time();
 
+    boot_stack_chk_guard_setup();
+
     gic_preinit();
 
     uart_init();
-- 
2.47.1

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 11.12.2024 03:04, 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.
> 
> 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.

Why exactly would it not be possible to offer the feature when new enough
gcc is in use?

Jan

Re: [PATCH v3 0/3] Add stack protector

[Volodymyr Babchuk]

Hello Jan,

Jan Beulich <jbeulich@suse.com> writes:

> On 11.12.2024 03:04, 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.
>> 
>> 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.
>
> Why exactly would it not be possible to offer the feature when new enough
> gcc is in use?

It is possible to use this feature with a modern enough GCC, yes. Are
you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
x86 platform?

-- 
WBR, Volodymyr

Re: [PATCH v3 0/3] Add stack protector

[Andrew Cooper]

On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
> Hello Jan,
>
> Jan Beulich <jbeulich@suse.com> writes:
>
>> On 11.12.2024 03:04, 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.
>>>
>>> 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.
>> Why exactly would it not be possible to offer the feature when new enough
>> gcc is in use?
> It is possible to use this feature with a modern enough GCC, yes. Are
> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
> x86 platform?

(With the knowledge that this is a disputed Kconfig pattern, and will
need rebasing), the way I want this to work is simply:

diff --git a/xen/Makefile b/xen/Makefile
index 0de0101fd0bf..5d0a88fb3c3f 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -434,6 +434,9 @@ endif
 
 ifeq ($(CONFIG_STACK_PROTECTOR),y)
 CFLAGS += -fstack-protector
+ifeq ($(CONFIG_X86),y)
+CFLAGS += -mstack-protector-guard=global
+endif
 else
 CFLAGS += -fno-stack-protector
 endif
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9cdd04721afa..7951ca908b36 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -28,6 +28,7 @@ config X86
        select HAS_PCI_MSI
        select HAS_PIRQ
        select HAS_SCHED_GRANULARITY
+       select HAS_STACK_PROTECTOR if
$(cc-option,-mstack-protector-guard=global)
        select HAS_UBSAN
        select HAS_VMAP
        select HAS_VPCI if HVM



Sadly, it doesn't build.  I get a handful of:

prelink.o: in function `cmdline_parse':
/home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
--no-relax
/home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
--no-relax

which is more toolchain-whispering than I feel like doing tonight.

~Andrew

Re: [PATCH v3 0/3] Add stack protector

[Andrew Cooper]

On 12/12/2024 1:17 am, Andrew Cooper wrote:
> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>> Hello Jan,
>>
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> On 11.12.2024 03:04, 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.
>>>>
>>>> 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.
>>> Why exactly would it not be possible to offer the feature when new enough
>>> gcc is in use?
>> It is possible to use this feature with a modern enough GCC, yes. Are
>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>> x86 platform?
> (With the knowledge that this is a disputed Kconfig pattern, and will
> need rebasing), the way I want this to work is simply:
>
> diff --git a/xen/Makefile b/xen/Makefile
> index 0de0101fd0bf..5d0a88fb3c3f 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -434,6 +434,9 @@ endif
>  
>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>  CFLAGS += -fstack-protector
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -mstack-protector-guard=global
> +endif
>  else
>  CFLAGS += -fno-stack-protector
>  endif
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..7951ca908b36 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>         select HAS_PCI_MSI
>         select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
> +       select HAS_STACK_PROTECTOR if
> $(cc-option,-mstack-protector-guard=global)
>         select HAS_UBSAN
>         select HAS_VMAP
>         select HAS_VPCI if HVM
>
>
>
> Sadly, it doesn't build.  I get a handful of:
>
> prelink.o: in function `cmdline_parse':
> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
>
> which is more toolchain-whispering than I feel like doing tonight.

P.S.  Irrespective of the x86 side of things, you need a final patch on
your series adjusting CHANGELOG.md.

~Andrew

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 12.12.2024 02:17, Andrew Cooper wrote:
> (With the knowledge that this is a disputed Kconfig pattern, and will
> need rebasing), the way I want this to work is simply:
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 0de0101fd0bf..5d0a88fb3c3f 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -434,6 +434,9 @@ endif
>  
>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>  CFLAGS += -fstack-protector
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -mstack-protector-guard=global
> +endif
>  else
>  CFLAGS += -fno-stack-protector
>  endif
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..7951ca908b36 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>         select HAS_PCI_MSI
>         select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
> +       select HAS_STACK_PROTECTOR if
> $(cc-option,-mstack-protector-guard=global)
>         select HAS_UBSAN
>         select HAS_VMAP
>         select HAS_VPCI if HVM
> 
> 
> 
> Sadly, it doesn't build.  I get a handful of:
> 
> prelink.o: in function `cmdline_parse':
> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> 
> which is more toolchain-whispering than I feel like doing tonight.

Imo the root of the problem is that the compiler doesn't itself mark
__stack_chk_guard hidden (it does so for __stack_chk_fail, albeit only for
32-bit code), and hence finds it necessary to use @gotpcrel to access the
variable. Even if the linker managed to relax all of these, it would then
still be less efficient compared to direct RIP-relative accesses.

I also can't see how we might be able to override the compiler's internal
declaration to mark it hidden (the same appears to be true for other items
the declares internally, like the retpoline thunks or even strcmp() et al).
Passing -fvisibility=hidden doesn't make a difference (just as another
data point).

Playing with -fstack-protector* flavors, I observe:
- -fstack-protector causing several failures, like you observed, oddly
  enough exclusively from __init functions,
- -fstack-protector-all and -fstack-protector-strong each causing a single
  (but respectively different) failure, for apparently random non-__init
  functions.
Taking this together it very much smells like a linker issue. I'll see
about checking there further.

Jan

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 12.12.2024 15:30, Jan Beulich wrote:
> On 12.12.2024 02:17, Andrew Cooper wrote:
>> (With the knowledge that this is a disputed Kconfig pattern, and will
>> need rebasing), the way I want this to work is simply:
>>
>> diff --git a/xen/Makefile b/xen/Makefile
>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -434,6 +434,9 @@ endif
>>  
>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>  CFLAGS += -fstack-protector
>> +ifeq ($(CONFIG_X86),y)
>> +CFLAGS += -mstack-protector-guard=global
>> +endif
>>  else
>>  CFLAGS += -fno-stack-protector
>>  endif
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 9cdd04721afa..7951ca908b36 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -28,6 +28,7 @@ config X86
>>         select HAS_PCI_MSI
>>         select HAS_PIRQ
>>         select HAS_SCHED_GRANULARITY
>> +       select HAS_STACK_PROTECTOR if
>> $(cc-option,-mstack-protector-guard=global)
>>         select HAS_UBSAN
>>         select HAS_VMAP
>>         select HAS_VPCI if HVM
>>
>>
>>
>> Sadly, it doesn't build.  I get a handful of:
>>
>> prelink.o: in function `cmdline_parse':
>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>> --no-relax
>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>> --no-relax
>>
>> which is more toolchain-whispering than I feel like doing tonight.
> 
> Imo the root of the problem is that the compiler doesn't itself mark
> __stack_chk_guard hidden (it does so for __stack_chk_fail, albeit only for
> 32-bit code), and hence finds it necessary to use @gotpcrel to access the
> variable. Even if the linker managed to relax all of these, it would then
> still be less efficient compared to direct RIP-relative accesses.
> 
> I also can't see how we might be able to override the compiler's internal
> declaration to mark it hidden (the same appears to be true for other items
> the declares internally, like the retpoline thunks or even strcmp() et al).
> Passing -fvisibility=hidden doesn't make a difference (just as another
> data point).
> 
> Playing with -fstack-protector* flavors, I observe:
> - -fstack-protector causing several failures, like you observed, oddly
>   enough exclusively from __init functions,
> - -fstack-protector-all and -fstack-protector-strong each causing a single
>   (but respectively different) failure, for apparently random non-__init
>   functions.
> Taking this together it very much smells like a linker issue. I'll see
> about checking there further.

The oddity with how many diags show up is down to internals of the linker.
It processes a single input section in full (continuing over this specific
type of error), but will stop processing afterwards if any such error was
encountered.

The issue itself is a wrong assumption in the linker: It believes that it
would only ever build small-model code when encountering this kind of
relocation, and when not linking a shared library or PIE. With this
assumption it converts the relocation resulting from @gotpcrel to
R_X86_64_32S (converting the MOV from GOT to MOV $imm), which of course
overflows when later trying to actually resolve it. What I'm yet to
understand is why it doesn't use R_X86_64_PC32 (also) in such a situation
(it does e.g. when building a shared library).

While so far I didn't try it, using --no-relax is presumably not an option,
as I expect that it'll leave us with a non-empty .got. Plus I didn't even
start looking into how the xen.efi linking would deal with the ELF-specific
gotpcrel relocs; the concept of GOT doesn't exist in PE/COFF, after all.

While the linker certainly wants fixing, I continue to think that getting
the compiler side right would yield the better overall result.

Jan

Re: [PATCH v3 0/3] Add stack protector

[Andrew Cooper]

On 12/12/2024 4:52 pm, Jan Beulich wrote:
> On 12.12.2024 15:30, Jan Beulich wrote:
>> On 12.12.2024 02:17, Andrew Cooper wrote:
>>> (With the knowledge that this is a disputed Kconfig pattern, and will
>>> need rebasing), the way I want this to work is simply:
>>>
>>> diff --git a/xen/Makefile b/xen/Makefile
>>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -434,6 +434,9 @@ endif
>>>  
>>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>>  CFLAGS += -fstack-protector
>>> +ifeq ($(CONFIG_X86),y)
>>> +CFLAGS += -mstack-protector-guard=global
>>> +endif
>>>  else
>>>  CFLAGS += -fno-stack-protector
>>>  endif
>>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>> index 9cdd04721afa..7951ca908b36 100644
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -28,6 +28,7 @@ config X86
>>>         select HAS_PCI_MSI
>>>         select HAS_PIRQ
>>>         select HAS_SCHED_GRANULARITY
>>> +       select HAS_STACK_PROTECTOR if
>>> $(cc-option,-mstack-protector-guard=global)
>>>         select HAS_UBSAN
>>>         select HAS_VMAP
>>>         select HAS_VPCI if HVM
>>>
>>>
>>>
>>> Sadly, it doesn't build.  I get a handful of:
>>>
>>> prelink.o: in function `cmdline_parse':
>>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>> --no-relax
>>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>> --no-relax
>>>
>>> which is more toolchain-whispering than I feel like doing tonight.
>> Imo the root of the problem is that the compiler doesn't itself mark
>> __stack_chk_guard hidden (it does so for __stack_chk_fail, albeit only for
>> 32-bit code), and hence finds it necessary to use @gotpcrel to access the
>> variable. Even if the linker managed to relax all of these, it would then
>> still be less efficient compared to direct RIP-relative accesses.
>>
>> I also can't see how we might be able to override the compiler's internal
>> declaration to mark it hidden (the same appears to be true for other items
>> the declares internally, like the retpoline thunks or even strcmp() et al).
>> Passing -fvisibility=hidden doesn't make a difference (just as another
>> data point).
>>
>> Playing with -fstack-protector* flavors, I observe:
>> - -fstack-protector causing several failures, like you observed, oddly
>>   enough exclusively from __init functions,
>> - -fstack-protector-all and -fstack-protector-strong each causing a single
>>   (but respectively different) failure, for apparently random non-__init
>>   functions.
>> Taking this together it very much smells like a linker issue. I'll see
>> about checking there further.
> The oddity with how many diags show up is down to internals of the linker.
> It processes a single input section in full (continuing over this specific
> type of error), but will stop processing afterwards if any such error was
> encountered.
>
> The issue itself is a wrong assumption in the linker: It believes that it
> would only ever build small-model code when encountering this kind of
> relocation, and when not linking a shared library or PIE. With this
> assumption it converts the relocation resulting from @gotpcrel to
> R_X86_64_32S (converting the MOV from GOT to MOV $imm), which of course
> overflows when later trying to actually resolve it. What I'm yet to
> understand is why it doesn't use R_X86_64_PC32 (also) in such a situation
> (it does e.g. when building a shared library).
>
> While so far I didn't try it, using --no-relax is presumably not an option,
> as I expect that it'll leave us with a non-empty .got. Plus I didn't even
> start looking into how the xen.efi linking would deal with the ELF-specific
> gotpcrel relocs; the concept of GOT doesn't exist in PE/COFF, after all.
>
> While the linker certainly wants fixing, I continue to think that getting
> the compiler side right would yield the better overall result.

Ok, so what precisely needs doing here?

For starters, I guess __stack_chk_guard wants to respect
-fvisibilty=hidden and/or #pragma.  I can see why it wouldn't want to in
regular userspace, but we're not that.

There's clearly also an LD error (bad assumptions about model).

How many other bugs need opening in the various bugzillas?

~Andrew

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 19.12.2024 01:20, Andrew Cooper wrote:
> On 12/12/2024 4:52 pm, Jan Beulich wrote:
>> On 12.12.2024 15:30, Jan Beulich wrote:
>>> On 12.12.2024 02:17, Andrew Cooper wrote:
>>>> (With the knowledge that this is a disputed Kconfig pattern, and will
>>>> need rebasing), the way I want this to work is simply:
>>>>
>>>> diff --git a/xen/Makefile b/xen/Makefile
>>>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>>>> --- a/xen/Makefile
>>>> +++ b/xen/Makefile
>>>> @@ -434,6 +434,9 @@ endif
>>>>  
>>>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>>>  CFLAGS += -fstack-protector
>>>> +ifeq ($(CONFIG_X86),y)
>>>> +CFLAGS += -mstack-protector-guard=global
>>>> +endif
>>>>  else
>>>>  CFLAGS += -fno-stack-protector
>>>>  endif
>>>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>>> index 9cdd04721afa..7951ca908b36 100644
>>>> --- a/xen/arch/x86/Kconfig
>>>> +++ b/xen/arch/x86/Kconfig
>>>> @@ -28,6 +28,7 @@ config X86
>>>>         select HAS_PCI_MSI
>>>>         select HAS_PIRQ
>>>>         select HAS_SCHED_GRANULARITY
>>>> +       select HAS_STACK_PROTECTOR if
>>>> $(cc-option,-mstack-protector-guard=global)
>>>>         select HAS_UBSAN
>>>>         select HAS_VMAP
>>>>         select HAS_VPCI if HVM
>>>>
>>>>
>>>>
>>>> Sadly, it doesn't build.  I get a handful of:
>>>>
>>>> prelink.o: in function `cmdline_parse':
>>>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>>> --no-relax
>>>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>>> --no-relax
>>>>
>>>> which is more toolchain-whispering than I feel like doing tonight.
>>> Imo the root of the problem is that the compiler doesn't itself mark
>>> __stack_chk_guard hidden (it does so for __stack_chk_fail, albeit only for
>>> 32-bit code), and hence finds it necessary to use @gotpcrel to access the
>>> variable. Even if the linker managed to relax all of these, it would then
>>> still be less efficient compared to direct RIP-relative accesses.
>>>
>>> I also can't see how we might be able to override the compiler's internal
>>> declaration to mark it hidden (the same appears to be true for other items
>>> the declares internally, like the retpoline thunks or even strcmp() et al).
>>> Passing -fvisibility=hidden doesn't make a difference (just as another
>>> data point).
>>>
>>> Playing with -fstack-protector* flavors, I observe:
>>> - -fstack-protector causing several failures, like you observed, oddly
>>>   enough exclusively from __init functions,
>>> - -fstack-protector-all and -fstack-protector-strong each causing a single
>>>   (but respectively different) failure, for apparently random non-__init
>>>   functions.
>>> Taking this together it very much smells like a linker issue. I'll see
>>> about checking there further.
>> The oddity with how many diags show up is down to internals of the linker.
>> It processes a single input section in full (continuing over this specific
>> type of error), but will stop processing afterwards if any such error was
>> encountered.
>>
>> The issue itself is a wrong assumption in the linker: It believes that it
>> would only ever build small-model code when encountering this kind of
>> relocation, and when not linking a shared library or PIE. With this
>> assumption it converts the relocation resulting from @gotpcrel to
>> R_X86_64_32S (converting the MOV from GOT to MOV $imm), which of course
>> overflows when later trying to actually resolve it. What I'm yet to
>> understand is why it doesn't use R_X86_64_PC32 (also) in such a situation
>> (it does e.g. when building a shared library).
>>
>> While so far I didn't try it, using --no-relax is presumably not an option,
>> as I expect that it'll leave us with a non-empty .got. Plus I didn't even
>> start looking into how the xen.efi linking would deal with the ELF-specific
>> gotpcrel relocs; the concept of GOT doesn't exist in PE/COFF, after all.
>>
>> While the linker certainly wants fixing, I continue to think that getting
>> the compiler side right would yield the better overall result.
> 
> Ok, so what precisely needs doing here?
> 
> For starters, I guess __stack_chk_guard wants to respect
> -fvisibilty=hidden and/or #pragma.  I can see why it wouldn't want to in
> regular userspace, but we're not that.

Yes, this is one of the things that may want reporting as a deficiency. Imo
it wants generalizing though, as it's not __stack_chk_guard alone which is
affected.

I'm not, btw, convinced the #pragma ought to have any effect. One might
consider it legitimate to have an effect if there's a subsequent re-
declaration. Yet one might also consider such an (incompatible) re-
declaration be an error.

> There's clearly also an LD error (bad assumptions about model).

I'm still in the process of collecting data for an eventual email or bug
report to be put together. For the purposes here I meanwhile think it is
largely irrelevant. That's because, as said, even if the linker was fixed
in this regard, there would still be the fact that we'd end up with a non-
empty .got (I did hack the linker enough to verify this). Whereas once the
compiler side was sorted, the linker issue wouldn't come into play anymore.

Jan

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 12.12.2024 02:17, Andrew Cooper wrote:
> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>> Hello Jan,
>>
>> Jan Beulich <jbeulich@suse.com> writes:
>>
>>> On 11.12.2024 03:04, 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.
>>>>
>>>> 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.
>>> Why exactly would it not be possible to offer the feature when new enough
>>> gcc is in use?
>> It is possible to use this feature with a modern enough GCC, yes. Are
>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>> x86 platform?
> 
> (With the knowledge that this is a disputed Kconfig pattern, and will
> need rebasing), the way I want this to work is simply:
> 
> diff --git a/xen/Makefile b/xen/Makefile
> index 0de0101fd0bf..5d0a88fb3c3f 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -434,6 +434,9 @@ endif
>  
>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>  CFLAGS += -fstack-protector
> +ifeq ($(CONFIG_X86),y)
> +CFLAGS += -mstack-protector-guard=global
> +endif
>  else
>  CFLAGS += -fno-stack-protector
>  endif
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..7951ca908b36 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>         select HAS_PCI_MSI
>         select HAS_PIRQ
>         select HAS_SCHED_GRANULARITY
> +       select HAS_STACK_PROTECTOR if
> $(cc-option,-mstack-protector-guard=global)
>         select HAS_UBSAN
>         select HAS_VMAP
>         select HAS_VPCI if HVM
> 
> 
> 
> Sadly, it doesn't build.  I get a handful of:
> 
> prelink.o: in function `cmdline_parse':
> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
> --no-relax
> 
> which is more toolchain-whispering than I feel like doing tonight.

For reference:
https://sourceware.org/pipermail/binutils/2025-January/138631.html

You didn't enter a gcc bug report yet, did you?

Jan

Re: [PATCH v3 0/3] Add stack protector

[Andrew Cooper]

On 14/01/2025 1:22 pm, Jan Beulich wrote:
> On 12.12.2024 02:17, Andrew Cooper wrote:
>> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>>> Hello Jan,
>>>
>>> Jan Beulich <jbeulich@suse.com> writes:
>>>
>>>> On 11.12.2024 03:04, 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.
>>>>>
>>>>> 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.
>>>> Why exactly would it not be possible to offer the feature when new enough
>>>> gcc is in use?
>>> It is possible to use this feature with a modern enough GCC, yes. Are
>>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>>> x86 platform?
>> (With the knowledge that this is a disputed Kconfig pattern, and will
>> need rebasing), the way I want this to work is simply:
>>
>> diff --git a/xen/Makefile b/xen/Makefile
>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -434,6 +434,9 @@ endif
>>  
>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>  CFLAGS += -fstack-protector
>> +ifeq ($(CONFIG_X86),y)
>> +CFLAGS += -mstack-protector-guard=global
>> +endif
>>  else
>>  CFLAGS += -fno-stack-protector
>>  endif
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 9cdd04721afa..7951ca908b36 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -28,6 +28,7 @@ config X86
>>         select HAS_PCI_MSI
>>         select HAS_PIRQ
>>         select HAS_SCHED_GRANULARITY
>> +       select HAS_STACK_PROTECTOR if
>> $(cc-option,-mstack-protector-guard=global)
>>         select HAS_UBSAN
>>         select HAS_VMAP
>>         select HAS_VPCI if HVM
>>
>>
>>
>> Sadly, it doesn't build.  I get a handful of:
>>
>> prelink.o: in function `cmdline_parse':
>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>> --no-relax
>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>> --no-relax
>>
>> which is more toolchain-whispering than I feel like doing tonight.
> For reference:
> https://sourceware.org/pipermail/binutils/2025-January/138631.html
>
> You didn't enter a gcc bug report yet, did you?

No, not yet.  I'm afraid I've not had the time.

~Andrew

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 14.01.2025 14:28, Andrew Cooper wrote:
> On 14/01/2025 1:22 pm, Jan Beulich wrote:
>> On 12.12.2024 02:17, Andrew Cooper wrote:
>>> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>>>> Hello Jan,
>>>>
>>>> Jan Beulich <jbeulich@suse.com> writes:
>>>>
>>>>> On 11.12.2024 03:04, 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.
>>>>>>
>>>>>> 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.
>>>>> Why exactly would it not be possible to offer the feature when new enough
>>>>> gcc is in use?
>>>> It is possible to use this feature with a modern enough GCC, yes. Are
>>>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>>>> x86 platform?
>>> (With the knowledge that this is a disputed Kconfig pattern, and will
>>> need rebasing), the way I want this to work is simply:
>>>
>>> diff --git a/xen/Makefile b/xen/Makefile
>>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>>> --- a/xen/Makefile
>>> +++ b/xen/Makefile
>>> @@ -434,6 +434,9 @@ endif
>>>  
>>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>>  CFLAGS += -fstack-protector
>>> +ifeq ($(CONFIG_X86),y)
>>> +CFLAGS += -mstack-protector-guard=global
>>> +endif
>>>  else
>>>  CFLAGS += -fno-stack-protector
>>>  endif
>>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>> index 9cdd04721afa..7951ca908b36 100644
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -28,6 +28,7 @@ config X86
>>>         select HAS_PCI_MSI
>>>         select HAS_PIRQ
>>>         select HAS_SCHED_GRANULARITY
>>> +       select HAS_STACK_PROTECTOR if
>>> $(cc-option,-mstack-protector-guard=global)
>>>         select HAS_UBSAN
>>>         select HAS_VMAP
>>>         select HAS_VPCI if HVM
>>>
>>>
>>>
>>> Sadly, it doesn't build.  I get a handful of:
>>>
>>> prelink.o: in function `cmdline_parse':
>>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>> --no-relax
>>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>> --no-relax
>>>
>>> which is more toolchain-whispering than I feel like doing tonight.
>> For reference:
>> https://sourceware.org/pipermail/binutils/2025-January/138631.html
>>
>> You didn't enter a gcc bug report yet, did you?
> 
> No, not yet.  I'm afraid I've not had the time.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118473

Jan

Re: [PATCH v3 0/3] Add stack protector

[Andrew Cooper]

On 14/01/2025 1:47 pm, Jan Beulich wrote:
> On 14.01.2025 14:28, Andrew Cooper wrote:
>> On 14/01/2025 1:22 pm, Jan Beulich wrote:
>>> On 12.12.2024 02:17, Andrew Cooper wrote:
>>>> On 12/12/2024 12:13 am, Volodymyr Babchuk wrote:
>>>>> Hello Jan,
>>>>>
>>>>> Jan Beulich <jbeulich@suse.com> writes:
>>>>>
>>>>>> On 11.12.2024 03:04, 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.
>>>>>>>
>>>>>>> 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.
>>>>>> Why exactly would it not be possible to offer the feature when new enough
>>>>>> gcc is in use?
>>>>> It is possible to use this feature with a modern enough GCC, yes. Are
>>>>> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
>>>>> x86 platform?
>>>> (With the knowledge that this is a disputed Kconfig pattern, and will
>>>> need rebasing), the way I want this to work is simply:
>>>>
>>>> diff --git a/xen/Makefile b/xen/Makefile
>>>> index 0de0101fd0bf..5d0a88fb3c3f 100644
>>>> --- a/xen/Makefile
>>>> +++ b/xen/Makefile
>>>> @@ -434,6 +434,9 @@ endif
>>>>  
>>>>  ifeq ($(CONFIG_STACK_PROTECTOR),y)
>>>>  CFLAGS += -fstack-protector
>>>> +ifeq ($(CONFIG_X86),y)
>>>> +CFLAGS += -mstack-protector-guard=global
>>>> +endif
>>>>  else
>>>>  CFLAGS += -fno-stack-protector
>>>>  endif
>>>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>>> index 9cdd04721afa..7951ca908b36 100644
>>>> --- a/xen/arch/x86/Kconfig
>>>> +++ b/xen/arch/x86/Kconfig
>>>> @@ -28,6 +28,7 @@ config X86
>>>>         select HAS_PCI_MSI
>>>>         select HAS_PIRQ
>>>>         select HAS_SCHED_GRANULARITY
>>>> +       select HAS_STACK_PROTECTOR if
>>>> $(cc-option,-mstack-protector-guard=global)
>>>>         select HAS_UBSAN
>>>>         select HAS_VMAP
>>>>         select HAS_VPCI if HVM
>>>>
>>>>
>>>>
>>>> Sadly, it doesn't build.  I get a handful of:
>>>>
>>>> prelink.o: in function `cmdline_parse':
>>>> /home/andrew/xen.git/xen/common/kernel.c:216:(.init.text+0x20f2): failed
>>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>>> --no-relax
>>>> /home/andrew/xen.git/xen/common/kernel.c:230:(.init.text+0x246f): failed
>>>> to convert GOTPCREL relocation against '__stack_chk_guard'; relink with
>>>> --no-relax
>>>>
>>>> which is more toolchain-whispering than I feel like doing tonight.
>>> For reference:
>>> https://sourceware.org/pipermail/binutils/2025-January/138631.html
>>>
>>> You didn't enter a gcc bug report yet, did you?
>> No, not yet.  I'm afraid I've not had the time.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118473

Thankyou.

~Andrew

Re: [PATCH v3 0/3] Add stack protector

[Jan Beulich]

On 12.12.2024 01:13, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> On 11.12.2024 03:04, 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.
>>>
>>> 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.
>>
>> Why exactly would it not be possible to offer the feature when new enough
>> gcc is in use?
> 
> It is possible to use this feature with a modern enough GCC, yes. Are
> you suggesting to make HAS_STACK_PROTECTOR dependent on GCC_VERSION for
> x86 platform?

Only kind of. I remain yet to be convinced (or formally outvoted) on such
wanting to live in (only) Kconfig. I actually proposed a hybrid model [1].
But yes - some time of build time dependency.

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2022-09/msg01793.html