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@epam.com>
Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
In v2:
- Reordered Kconfig entry
- Added Oleksii's Tested-by tag
---
xen/arch/riscv/Kconfig | 1 +
xen/arch/riscv/setup.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index 1858004676..79b3b68754 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -4,6 +4,7 @@ config RISCV
select GENERIC_BUG_FRAME
select HAS_DEVICE_TREE
select HAS_PMAP
+ select HAS_STACK_PROTECTOR
select HAS_VMAP
config RISCV_64
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 9680332fee..59eddb465a 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -7,6 +7,7 @@
#include <xen/init.h>
#include <xen/mm.h>
#include <xen/shutdown.h>
+#include <xen/stack-protector.h>
#include <xen/vmap.h>
#include <public/version.h>
@@ -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();
+
cmdline = boot_fdt_cmdline(device_tree_flattened);
printk("Command line: %s\n", cmdline);
cmdline_parse(cmdline);
--
2.47.1On 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@epam.com> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> 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. Jan
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@epam.com>
> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>
> 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@epam.com/20241130010954.36057-3-volodymyr._5Fbabchuk@epam.com/
) 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
Hello Oleksii, oleksii.kurochko@gmail.com 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@epam.com> >> > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >> >> 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@epam.com/20241130010954.36057-3-volodymyr._5Fbabchuk@epam.com/ > ) 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
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@epam.com> > > Tested-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > 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. I will double check that but it worked for me ( I didn't face BUG_ON() ). ~ Oleksii
© 2016 - 2024 Red Hat, Inc.