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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/12/2024 11:16 pm, Julien Grall wrote:
> 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.

Only in v8.5 as far as I can see, and even then it's not required. 
Also, it suffers from the same problem as RDRAND on x86; we need to boot
to at least feature detection before we can safely use it if it's available.

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

The C instrumentation is always present, right from the very start of
start_xen().

Even working with a canary of 0, there's some value.  It will spot
clobbering with a non-zero value, but it won't spot e.g. an overly-long
memset(, 0).

During boot, we're not defending against a malicious entity.  Simply
defending against bad developer expectations.

Therefore, anything to get a non-zero value prior to entering C will be
an improvement.  Best-effort is fine, and if there's one platform with
an errata that causes it to miss out, then oh well.  Any other platform
which manifests a crash will still lead to the problem being fixed.

I suppose taking this argument to it's logical conclusion, we could
initialise __stack_chk_guard with a poison pattern, although not one
shared by any other poison pattern in Xen.  That alone would be better
than using 0 in early boot.

~Andrew