Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Uros Bizjak]

> 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.

#define __call_on_irqstack(func, asm_call, constr...) \
+{ \
+ 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 \

Please note that GCC documents "U" register constraint that can be
used here instead of declaring hard register in the variable
declaration:

    'U'
         The call-clobbered integer registers.

+ : [__func] "i" (func), [__tos] "r" (tos) constr \

There is no need to declare "tos" as read operand again, it is already
declared above as readwrite (+) operand.

Considering that (according to the above documentation) it is
necessary to list all input registers that pass function arguments,
the compiler is free to allocate any remaining register from "U"
register class, not only r11. Using an earlyclobber modifier prevents
the compiler from allocating a register that carries input argument,
so:

: [__tos] "+&U" (tos) IRQSTACK_CALL_CONSTRAINT \
: [__func] "i" (func) constr \

could be used.

Also note that functions with variable arguments pass information
about the number of vector registers used in %rax, so %rax should be
listed as input argument in this case. But this should be of no issue
here.

Uros.

+ : "cc", "rax", "rcx", "rdx", "rsi", "rdi", "r8", "r9", "r10", \
+  "memory" \
+ ); \

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Thomas Gleixner]

Uros,

On Fri, Feb 05 2021 at 12:03, Uros Bizjak wrote:

can you please fix your mail client to generate at least the 'In-Reply-to:'
header? Lacking that header breaks threading on lore:

        https://lore.kernel.org/lkml/20210204204903.350275743@linutronix.de/

Your mail is missing there. Ideally it also emits 'References'.

> #define __call_on_irqstack(func, asm_call, constr...) \
> +{ \
> + 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 \
>
> Please note that GCC documents "U" register constraint that can be
> used here instead of declaring hard register in the variable
> declaration:
>
>     'U'
>          The call-clobbered integer registers.

That's not really helpful because clang does not support 'U'.

> + : [__func] "i" (func), [__tos] "r" (tos) constr \
>
> There is no need to declare "tos" as read operand again, it is already
> declared above as readwrite (+) operand.

It makes clang builds fail.

> Considering that (according to the above documentation) it is
> necessary to list all input registers that pass function arguments,
> the compiler is free to allocate any remaining register from "U"
> register class, not only r11. Using an earlyclobber modifier prevents
> the compiler from allocating a register that carries input argument,
> so:
>
> : [__tos] "+&U" (tos) IRQSTACK_CALL_CONSTRAINT \
> : [__func] "i" (func) constr \
>
> could be used.

See above. Without the U constraint we can't rely on the compiler to do
the right thing without the explicit register asm("r11"); And even with
'U' we need to enforce that there is only one U register left to use.

The problem is that the compiler does not know about the call. So we
need to ensure via the clobbers and input/output arguments that it can't
use any of the callee clobbered registers accross the inline asm.

With

        void *tos = this_cpu_read(...);

        : "cc", .... "r9", "r10"

the compiler could still use "r11" for some other stuff and stick tos
into a callee saved register, e.g. r15. If the called function then
clobbers "r11" everything goes south.

There is no point in being extra smart here. The functions have no
register pressure as they are small so enforcing the register allocation
is not restricting the compiler freedom to much. But it ensures that the
compiler can't do anything subtly wrong which would end up being a hard
to debug disaster.

> Also note that functions with variable arguments pass information
> about the number of vector registers used in %rax, so %rax should be
> listed as input argument in this case. But this should be of no issue
> here.

That's really irrelevant as it's a very narrow use case for functions
with 0..2 arguments.

Thanks,

        tglx

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Thomas Gleixner]

On Tue, Feb 09 2021 at 10:22, Josh Poimboeuf wrote:
> On Tue, Feb 09, 2021 at 04:12:33PM +0100, Thomas Gleixner wrote:
>> On Mon, Feb 08 2021 at 14:42, Josh Poimboeuf wrote:
>> > On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
>> >>  #ifdef CONFIG_X86_64
>> >> +
>> >> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> >> +# define IRQSTACK_CALL_CONSTRAINT	, ASM_CALL_CONSTRAINT
>> >> +#else
>> >> +# define IRQSTACK_CALL_CONSTRAINT
>> >> +#endif
>> >
>> > Is this really needed?  i.e. does ASM_CALL_CONSTRAINT actually affect
>> > code generation with !FRAME_POINTER?
>> 
>> The problem is that if the asm inline is the first operation in a
>> function some compilers insert the asm inline before setting up the
>> frame pointer.
>> 
>> That's actualy irrelevant here as the compiler cannot reorder against
>> the C code leading to the asm inline. So we can probably replace it with
>> a big fat comment.
>
> Actually, I think keeping ASM_CALL_CONSTRAINT is a good idea.
>
> What I meant was, is the #ifdef needed?  My previous understanding was
> that ASM_CALL_CONSTRAINT has no effect for !FRAME_POINTER (i.e., ORC).
>
> So is there any reason to *not* have ASM_CALL_CONSTRAINT with ORC?

You're right. No idea how I ended up with that ifdef.

Thanks,

        tglx

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Josh Poimboeuf]

On Tue, Feb 09, 2021 at 04:12:33PM +0100, Thomas Gleixner wrote:
> On Mon, Feb 08 2021 at 14:42, Josh Poimboeuf wrote:
> > On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
> >>  #ifdef CONFIG_X86_64
> >> +
> >> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> >> +# define IRQSTACK_CALL_CONSTRAINT	, ASM_CALL_CONSTRAINT
> >> +#else
> >> +# define IRQSTACK_CALL_CONSTRAINT
> >> +#endif
> >
> > Is this really needed?  i.e. does ASM_CALL_CONSTRAINT actually affect
> > code generation with !FRAME_POINTER?
> 
> The problem is that if the asm inline is the first operation in a
> function some compilers insert the asm inline before setting up the
> frame pointer.
> 
> That's actualy irrelevant here as the compiler cannot reorder against
> the C code leading to the asm inline. So we can probably replace it with
> a big fat comment.

Actually, I think keeping ASM_CALL_CONSTRAINT is a good idea.

What I meant was, is the #ifdef needed?  My previous understanding was
that ASM_CALL_CONSTRAINT has no effect for !FRAME_POINTER (i.e., ORC).

So is there any reason to *not* have ASM_CALL_CONSTRAINT with ORC?

-- 
Josh

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Thomas Gleixner]

On Mon, Feb 08 2021 at 14:42, Josh Poimboeuf wrote:
> On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
>>  #ifdef CONFIG_X86_64
>> +
>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> +# define IRQSTACK_CALL_CONSTRAINT	, ASM_CALL_CONSTRAINT
>> +#else
>> +# define IRQSTACK_CALL_CONSTRAINT
>> +#endif
>
> Is this really needed?  i.e. does ASM_CALL_CONSTRAINT actually affect
> code generation with !FRAME_POINTER?

The problem is that if the asm inline is the first operation in a
function some compilers insert the asm inline before setting up the
frame pointer.

That's actualy irrelevant here as the compiler cannot reorder against
the C code leading to the asm inline. So we can probably replace it with
a big fat comment.

Thanks,

        tglx

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Josh Poimboeuf]

On Thu, Feb 04, 2021 at 09:49:08PM +0100, Thomas Gleixner wrote:
>  #ifdef CONFIG_X86_64
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +# define IRQSTACK_CALL_CONSTRAINT	, ASM_CALL_CONSTRAINT
> +#else
> +# define IRQSTACK_CALL_CONSTRAINT
> +#endif

Is this really needed?  i.e. does ASM_CALL_CONSTRAINT actually affect
code generation with !FRAME_POINTER?

If so then we should rework ASM_CALL_CONSTRAINT itself to be something
similar to the above, as it's only ever needed with frame pointers.

-- 
Josh

Re: [patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Borislav Petkov]

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@linutronix.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

[patch 05/12] x86/irq: Provide macro for inlining irq stack switching

[Thomas Gleixner]

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@linutronix.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
+ *	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
+ *	  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
+ *	  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...)			\
+{									\
+	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			\
+	: "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);