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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Oleksii,


oleksii.kurochko@xxxxxxxxx writes:

> 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.

Thank you for testing this once more. I'll drop this patch from v3 then, so
you'll be able to include it into your series.

--
WBR, Volodymyr