1. Patchew
  2. Xen
  3. [PATCH v4 0/4] Add/enable stack protector
  4. View patch

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

Volodymyr Babchuk posted 4 patches 1 year ago
There is a newer version of this series
[PATCH v4 3/4] xen: arm: enable stack protector feature
Posted by Volodymyr Babchuk 1 year ago 
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
 	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
 
-- 
2.47.1
Re: [PATCH v4 3/4] xen: arm: enable stack protector feature
Posted by Julien Grall 12 months ago 
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,

-- 
Julien Grall
Re: [PATCH v4 3/4] xen: arm: enable stack protector feature
Posted by Julien Grall 12 months ago 

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,

-- 
Julien Grall

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.