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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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