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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 00:40, 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 two phases:
> 
> 1. Pre-defined randomly-selected value.
> 
> 2. Own implementation 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.
> 
> boot_stack_chk_guard_setup() is declared as always_inline to ensure
> that it will not trigger stack protector by itself. And of course,
> caller should ensure that stack protection code will not be reached
> later. It is possible to call the same function from an ASM code by
> introducing simple trampoline in stack-protector.c, but right now
> there is no use case for such trampoline.
> 
> As __stack_chk_fail() is not called by Xen source code directly, and
> only called by compiler-generated code, it does not needed to be
> declared separately. So we need separate MISRA deviation for it.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> 
> ---
> 
> Changes in v8:
>  - Code formatting fixes
>  - Added an explicit MISRA deviation for __stack_chk_fail()
>  - Marked __stack_chk_fail() as noreturn
> 
> Changes in v7:
>  - declared boot_stack_chk_guard_setup as always_inline
>  - moved `#ifdef CONFIG_STACK_PROTECTOR` inside the function
> 
> Changes in v6:
>  - boot_stack_chk_guard_setup() moved to stack-protector.h
>  - Removed Andrew's r-b tag
> 
> Changes in v5:
>  - Fixed indentation
>  - Added stack-protector.h
> ---
>  docs/misra/safe.json              |  8 +++++++
>  xen/Makefile                      |  4 ++++
>  xen/common/Kconfig                | 15 ++++++++++++
>  xen/common/Makefile               |  1 +
>  xen/common/stack-protector.c      | 22 +++++++++++++++++
>  xen/include/xen/stack-protector.h | 39 +++++++++++++++++++++++++++++++
>  6 files changed, 89 insertions(+)
>  create mode 100644 xen/common/stack-protector.c
>  create mode 100644 xen/include/xen/stack-protector.h
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 3d68b59169..e249bcbf81 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -108,6 +108,14 @@
>          },
>          {
>              "id": "SAF-13-safe",
> +            "analyser": {
> +                "eclair": "MC3A2.R8.4"
> +            },
> +            "name": "Rule 8.4: compiler-called function",
> +            "text": "A function, for which compiler generates calls to do 
> not need to have a visible declaration prior to its definition."

Nit: s/ do / does /. Not being a native speaker, I'm still uncertain of the 
need for
the comma in the place you put it - it reads odd this way to me (at least 
without a
2nd comma). I'm anyway inclined to word this differently, to be more precise
(otherwise the deviation here would equally apply to the data item
"__stack_chk_guard"):

"A function, all invocations of which are compiler generated, does not need ..."

With this suitably sorted (can be adjusted on commit to whatever the final 
outcome
is going to be):
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan