| Message ID | 20250114042553.1624831-4-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add/enable stack protector | expand |
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@epam.com> > > --- > 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,
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@epam.com> >> >> --- >> 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,
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 select HAS_UBSAN config ARCH_DEFCONFIG diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S 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 bl check_cpu_mode bl cpu_init
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@epam.com> --- 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(+)