Message-ID: <20210208150649.GB18227@zn.tnic>
Date:   Mon, 8 Feb 2021 16:06:49 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack
 switching

On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
> The effort to make the ASM entry code slim and unified moved the irq stack
> switching out of the low level ASM code so that the whole return from
> interrupt work and state handling can be done in C and the ASM code just
> handles the low level details of entry and exit.
> 
> This ended up being a suboptimal implementation for various reasons
> (including tooling). The main pain points are:
> 
>  - The indirect call which is expensive thanks to retpoline
> 
>  - The inability to stay on the irq stack for softirq processing on return
>    from interrupt
> 
>  - The fact that the stack switching code ends up being an easy to target
>    exploit gadget.
> 
> Prepare for inlining the stack switching logic into the C entry points by
> providing a ASM macro which contains the guts of the switching mechanism:
> 
>   1) Store RSP at the top of the irq stack
>   2) Switch RSP to the irq stack
>   3) Invoke code
>   4) Pop the original RSP back
> 
> Document the unholy asm() logic while at it to reduce the amount of head
> scratching required a half year from now.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/include/asm/irq_stack.h |  104 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> --- a/arch/x86/include/asm/irq_stack.h
> +++ b/arch/x86/include/asm/irq_stack.h
> @@ -7,6 +7,110 @@
>  #include <asm/processor.h>
>  
>  #ifdef CONFIG_X86_64
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +# define IRQSTACK_CALL_CONSTRAINT	, ASM_CALL_CONSTRAINT
> +#else
> +# define IRQSTACK_CALL_CONSTRAINT
> +#endif
> +
> +/*
> + * Macro to inline switching to an interrupt stack and invoking function
> + * calls from there. The following rules apply:
> + *
> + * - Ordering:
> + *
> + *   1. Write the stack pointer content into the top most place of

I think "content" is not needed here - just "Write the stack pointer".

> + *	the irq stack. This ensures that the various unwinders can
> + *	link back to the original stack.
> + *
> + *   2. Switch the stack pointer to the top of the irq stack.
> + *
> + *   3. Invoke whatever needs to be done (@asm_call argument)
> + *
> + *   4. Pop the original stack pointer from the top of the irq stack
> + *	which brings it back to the original stack where it left off.
> + *
> + * - Function invocation:
> + *
> + *   To allow flexible usage of the macro, the actual function code including
> + *   the store of the arguments in the call ABI registers is handed in via
> + *   the @asm_call argument.
> + *
> + * - Local variables:
> + *
> + *   @tos:
> + *	The @tos variable holds a pointer to the top of the irq stack and
> + *	_must_ be allocated in a non-callee saved register as this is a
> + *	restriction coming from objtool.
> + *
> + *	Note, that (tos) is both in input and output constraints to ensure
> + *	that the compiler does not assume that R11 is left untouched in
> + *	case this macro is used in some place where the per cpu interrupt
> + *	stack pointer is used again afterwards
> + *
> + * - Function arguments:
> + *        The function argument(s) if any have to be defined in register

Commas:

The function argument(s), if any, ...

> + *	  variables at the place where this is invoked. Storing the
> + *	  argument(s) in the proper register(s) is part of the @asm_call
> + *
> + * - Constraints:
> + *
> + *   The constraints have to be done very carefully because the compiler
> + *   does not know about the assembly call.
> + *
> + *   output:
> + *     As documented already above the @tos variable is required to be in
> + *     the output constraints to make the compiler aware that R11 cannot be
> + *     reused after the asm() statement.
> + *
> + *     For builds with CONFIG_UNWIND_FRAME_POINTER ASM_CALL_CONSTRAINT is
> + *     required as well as this prevents certain creative GCC variants from
> + *     misplacing the ASM code.
> + *
> + *  input:
> + *    - func:
> + *	  Immediate, which tells the compiler that the function is referenced.
> + *
> + *    - tos:
> + *	  Register. The actual register is defined by the variable declaration.
> + *
> + *    - function arguments:
> + *	  The constraints are handed in via the 'argconstr' argument list. They

"argconstr" or "constr"?

> + *	  describe the register arguments which are used in @asm_call.
> + *
> + *  clobbers:
> + *     Function calls can clobber anything except the callee-saved
> + *     registers. Tell the compiler.
> + */
> +#define __call_on_irqstack(func, asm_call, constr...)			\

Does the name need to be prepended with "__"? I don't see a
"call_on_irqstack" variant...

> +{									\
> +	register void *tos asm("r11");					\
> +									\
> +	tos = ((void *)__this_cpu_read(hardirq_stack_ptr));		\
> +									\
> +	asm_inline volatile(						\
> +	"movq	%%rsp, (%[__tos])			\n"		\
> +	"movq	%[__tos], %%rsp				\n"		\
> +									\
> +	asm_call							\
> +									\
> +	"popq	%%rsp					\n"		\
> +									\
> +	: "+r" (tos) IRQSTACK_CALL_CONSTRAINT				\
> +	: [__func] "i" (func), [__tos] "r" (tos) constr			\

I think you can call the symbolic names the same as the variables -
i.e., without the "__" so that there's less confusion when looking at
the code.

> +	: "cc", "rax", "rcx", "rdx", "rsi", "rdi", "r8", "r9", "r10",	\
> +	  "memory"							\
> +	);								\
> +}
> +
> +/* Macros to assert type correctness for run_*_on_irqstack macros */
> +#define assert_function_type(func, proto)				\
> +	static_assert(__builtin_types_compatible_p(typeof(&func), proto))
> +
> +#define assert_arg_type(arg, proto)					\
> +	static_assert(__builtin_types_compatible_p(typeof(arg), proto))
> +
>  static __always_inline bool irqstack_active(void)
>  {
>  	return __this_cpu_read(hardirq_stack_inuse);
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists