Re: [PATCH v3 2/3] xen: common: add ability to enable stack protector

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Jan,

Jan Beulich <jbeulich@xxxxxxxx> writes:

> On 11.12.2024 03:04, Volodymyr Babchuk wrote:

[...]

>
>> @@ -213,6 +216,18 @@ config SPECULATIVE_HARDEN_LOCK
>>  
>>  endmenu
>>  
>> +menu "Compiler options"
>> +
>> +config STACK_PROTECTOR
>> +    bool "Stack protector"
>> +    depends on HAS_STACK_PROTECTOR
>> +    help
>> +      Enable the Stack Protector compiler hardening option. This inserts a
>> +      canary value in the stack frame of functions, and performs an 
>> integrity
>> +      check on exit.
>> +
>> +endmenu
>
> "Compiler options" reads a little odd to me as a menu title. The preceding one
> is "Speculative hardening"; how about making this one "Other hardening" or 
> some
> such?

It was on of the Andrew's suggestion. Other was "Hardening". So yes, I
can rename it to "Other hardening", hope Andrew will be okay with this.

[...]

>> +/* This function should be called from ASM only */
>
> And with no (stack-protector enabled) C functions up the call stack. This
> may be as easy to express in the comment as by simply adding "early".

Like "This function should be called from early ASM only" ?

> However, considering the so far hypothetical case of offering the feature
> also on x86: What about xen.efi, which from the very start uses C code?
>

It depends on what other services are available. If RNG can be used
already, we don't need to call this function at all and can use
boot_stack_chk_guard_setup() right away.

>> +void __init asmlinkage boot_stack_chk_guard_setup_early(void)
>> +{

[...]

>> +void asmlinkage __stack_chk_fail(void)
>> +{
>> +    panic("Stack Protector integrity violation identified in %ps\n",
>> +      __builtin_return_address(0));
>
> Again.
>
> Is panic() really the right construct to use here, though?
> __builtin_return_address() will merely identify the immediate caller. A
> full stack trace (from BUG()) would likely be more useful in identifying
> the offender.

Okay, I'll put just plain BUG(); here.

>
>> --- a/xen/include/asm-generic/random.h
>> +++ b/xen/include/asm-generic/random.h
>> @@ -2,6 +2,11 @@
>>  #ifndef __ASM_GENERIC_RANDOM_H__
>>  #define __ASM_GENERIC_RANDOM_H__
>>  
>> +/*
>> + * When implementing arch_get_random(), please make sure that
>> + * it can provide random data before stack protector is initialized
>> + * (i.e. before boot_stack_chk_guard_setup() is called).
>> + */
>>  static inline unsigned int arch_get_random(void)
>>  {
>>      return 0;
>
> What exactly will go (entirely) wrong if the comment isn't followed?

This will not cause immediate harm, but it will give false confidence to
anyone who enables stack protector.

I'd prefer more substantial protection, but we can't even check if
random generator is available in runtime. Taking into account that we
potential can get 0 as result of RNG, we can't even put
WARN_ON(!arch_get_random()) check.

> (I'm afraid anyway that the comment living here is easy to miss.)

I didn't found a better place for it. Maybe you can suggest one?


[...]

Thank you for the review. I'll address all your other comments.

-- 
WBR, Volodymyr