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

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

On 30/11/2024 01:10, Volodymyr Babchuk wrote:
> Enable previously added CONFIG_STACK_PROTECTOR feature for ARM
> platform. Here we can call boot_stack_chk_guard_setup() in start_xen()
> function, because it never returns, so stack protector code will not
> be triggered because of changed canary.
It would be good to explain how you decided to call...

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
>
> ---
>
> In v2:
>   - Reordered Kconfig entry
> ---
>   xen/arch/arm/Kconfig | 1 +
>   xen/arch/arm/setup.c | 3 +++
>   2 files changed, 4 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 23bbc91aad..a24c88c327 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -16,6 +16,7 @@ config ARM
>         select HAS_ALTERNATIVE if HAS_VMAP
>         select HAS_DEVICE_TREE
>         select HAS_PASSTHROUGH
> +       select HAS_STACK_PROTECTOR
>         select HAS_UBSAN
>         select IOMMU_FORCE_PT_SHARE
>   
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2e27af4560..f855e97e25 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -13,6 +13,7 @@
>   #include <xen/domain_page.h>
>   #include <xen/grant_table.h>
>   #include <xen/types.h>
> +#include <xen/stack-protector.h>
>   #include <xen/string.h>
>   #include <xen/serial.h>
>   #include <xen/sched.h>
> @@ -341,6 +342,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>        */
>       system_state = SYS_STATE_boot;
>   
> +    boot_stack_chk_guard_setup();
... the function here. If I am not mistaken, at this point, cpu_khz 
(used by NOW() in get_random()) would be zero. It is only initialized by 
preinit_xen_time() which happens later.
So I think it should be called a bit further down and gain a comment to 
about the call dependencies.
Cheers,

--
Julien Grall