Re: [PATCH v2 2/4] xen: common: add ability to enable stack protector

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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