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

[Julien Grall <julien@xxxxxxx>]

On 13/02/2025 14:20, Julien Grall wrote:
> 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
Just to expand, this is a problem because Xen may not be loaded with VA 
== PA. So depending on how the GCC generated boot_stack_chk_guard_setup, 
the global variable may be accessed with an absolution address (this 
would be the VA).
This means we could overwrite "random" memory.

>    * We don't follow the AAPCS
>
> If you want to call from assembly, then I think it just needs to be 
> called before launch.
Actually we only setup the stack in launch. So I guess some re-ordering 
is needed to setup the stack earlier.
Cheers,

--
Julien Grall