[PATCH 1/2] objtool: Don't warn for intra function calls in alternative

[PATCH 1/2] objtool: Don't warn for intra function calls in alternative

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

objtool suggests to put intra function calls, like used in
stuff rsb, into alternative, but then starts warning about the
alternative section too. Avoid these warnings.

Needed for the next patch.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/objtool/check.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2e458eb45586..ec46db0dcdc0 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -544,7 +544,8 @@ static int add_call_destinations(struct objtool_file *file)
 			insn->call_dest = find_symbol_by_offset(insn->sec,
 								dest_off);
 
-			if (!insn->call_dest && !insn->ignore) {
+			if (!insn->call_dest && !insn->ignore &&
+			    strcmp(insn->sec->name, ".altinstr_replacement")) {
 				WARN_FUNC("unsupported intra-function call",
 					  insn->sec, insn->offset);
 				WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
-- 
2.14.3

[PATCH 2/2] x86/retpoline: Fix return buffer filling

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

An earlier patch moved the RSB filling out of line, ending
it with a return. This results in the return buffer filling
only giving 15 instead of 16 usable returns because
the return from fill_rsb already uses one up.

Since the kernel call chains can be quite deep that's
somewhat dangerous and better avoided.

Always do one extra return buffer fill in the caller.
I added a STUFF_ONE_RSB macro for this, and fixed
up all the caller

An alternative would be to revert 1dde7415e999
and just go back to the inline version.

Needs the earlier patch to avoid lots of bogus warnings
from objtool, but even with that there is one left.

Fixes: 1dde7415e999 ("x86/retpoline: Simplify vmexit...")
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/nospec-branch.h | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 788c4da7dda9..af0ca73abb23 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -77,10 +77,19 @@
 #endif
 .endm
 
+.macro STUFF_ONE_RSB
+#ifdef CONFIG_RETPOLINE
+	call 581f
+	pause ; lfence
+581:	add  $(BITS_PER_LONG/8), %_ASM_SP
+#endif
+.endm
+
 /* This clobbers the BX register */
 .macro FILL_RETURN_BUFFER nr:req ftr:req
 #ifdef CONFIG_RETPOLINE
-	ALTERNATIVE "", "call __clear_rsb", \ftr
+	ALTERNATIVE "", "call __clear_rsb" , \ftr
+	ALTERNATIVE "", "STUFF_ONE_RSB", \ftr
 #endif
 .endm
 
@@ -133,6 +142,20 @@
 # define THUNK_TARGET(addr) [thunk_target] "rm" (addr)
 #endif
 
+#ifdef CONFIG_X86_64
+#define STUFF_ONE_RSB	\
+	"	call 881f\n"					\
+	"	pause;lfence\n"					\
+	"881:\n"						\
+	"	addq $8,%%rsp\n"
+#else
+#define STUFF_ONE_RSB	\
+	"	call 881f\n"					\
+	"	pause;lfence\n"					\
+	"881:\n"						\
+	"	addl $4,%%esp\n"
+#endif
+
 /* The Spectre V2 mitigation variants */
 enum spectre_v2_mitigation {
 	SPECTRE_V2_NONE,
@@ -156,7 +179,7 @@ static inline void vmexit_fill_RSB(void)
 {
 #ifdef CONFIG_RETPOLINE
 	alternative_input("",
-			  "call __fill_rsb",
+			  "call __fill_rsb;" STUFF_ONE_RSB,
 			  X86_FEATURE_RETPOLINE,
 			  ASM_NO_INPUT_CLOBBER(_ASM_BX, "memory"));
 #endif
-- 
2.14.3

Re: [PATCH 2/2] x86/retpoline: Fix return buffer filling

[Josh Poimboeuf]

On Mon, Feb 12, 2018 at 04:04:22PM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> An earlier patch moved the RSB filling out of line, ending
> it with a return. This results in the return buffer filling
> only giving 15 instead of 16 usable returns because
> the return from fill_rsb already uses one up.
> 
> Since the kernel call chains can be quite deep that's
> somewhat dangerous and better avoided.
> 
> Always do one extra return buffer fill in the caller.
> I added a STUFF_ONE_RSB macro for this, and fixed
> up all the caller
> 
> An alternative would be to revert 1dde7415e999
> and just go back to the inline version.
> 
> Needs the earlier patch to avoid lots of bogus warnings
> from objtool, but even with that there is one left.
> 
> Fixes: 1dde7415e999 ("x86/retpoline: Simplify vmexit...")
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Instead of patch 1/2, the alternatives need to be annotated like:

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index af0ca73abb23..4deeb869ab83 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -89,6 +89,7 @@
 .macro FILL_RETURN_BUFFER nr:req ftr:req
 #ifdef CONFIG_RETPOLINE
 	ALTERNATIVE "", "call __clear_rsb" , \ftr
+	ANNOTATE_NOSPEC_ALTERNATIVE
 	ALTERNATIVE "", "STUFF_ONE_RSB", \ftr
 #endif
 .endm
@@ -178,7 +179,7 @@ extern char __indirect_thunk_end[];
 static inline void vmexit_fill_RSB(void)
 {
 #ifdef CONFIG_RETPOLINE
-	alternative_input("",
+	alternative_input(ANNOTATE_NOSPEC_ALTERNATIVE,
 			  "call __fill_rsb;" STUFF_ONE_RSB,
 			  X86_FEATURE_RETPOLINE,
 			  ASM_NO_INPUT_CLOBBER(_ASM_BX, "memory"));

Re: [PATCH 2/2] x86/retpoline: Fix return buffer filling

[Borislav Petkov]

On Mon, Feb 12, 2018 at 04:04:22PM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> An earlier patch moved the RSB filling out of line, ending
> it with a return. This results in the return buffer filling
> only giving 15 instead of 16 usable returns because
> the return from fill_rsb already uses one up.

Or, we can get rid of the RET:

---
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index 480edc3a5e03..359130ceaa64 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -91,7 +91,8 @@ GENERATE_THUNK(r15)
 
 ENTRY(__fill_rsb)
 	STUFF_RSB RSB_FILL_LOOPS, %_ASM_SP
-	ret
+	pop %_ASM_BX
+	jmp *%_ASM_BX
 END(__fill_rsb)
 EXPORT_SYMBOL_GPL(__fill_rsb)
 
@@ -99,6 +100,7 @@ EXPORT_SYMBOL_GPL(__fill_rsb)
 
 ENTRY(__clear_rsb)
 	STUFF_RSB RSB_CLEAR_LOOPS, %_ASM_SP
-	ret
+	pop %_ASM_BX
+	jmp *%_ASM_BX
 END(__clear_rsb)
 EXPORT_SYMBOL_GPL(__clear_rsb)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/retpoline: Fix return buffer filling

[Borislav Petkov]

On Tue, Feb 13, 2018 at 12:21:18PM +0100, Borislav Petkov wrote:
> On Mon, Feb 12, 2018 at 04:04:22PM -0800, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > An earlier patch moved the RSB filling out of line, ending
> > it with a return. This results in the return buffer filling
> > only giving 15 instead of 16 usable returns because
> > the return from fill_rsb already uses one up.
> 
> Or, we can get rid of the RET:
> 
> ---
> diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> index 480edc3a5e03..359130ceaa64 100644
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -91,7 +91,8 @@ GENERATE_THUNK(r15)
>  
>  ENTRY(__fill_rsb)
>  	STUFF_RSB RSB_FILL_LOOPS, %_ASM_SP
> -	ret
> +	pop %_ASM_BX
> +	jmp *%_ASM_BX

... and that's an indirect JMP too :-\

I guess we could use RET far which is, reportedly, not affected.
Something like that, but I need to make it build first:

---
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 788c4da7dda9..04642f549817 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -6,6 +6,7 @@
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
+#include <asm/segment.h>
 
 #ifdef __ASSEMBLY__
 
@@ -80,7 +81,7 @@
 /* This clobbers the BX register */
 .macro FILL_RETURN_BUFFER nr:req ftr:req
 #ifdef CONFIG_RETPOLINE
-	ALTERNATIVE "", "call __clear_rsb", \ftr
+	ALTERNATIVE "", __stringify(push $__KERNEL_CS; call __clear_rsb), \ftr
 #endif
 .endm
 
@@ -156,7 +157,7 @@ static inline void vmexit_fill_RSB(void)
 {
 #ifdef CONFIG_RETPOLINE
 	alternative_input("",
-			  "call __fill_rsb",
+			  "push $__KERNEL_CS ; call __fill_rsb",
 			  X86_FEATURE_RETPOLINE,
 			  ASM_NO_INPUT_CLOBBER(_ASM_BX, "memory"));
 #endif
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index 480edc3a5e03..961661233b34 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -91,7 +91,7 @@ GENERATE_THUNK(r15)
 
 ENTRY(__fill_rsb)
 	STUFF_RSB RSB_FILL_LOOPS, %_ASM_SP
-	ret
+	lret
 END(__fill_rsb)
 EXPORT_SYMBOL_GPL(__fill_rsb)
 
@@ -99,6 +99,6 @@ EXPORT_SYMBOL_GPL(__fill_rsb)
 
 ENTRY(__clear_rsb)
 	STUFF_RSB RSB_CLEAR_LOOPS, %_ASM_SP
-	ret
+	lret
 END(__clear_rsb)
 EXPORT_SYMBOL_GPL(__clear_rsb)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/retpoline: Fix return buffer filling

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Mon, 2018-02-12 at 16:04 -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> An earlier patch moved the RSB filling out of line, ending
> it with a return. This results in the return buffer filling
> only giving 15 instead of 16 usable returns because
> the return from fill_rsb already uses one up.
> 
> Since the kernel call chains can be quite deep that's
> somewhat dangerous and better avoided.

This only matters for Skylake, right? In conjunction with the call
depth counting stuff that Ingo and Thomas looked at and seem to have
given up on?

On non-Skylake we don't care about underflow, and all that matters is
that we get rid of any *bogus* entries in the RSB? 

However... that was supposed to be a 'clear RSB' operation, with 32
CALLs in sequence. And Boris changed it to 16 by calling __fill_rsb()
instead of __clear_rsb():

-       asm volatile (ANNOTATE_NOSPEC_ALTERNATIVE
-                     ALTERNATIVE("jmp 910f",
-                                 __stringify(__FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1)),
-                                 X86_FEATURE_RETPOLINE)
-                     "910:"
-                     : "=r" (loops), ASM_CALL_CONSTRAINT
-                     : : "memory" );
+       alternative_input("",
+                         "call __fill_rsb",
+                         X86_FEATURE_RETPOLINE,
+                         ASM_NO_INPUT_CLOBBER(_ASM_BX, "memory"));

I think we do need to revert that patch. And perhaps stop accepting any
more similar bikeshedding.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [PATCH 2/2] x86/retpoline: Fix return buffer filling

[Andi Kleen]

> However... that was supposed to be a 'clear RSB' operation, with 32
> CALLs in sequence. And Boris changed it to 16 by calling __fill_rsb()
> instead of __clear_rsb():

True. That's even worse.
> 
> -       asm volatile (ANNOTATE_NOSPEC_ALTERNATIVE
> -                     ALTERNATIVE("jmp 910f",
> -                                 __stringify(__FILL_RETURN_BUFFER(%0, RSB_CLEAR_LOOPS, %1)),
> -                                 X86_FEATURE_RETPOLINE)
> -                     "910:"
> -                     : "=r" (loops), ASM_CALL_CONSTRAINT
> -                     : : "memory" );
> +       alternative_input("",
> +                         "call __fill_rsb",
> +                         X86_FEATURE_RETPOLINE,
> +                         ASM_NO_INPUT_CLOBBER(_ASM_BX, "memory"));
> 
> I think we do need to revert that patch. And perhaps stop accepting any
> more similar bikeshedding.

Yes revertion would be the right way.

I already regret the time I wasted trying to fix it.

-Andi