Re: [PATCH v4 3/4] xen: arm: enable stack protector feature

[Julien Grall <julien@xxxxxxx>]

Hi,

On 14/01/2025 04:25, Volodymyr Babchuk wrote:
> Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
> platform. We initialize stack protector very early, in head.S using
> boot_stack_chk_guard_setup. This ensures that all C code from the very
> beginning can use stack protector.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> ---
> In v4:
>   - setup.c does not call boot_stack_chk_guard_setup() anymore, because
>     the original implementation was removed and
>     boot_stack_chk_guard_setup_early was renamed to boot_stack_chk_guard_setup
> In v3:
>   - Call boot_stack_chk_guard_setup_early from head.S to ensure
>     that stack is protected from early boot stages
>   - Call boot_stack_chk_guard_setup() later, when time subsystem is
>     sufficiently initialized to provide values for the random number
>     generator.
> In v2:
>   - Reordered Kconfig entry
> ---
>   xen/arch/arm/Kconfig      | 1 +
>   xen/arch/arm/arm64/head.S | 3 +++
>   2 files changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index a26d3e1182..8f1a3c7d74 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -16,6 +16,7 @@ config ARM
>         select GENERIC_UART_INIT
>         select HAS_ALTERNATIVE if HAS_VMAP
>         select HAS_DEVICE_TREE
> +       select HAS_STACK_PROTECTOR
This is select stack protection for both 32-bit and 64-bit. Yet...

>         select HAS_UBSAN
>   
>   config ARCH_DEFCONFIG
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
... you only modify arm64. Why?

> index 72c7b24498..5cbd62af86 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -250,6 +250,9 @@ real_start_efi:
>   #endif
>           PRINT("- Boot CPU booting -\r\n")
>   
> +#ifdef CONFIG_STACK_PROTECTOR
> +        bl    boot_stack_chk_guard_setup
> +#endif
I don't think you can call a C function this early:
  * The stack is not set  (it is not clear why would the function not 
use it)
  * The MMU is not turned on
  * We don't follow the AAPCS

If you want to call from assembly, then I think it just needs to be 
called before launch.
Cheers,

--
Julien Grall