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

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

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

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

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