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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/11/2024 1:10 am, Volodymyr Babchuk wrote:
> 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
> @@ -341,6 +342,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
>       */
>      system_state = SYS_STATE_boot;
>  
> +    boot_stack_chk_guard_setup();
> +
>      if ( acpi_disabled )
>      {
>          printk("Booting using Device Tree\n");

I still think that __stack_chk_guard wants setting up in ASM before
entering C.

The only reason this call is so late is because Xen's get_random()
sequence is less than helpful.  That wants rewriting somewhat, but maybe
now isn't the best time.

Even if you initialise __stack_chk_guard it to -1 rather than 0, it's
still got a better chance of catching errors during very early boot; the
instrumentation is present, but is using 0 as the canary value.

On x86, dumping the current TSC value into __stack_chk_guard would be
far better than using -1.  Even if it skewed to a lower number, it's
unpredictable and not going to reoccur by accident during a stack overrun.

Surely ARM has something similar it could use?

[edit] Yes, get_cycles(), which every architecture seems to have.  In
fact, swapping get_random() from NOW() to get_cycles() would be good
enough to get it usable from early assembly.

~Andrew

A better option for get_random() would be to use a proven PRNG (e.g. one
of the xorshift family), seeded with get_cycles() and then re-seeded
with a real RDRAND/etc instruction if such a capability is found to
exist on the hardware.