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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-12-02 at 09:12 +0100, Jan Beulich wrote:
> On 30.11.2024 02:10, Volodymyr Babchuk wrote:
> > Enable previously added CONFIG_STACK_PROTECTOR feature for RISC-V
> > 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.
> > 
> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > Tested-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> Isn't this premature? For ...
> 
> > @@ -57,6 +58,8 @@ void __init noreturn start_xen(unsigned long
> > bootcpu_id,
> >      if ( !boot_fdt_info(device_tree_flattened, dtb_addr) )
> >          BUG();
> >  
> > +    boot_stack_chk_guard_setup();
> 
> ... this function's use of get_random(), either arch_get_random()
> needs
> to be implemented, or (as Julien also pointed out for Arm64) NOW()
> needs
> to work. Yet get_s_time() presently expands to just BUG_ON(). Given
> this
> it's not even clear to me how Oleksii managed to actually test this.
Okay, I think I found what is the problem and why my testing on staging
wasn't really correct.

In xen/include/xen/stack_protector.h (
https://patchew.org/Xen/20241130010954.36057-1-volodymyr._5Fbabchuk@xxxxxxxx/20241130010954.36057-3-volodymyr._5Fbabchuk@xxxxxxxx/
) CONFIG_STACKPROTECTOR is used for ifdef-ing so the stub version of 
boot_stack_chk_guard_setup() is used and there is no need for
get_random() implementation:
1. Shouldn't it be /s/CONFIG_STACKPROTECTOR/CONFIG_STACK_PROTECTOR ?
2. CONFIG_STACK_PROTECTOR isn't set in case of RISC-V, at least:
      grep -Rni "STACK_PROTECTOR" xen/.config 
      38:CONFIG_HAS_STACK_PROTECTOR=y
      76:# CONFIG_STACK_PROTECTOR is not set
   
   Shouldn't it be default HAS_STACK_PROTECTOR ( or something similar )
   for 'config STACK_PROTECTOR':
      diff --git a/xen/common/Kconfig b/xen/common/Kconfig
      index 0ffd825510..f3156dbb9a 100644
      --- a/xen/common/Kconfig
      +++ b/xen/common/Kconfig
      @@ -521,6 +521,7 @@ config TRACEBUFFER
       
       config STACK_PROTECTOR
              bool "Stack protection"
      +       default HAS_STACK_PROTECTOR
              depends on HAS_STACK_PROTECTOR
              help
                Use compiler's option -fstack-protector (supported both by
      GCC
      
With these changes, I can confirm Jan's statement that the BUG_ON()
occurs due to the absence of the get_random()/NOW() implementation.

My second test (on my downstream branch) wasn't relevant because
get_s_time() and NOW() are implemented there.


Aside from the changes I mentioned above, I can re-send this patch when
I submit the patch enabling get_s_time() and NOW() for RISC-V. If you,
Volodymyr, are okay with that, we can proceed in this way.

Best regards,
 Oleksii