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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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