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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> ---
> 
> 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