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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2024 01:47, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@xxxxxxxx> writes:
>> On 11.12.2024 03:04, Volodymyr Babchuk wrote:
>>> --- 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.

Right, and hence 0 isn't strictly something that can be called "bad".
With at least some randomness one will of course observe a possible
problem at least across two or more runs. However, you don't call
arch_get_random() directly anyway, and get_random() has fallback code,
which is no more likely to return 0 than arch_get_random() is.

In fact this fallback code means get_random() will also use it when
arch_get_random() returns 0 as coming from an actual RNG. Which can
be considered bogus, as for a good random number source _every_
possible value ought to have equal probability of being returned.
Plus if we special-case 0, why not also special-case ~0? Or any other
number with only very few bits set/clear?

For the purpose of stack protector we may want to consider using a
mix of static pattern (not used elsewhere in the codebase) and random
number. The static pattern part would then want arranging such that
at least any value representing a valid address (within Xen alone)
won't match possible canary values.

>> (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?

I'm of two minds here really. Part of me wants this simply dropped. Yet
then some may deem this worthwhile information, except that then it
needs to live is a suitably exposed place. Just that I can't think of
any that would really fit.

Jan