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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Tue, 3 Dec 2024 at 22:00, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> 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?

There is a optional system register to read a random number.

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

Not quite. Technically we can't rely on the timer counter until
platform_init_time() is called.
This was to cater an issue on the exynos we used in OssTest. But
arguably this is the exception
rather than the norm because the firmware ought to properly initialize
the timer...

I haven't checked recent firmware. But I could be convinced to access
the counter before
hand if we really think that setting __stack_chk_guard from ASM is much better.

Cheers,