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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/2024 9:07 pm, Volodymyr Babchuk wrote:
> diff --git a/Config.mk b/Config.mk
> index f1eab9a20a..c9fef4659f 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -190,7 +190,7 @@ endif
>  APPEND_LDFLAGS += $(foreach i, $(APPEND_LIB), -L$(i))
>  APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
>  
> -EMBEDDED_EXTRA_CFLAGS := -fno-pie -fno-stack-protector 
> -fno-stack-protector-all
> +EMBEDDED_EXTRA_CFLAGS := -fno-pie
>  EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables
>  
>  XEN_EXTFILES_URL ?= https://xenbits.xen.org/xen-extfiles
> diff --git a/stubdom/Makefile b/stubdom/Makefile
> index 2a81af28a1..41424f6aca 100644
> --- a/stubdom/Makefile
> +++ b/stubdom/Makefile
> @@ -54,6 +54,8 @@ TARGET_CFLAGS += $(CFLAGS)
>  TARGET_CPPFLAGS += $(CPPFLAGS)
>  $(call cc-options-add,TARGET_CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  
> +$(call cc-option-add,TARGET_CFLAGS,CC,-fno-stack-protector)
> +

I'd suggest splitting this into two patches, so removing the flags from
EMBEDDED_EXTRA_CFLAGS is separate from the new infrastructure.

Also, we're losing -fno-stack-protector-all, with no discussion.

AFAICT, it was introduced in c/s f8beb54e245 in 2004, alongside
-fno-stack-protector.  But further investigation shows that it is not,
nor has ever been, a valid option to GCC or Clang.

I've submitted a patch in isolation to drop this.  (And Jan has reviewed
it while I've been writing the rest of the email, so I'll get it
committed in due course).

> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 90268d9249..0ffd825510 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -86,6 +86,9 @@ config HAS_UBSAN
>  config HAS_VMAP
>       bool
>  
> +config HAS_STACK_PROTECTOR
> +     bool
> +
>  config MEM_ACCESS_ALWAYS_ON
>       bool
>  
> @@ -516,4 +519,14 @@ config TRACEBUFFER
>         to be collected at run time for debugging or performance analysis.
>         Memory and execution overhead when not active is minimal.
>  
> +config STACK_PROTECTOR
> +     bool "Stack protection"
> +     depends on HAS_STACK_PROTECTOR
> +     help
> +       Use compiler's option -fstack-protector (supported both by GCC
> +       and clang) to generate code that checks for corrupted stack

Clang

> +       and halts the system in case of any problems.
> +
> +       Please note that this option will impair performance.
> +

I think we probably want a top-level Hardening menu.  Or maybe a
Compiler Options?

There are multiple levels of stack protector, not to mention other
options such as trivial-auto-var-init that we want in due course.


>  endmenu
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index b279b09bfb..a9f2d05476 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -45,6 +45,7 @@ obj-y += shutdown.o
>  obj-y += softirq.o
>  obj-y += smp.o
>  obj-y += spinlock.o
> +obj-$(CONFIG_STACK_PROTECTOR) += stack_protector.o
>  obj-y += stop_machine.o
>  obj-y += symbols.o
>  obj-y += tasklet.o
> diff --git a/xen/common/stack_protector.c b/xen/common/stack_protector.c
> new file mode 100644
> index 0000000000..de7c20f682
> --- /dev/null
> +++ b/xen/common/stack_protector.c

stack-protector.c please.

> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <xen/lib.h>
> +#include <xen/random.h>
> +
> +#ifndef CONFIG_X86
> +/*
> + * GCC uses TLS to store stack canary value on x86.
> + * All other platforms use this global variable.
> + */
> +unsigned long __stack_chk_guard;
> +#endif

Don't bother excluding x86 like this.  Leave that to whomever adds x86
support.

Especially as the global form is the only one we're liable to want to
introduce in the first place.

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

Xen style, not Linux please.


> diff --git a/xen/include/xen/stack_protector.h 
> b/xen/include/xen/stack_protector.h
> new file mode 100644
> index 0000000000..97f1eb5ac0
> --- /dev/null
> +++ b/xen/include/xen/stack_protector.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN__STACK_PROTECTOR_H
> +#define XEN__STACK_PROTECTOR_H
> +
> +#ifdef CONFIG_STACKPROTECTOR
> +
> +#ifndef CONFIG_X86
> +extern unsigned long __stack_chk_guard;
> +#endif
> +
> +/*
> + * This function should be always inlined. Also it should be called
> + * from a function that never returns.
> + */
> +static inline void boot_stack_chk_guard_setup(void)

You must use always_inline if you need it to be always inline.

But, the rest of the comment isn't entirely accurate.  It can also be
from a function with stack-protector disabled.

But the best option is to initialise __stack_chk_guard from asm before
calling into C.

~Andrew