Re: [PATCH v7 1/3] xen: common: add ability to enable stack protector

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2025-03-24 13:50, Jan Beulich wrote:
> On 18.03.2025 03:34, Volodymyr Babchuk wrote:
>> Both GCC and Clang support -fstack-protector feature, which add stack
>> canaries to functions where stack corruption is possible. This patch
>> makes general preparations to enable this feature on different
>> supported architectures:
>> 
>>  - Added CONFIG_HAS_STACK_PROTECTOR option so each architecture
>>    can enable this feature individually
>>  - Added user-selectable CONFIG_STACK_PROTECTOR option
>>  - Implemented code that sets up random stack canary and a basic
>>    handler for stack protector failures
>> 
>> Stack guard value is initialized in two phases:
>> 
>> 1. Pre-defined randomly-selected value.
>> 
>> 2. Own implementation linear congruent random number generator. It
>> relies on get_cycles() being available very early. If get_cycles()
>> returns zero, it would leave pre-defined value from the previous
>> step.

[...]

>> +void asmlinkage __stack_chk_fail(void)
> 
> The use of asmlinkage here comes close to an abuse: The Misra deviation 
> is
> about C code called from assembly code only. This isn't the case here; 
> instead
> it's a function that the compiler generates calls to without source 
> code
> explicitly saying so.
> 
> This imo wants approving from the Misra side as well, and even if 
> approved
> likely requires a justifying code comment.
> 

Here my suggestion would be an explicit deviation via a code comment, as 
described in [1], to describe the motivation of introducing such 
definition without a declaration. Moreover, asmlinkage is only relevant 
for the missing declaration, but is not effective for other rules. It is 
probably appropriate to mark the function "noreturn" as well, given its 
purpose.

[1] 
https://gitlab.com/xen-project/xen/-/blob/staging/docs/misra/documenting-violations.rst

>> --- /dev/null
>> +++ b/xen/include/xen/stack-protector.h
>> @@ -0,0 +1,39 @@
>> +#ifndef __XEN_STACK_PROTECTOR_H__
>> +#define __XEN_STACK_PROTECTOR_H__
>> +
>> +extern unsigned long __stack_chk_guard;
>> +
>> +/*
>> + * This function should be called from a C function that escapes 
>> stack
>> + * canary tracking (by calling reset_stack_and_jump() for example).
>> + */
>> +static always_inline void boot_stack_chk_guard_setup(void)
>> +{
>> +#ifdef CONFIG_STACK_PROTECTOR
>> +
>> +	/*
> 
> Nit: Hard tab slipped in.
> 
> Jan

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253