[PATCH] x86: Allow for exclusions in checking RETHUNK

[PATCH] x86: Allow for exclusions in checking RETHUNK

[Kees Cook]

LKDTM builds a "just return" function that lives in .rodata, but this
creates problems when validating alternatives in the face of RETHUNK.
Export RETHUNK_CFLAGS so they can be disabled for the LKDTM function,
and ask objtool to ignore this function. (Use of STACK_FRAME_NON_STANDARD
here seems to generate a non-.rela section, that needed to be adjusted.)

Reported-by: kernel test robot <oliver.sang@intel.com>
Link: https://lore.kernel.org/lkml/Ys58BxHxoDZ7rfpr@xsang-OptiPlex-9020/
Debugged-by: Peter Zijlstra <peterz@infradead.org>
Fixes: ee88d363d156 ("x86,static_call: Use alternative RET encoding")
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: x86@kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/Makefile           | 1 +
 drivers/misc/lkdtm/Makefile | 2 +-
 drivers/misc/lkdtm/rodata.c | 4 ++++
 tools/objtool/check.c       | 4 +++-
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 1f40dad30d50..7854685c5f25 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -27,6 +27,7 @@ RETHUNK_CFLAGS		:= -mfunction-return=thunk-extern
 RETPOLINE_CFLAGS	+= $(RETHUNK_CFLAGS)
 endif
 
+export RETHUNK_CFLAGS
 export RETPOLINE_CFLAGS
 export RETPOLINE_VDSO_CFLAGS
 
diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
index 2e0aa74ac185..fd96ac1617f7 100644
--- a/drivers/misc/lkdtm/Makefile
+++ b/drivers/misc/lkdtm/Makefile
@@ -16,7 +16,7 @@ lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
 KASAN_SANITIZE_rodata.o		:= n
 KASAN_SANITIZE_stackleak.o	:= n
 KCOV_INSTRUMENT_rodata.o	:= n
-CFLAGS_REMOVE_rodata.o		+= $(CC_FLAGS_LTO)
+CFLAGS_REMOVE_rodata.o		+= $(CC_FLAGS_LTO) $(RETHUNK_CFLAGS)
 
 OBJCOPYFLAGS :=
 OBJCOPYFLAGS_rodata_objcopy.o	:= \
diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c
index baacb876d1d9..708a2558a7ac 100644
--- a/drivers/misc/lkdtm/rodata.c
+++ b/drivers/misc/lkdtm/rodata.c
@@ -4,8 +4,12 @@
  * (via objcopy tricks), to validate the non-executability of .rodata.
  */
 #include "lkdtm.h"
+#include <linux/objtool.h>
 
 void noinstr lkdtm_rodata_do_nothing(void)
 {
 	/* Does nothing. We just want an architecture agnostic "return". */
 }
+
+/* This is a lie, but given the objcopy, we need objtool to ignore it. */
+STACK_FRAME_NON_STANDARD(lkdtm_rodata_do_nothing);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b341f8a8c7c5..c1b58a682ace 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -902,6 +902,8 @@ static void add_ignores(struct objtool_file *file)
 	struct reloc *reloc;
 
 	sec = find_section_by_name(file->elf, ".rela.discard.func_stack_frame_non_standard");
+	if (!sec)
+		sec = find_section_by_name(file->elf, ".discard.func_stack_frame_non_standard");
 	if (!sec)
 		return;
 
@@ -3719,7 +3721,7 @@ static int validate_retpoline(struct objtool_file *file)
 		    insn->type != INSN_RETURN)
 			continue;
 
-		if (insn->retpoline_safe)
+		if (insn->retpoline_safe || insn->ignore)
 			continue;
 
 		/*
-- 
2.32.0

Re: [PATCH] x86: Allow for exclusions in checking RETHUNK

[Josh Poimboeuf]

On Wed, Jul 13, 2022 at 02:31:33PM -0700, Kees Cook wrote:
> +++ b/drivers/misc/lkdtm/rodata.c
> @@ -4,8 +4,12 @@
>   * (via objcopy tricks), to validate the non-executability of .rodata.
>   */
>  #include "lkdtm.h"
> +#include <linux/objtool.h>
>  
>  void noinstr lkdtm_rodata_do_nothing(void)
>  {
>  	/* Does nothing. We just want an architecture agnostic "return". */
>  }

Since the function only consists of a single RET instruction, could we
just do an asm(ANNOTATE_UNSAFE_RET) here?  i.e. see patch below.

> +/* This is a lie, but given the objcopy, we need objtool to ignore it. */
> +STACK_FRAME_NON_STANDARD(lkdtm_rodata_do_nothing);
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index b341f8a8c7c5..c1b58a682ace 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -902,6 +902,8 @@ static void add_ignores(struct objtool_file *file)
>  	struct reloc *reloc;
>  
>  	sec = find_section_by_name(file->elf, ".rela.discard.func_stack_frame_non_standard");
> +	if (!sec)
> +		sec = find_section_by_name(file->elf, ".discard.func_stack_frame_non_standard");
>  	if (!sec)
>  		return;

Why was there no .rela section?

Anyway I don't see how this can work, since the code below it traverses
sec->reloc_list, which only exists for rela sections.

Here's the ANNOTATE_UNSAFE_RET idea.  Most of the diff is moving the
annotation macros to objtool.h (where they belong anyway).

If there are no objections I can split this up into proper patches
tomorrow.

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index bb05ed4f46bd..9b7cfc68767b 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -63,35 +63,6 @@
 
 #ifdef __ASSEMBLY__
 
-/*
- * This should be used immediately before an indirect jump/call. It tells
- * objtool the subsequent indirect jump/call is vouched safe for retpoline
- * builds.
- */
-.macro ANNOTATE_RETPOLINE_SAFE
-	.Lannotate_\@:
-	.pushsection .discard.retpoline_safe
-	_ASM_PTR .Lannotate_\@
-	.popsection
-.endm
-
-/*
- * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions
- * vs RETBleed validation.
- */
-#define ANNOTATE_UNRET_SAFE ANNOTATE_RETPOLINE_SAFE
-
-/*
- * Abuse ANNOTATE_RETPOLINE_SAFE on a NOP to indicate UNRET_END, should
- * eventually turn into it's own annotation.
- */
-.macro ANNOTATE_UNRET_END
-#ifdef CONFIG_DEBUG_ENTRY
-	ANNOTATE_RETPOLINE_SAFE
-	nop
-#endif
-.endm
-
 /*
  * JMP_NOSPEC and CALL_NOSPEC macros can be used instead of a simple
  * indirect jmp/call which may be susceptible to the Spectre variant 2
@@ -155,12 +126,6 @@
 
 #else /* __ASSEMBLY__ */
 
-#define ANNOTATE_RETPOLINE_SAFE					\
-	"999:\n\t"						\
-	".pushsection .discard.retpoline_safe\n\t"		\
-	_ASM_PTR " 999b\n\t"					\
-	".popsection\n\t"
-
 typedef u8 retpoline_thunk_t[RETPOLINE_THUNK_SIZE];
 extern retpoline_thunk_t __x86_indirect_thunk_array[];
 
diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c
index 708a2558a7ac..b4aeb50c4a96 100644
--- a/drivers/misc/lkdtm/rodata.c
+++ b/drivers/misc/lkdtm/rodata.c
@@ -8,6 +8,7 @@
 
 void noinstr lkdtm_rodata_do_nothing(void)
 {
+	asm(ANNOTATE_RETPOLINE_SAFE);
 	/* Does nothing. We just want an architecture agnostic "return". */
 }
 
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 10bc88cc3bf6..0bd80ba8e6b2 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -43,6 +43,12 @@ struct unwind_hint {
 #define UNWIND_HINT_TYPE_SAVE		5
 #define UNWIND_HINT_TYPE_RESTORE	6
 
+/*
+ * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions
+ * vs RETBleed validation.
+ */
+#define ANNOTATE_UNRET_SAFE ANNOTATE_RETPOLINE_SAFE
+
 #ifdef CONFIG_OBJTOOL
 
 #include <asm/asm.h>
@@ -84,6 +90,12 @@ struct unwind_hint {
 #define STACK_FRAME_NON_STANDARD_FP(func)
 #endif
 
+#define ANNOTATE_RETPOLINE_SAFE					\
+	"999:\n\t"						\
+	".pushsection .discard.retpoline_safe\n\t"		\
+	_ASM_PTR " 999b\n\t"					\
+	".popsection\n\t"
+
 #define ANNOTATE_NOENDBR					\
 	"986: \n\t"						\
 	".pushsection .discard.noendbr\n\t"			\
@@ -98,6 +110,29 @@ struct unwind_hint {
 
 #else /* __ASSEMBLY__ */
 
+/*
+ * This should be used immediately before an indirect jump/call. It tells
+ * objtool the subsequent indirect jump/call is vouched safe for retpoline
+ * builds.
+ */
+.macro ANNOTATE_RETPOLINE_SAFE
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_safe
+	_ASM_PTR .Lannotate_\@
+	.popsection
+.endm
+
+/*
+ * Abuse ANNOTATE_RETPOLINE_SAFE on a NOP to indicate UNRET_END, should
+ * eventually turn into it's own annotation.
+ */
+.macro ANNOTATE_UNRET_END
+#ifdef CONFIG_DEBUG_ENTRY
+	ANNOTATE_RETPOLINE_SAFE
+	nop
+#endif
+.endm
+
 /*
  * This macro indicates that the following intra-function call is valid.
  * Any non-annotated intra-function call will cause objtool to issue a warning.
@@ -172,6 +207,8 @@ struct unwind_hint {
 
 #else /* !CONFIG_OBJTOOL */
 
+#define ANNOTATE_RETPOLINE_SAFE
+
 #ifndef __ASSEMBLY__
 
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)	\
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index 10bc88cc3bf6..0bd80ba8e6b2 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -43,6 +43,12 @@ struct unwind_hint {
 #define UNWIND_HINT_TYPE_SAVE		5
 #define UNWIND_HINT_TYPE_RESTORE	6
 
+/*
+ * (ab)use RETPOLINE_SAFE on RET to annotate away 'bare' RET instructions
+ * vs RETBleed validation.
+ */
+#define ANNOTATE_UNRET_SAFE ANNOTATE_RETPOLINE_SAFE
+
 #ifdef CONFIG_OBJTOOL
 
 #include <asm/asm.h>
@@ -84,6 +90,12 @@ struct unwind_hint {
 #define STACK_FRAME_NON_STANDARD_FP(func)
 #endif
 
+#define ANNOTATE_RETPOLINE_SAFE					\
+	"999:\n\t"						\
+	".pushsection .discard.retpoline_safe\n\t"		\
+	_ASM_PTR " 999b\n\t"					\
+	".popsection\n\t"
+
 #define ANNOTATE_NOENDBR					\
 	"986: \n\t"						\
 	".pushsection .discard.noendbr\n\t"			\
@@ -98,6 +110,29 @@ struct unwind_hint {
 
 #else /* __ASSEMBLY__ */
 
+/*
+ * This should be used immediately before an indirect jump/call. It tells
+ * objtool the subsequent indirect jump/call is vouched safe for retpoline
+ * builds.
+ */
+.macro ANNOTATE_RETPOLINE_SAFE
+	.Lannotate_\@:
+	.pushsection .discard.retpoline_safe
+	_ASM_PTR .Lannotate_\@
+	.popsection
+.endm
+
+/*
+ * Abuse ANNOTATE_RETPOLINE_SAFE on a NOP to indicate UNRET_END, should
+ * eventually turn into it's own annotation.
+ */
+.macro ANNOTATE_UNRET_END
+#ifdef CONFIG_DEBUG_ENTRY
+	ANNOTATE_RETPOLINE_SAFE
+	nop
+#endif
+.endm
+
 /*
  * This macro indicates that the following intra-function call is valid.
  * Any non-annotated intra-function call will cause objtool to issue a warning.
@@ -172,6 +207,8 @@ struct unwind_hint {
 
 #else /* !CONFIG_OBJTOOL */
 
+#define ANNOTATE_RETPOLINE_SAFE
+
 #ifndef __ASSEMBLY__
 
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)	\

Re: [PATCH] x86: Allow for exclusions in checking RETHUNK

[Peter Zijlstra]

On Wed, Jul 13, 2022 at 04:55:56PM -0700, Josh Poimboeuf wrote:
> Here's the ANNOTATE_UNSAFE_RET idea.

Right, I suppose that strictly speaking the compiler can do whatever and
there's no actual guarantee the annotation hits the RET instruction, in
practise it should work, esp. since noinstr.

> Most of the diff is moving the
> annotation macros to objtool.h (where they belong anyway).

Yeah, moving those is a good idea.

Re: [PATCH] x86: Allow for exclusions in checking RETHUNK

[Josh Poimboeuf]

On Thu, Jul 14, 2022 at 09:18:12AM +0200, Peter Zijlstra wrote:
> On Wed, Jul 13, 2022 at 04:55:56PM -0700, Josh Poimboeuf wrote:
> > Here's the ANNOTATE_UNSAFE_RET idea.
> 
> Right, I suppose that strictly speaking the compiler can do whatever and
> there's no actual guarantee the annotation hits the RET instruction, in
> practise it should work, esp. since noinstr.

Hm, KASAN is introducing a weird function, resulting in a naked return
warning since we have RETHUNK_CFLAGS removed on that file.

0000000000000000 <_sub_I_00099_0>:
   0:	e8 00 00 00 00       	call   5 <_sub_I_00099_0+0x5>	1: R_X86_64_PLT32	__tsan_init-0x4
   5:	c3                   	ret    


Looks like the "KASAN_SANITIZE_rodata.o := n" isn't working somehow?

-- 
Josh

Re: [PATCH] x86: Allow for exclusions in checking RETHUNK

[Josh Poimboeuf]

On Thu, Jul 14, 2022 at 11:50:08AM -0700, Josh Poimboeuf wrote:
> On Thu, Jul 14, 2022 at 09:18:12AM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 13, 2022 at 04:55:56PM -0700, Josh Poimboeuf wrote:
> > > Here's the ANNOTATE_UNSAFE_RET idea.
> > 
> > Right, I suppose that strictly speaking the compiler can do whatever and
> > there's no actual guarantee the annotation hits the RET instruction, in
> > practise it should work, esp. since noinstr.
> 
> Hm, KASAN is introducing a weird function, resulting in a naked return
> warning since we have RETHUNK_CFLAGS removed on that file.
> 
> 0000000000000000 <_sub_I_00099_0>:
>    0:	e8 00 00 00 00       	call   5 <_sub_I_00099_0+0x5>	1: R_X86_64_PLT32	__tsan_init-0x4
>    5:	c3                   	ret    
> 
> 
> Looks like the "KASAN_SANITIZE_rodata.o := n" isn't working somehow?

Oh never mind, I got KASAN/KCSCAN mixed up.  Needs both disabled :-/

-- 
Josh

Re: [PATCH] x86: Allow for exclusions in checking RETHUNK

[Josh Poimboeuf]

On Thu, Jul 14, 2022 at 11:56:07AM -0700, Josh Poimboeuf wrote:
> On Thu, Jul 14, 2022 at 11:50:08AM -0700, Josh Poimboeuf wrote:
> > On Thu, Jul 14, 2022 at 09:18:12AM +0200, Peter Zijlstra wrote:
> > > On Wed, Jul 13, 2022 at 04:55:56PM -0700, Josh Poimboeuf wrote:
> > > > Here's the ANNOTATE_UNSAFE_RET idea.
> > > 
> > > Right, I suppose that strictly speaking the compiler can do whatever and
> > > there's no actual guarantee the annotation hits the RET instruction, in
> > > practise it should work, esp. since noinstr.
> > 
> > Hm, KASAN is introducing a weird function, resulting in a naked return
> > warning since we have RETHUNK_CFLAGS removed on that file.
> > 
> > 0000000000000000 <_sub_I_00099_0>:
> >    0:	e8 00 00 00 00       	call   5 <_sub_I_00099_0+0x5>	1: R_X86_64_PLT32	__tsan_init-0x4
> >    5:	c3                   	ret    
> > 
> > 
> > Looks like the "KASAN_SANITIZE_rodata.o := n" isn't working somehow?
> 
> Oh never mind, I got KASAN/KCSCAN mixed up.  Needs both disabled :-/

Well, my ANNOTATE_UNSAFE_RET trick didn't quite work either, as it
results in .discard.retpoline_safe pointing to .rodata when IBT is
enabled.

Instead I'll just do OBJECT_FILES_NON_STANDARD_rodata.o.  That shouldn't
break LTO/IBT because the linked code lives in .rodata anyway.

Will have patches tomorrow, if they pass bot testing.

-- 
Josh