| Message ID | 20220713213133.455599-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Allow for exclusions in checking RETHUNK | expand |
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) \
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.
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?
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 :-/
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.
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; /*
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(-)