| Message ID | 20190222222635.GK14054@worktop.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC] objtool: STAC/CLAC validation | expand |
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@infradead.org> wrote: > > So find the below patch -- which spotted fail in ptrace.c > > It has an AC_SAFE(func) annotation which allows marking specific > functions as safe to call. The patch includes 2 instances which were > required to make arch/x86 'build': Looks sane enough to me. Can you make it do DF too while at it? I really think AC and DF are the same in this context. If you call an arbitrary function with DF set, things will very quickly go sideways (even if it might work in practice as long as the function just doesn't do a memcpy or similar) Linus
On February 22, 2019 2:26:35 PM PST, Peter Zijlstra <peterz@infradead.org> wrote: >On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote: > >> But correct :) > >> I agree, that a function which is doing the actual copy should be >callable, >> but random other functions? NO! > >So find the below patch -- which spotted fail in ptrace.c > >It has an AC_SAFE(func) annotation which allows marking specific >functions as safe to call. The patch includes 2 instances which were >required to make arch/x86 'build': > >arch/x86/ia32/ia32_signal.o: warning: objtool: >ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC >set >arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to >getreg() with AC set > >It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the >lack of notrace annotations on functions marked AC_SAFE(): > >arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to >__fentry__() with AC set > >It builds arch/x86 relatively clean; it only complains about some >redundant CLACs in entry_64.S because it doesn't understand interrupts >and I've not bothered creating an annotation for them yet. > >arch/x86/entry/entry_64.o: warning: objtool: >.altinstr_replacement+0x4d: redundant CLAC >arch/x86/entry/entry_64.o: warning: objtool: >.altinstr_replacement+0x5a: redundant CLAC > ... >arch/x86/entry/entry_64.o: warning: objtool: >.altinstr_replacement+0xb1: redundant CLAC > >Also, I realized we don't need special annotations for preempt_count; >preempt_disable() emits a CALL instruction which should readily trigger >the warnings added here. > >The VDSO thing is a bit of a hack, but I couldn't quickly find anything >better. > >Comments? > >--- > arch/x86/include/asm/special_insns.h | 2 ++ > arch/x86/kernel/ptrace.c | 3 +- > include/linux/frame.h | 23 ++++++++++++++ > tools/objtool/arch.h | 4 ++- > tools/objtool/arch/x86/decode.c | 14 ++++++++- >tools/objtool/check.c | 59 >++++++++++++++++++++++++++++++++++++ > tools/objtool/check.h | 3 +- > tools/objtool/elf.h | 1 + > 8 files changed, 105 insertions(+), 4 deletions(-) > >diff --git a/arch/x86/include/asm/special_insns.h >b/arch/x86/include/asm/special_insns.h >index 43c029cdc3fe..cd31e4433f4c 100644 >--- a/arch/x86/include/asm/special_insns.h >+++ b/arch/x86/include/asm/special_insns.h >@@ -5,6 +5,7 @@ > > #ifdef __KERNEL__ > >+#include <linux/frame.h> > #include <asm/nops.h> > > /* >@@ -135,6 +136,7 @@ static inline void native_wbinvd(void) > } > > extern asmlinkage void native_load_gs_index(unsigned); >+AC_SAFE(native_load_gs_index); > > static inline unsigned long __read_cr4(void) > { >diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c >index 4b8ee05dd6ad..e278b4055a8b 100644 >--- a/arch/x86/kernel/ptrace.c >+++ b/arch/x86/kernel/ptrace.c >@@ -420,7 +420,7 @@ static int putreg(struct task_struct *child, > return 0; > } > >-static unsigned long getreg(struct task_struct *task, unsigned long >offset) >+static notrace unsigned long getreg(struct task_struct *task, unsigned >long offset) > { > switch (offset) { > case offsetof(struct user_regs_struct, cs): >@@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct >*task, unsigned long offset) > > return *pt_regs_access(task_pt_regs(task), offset); > } >+AC_SAFE(getreg); > > static int genregs_get(struct task_struct *target, > const struct user_regset *regset, >diff --git a/include/linux/frame.h b/include/linux/frame.h >index 02d3ca2d9598..5d354cf42a56 100644 >--- a/include/linux/frame.h >+++ b/include/linux/frame.h >@@ -21,4 +21,27 @@ > > #endif /* CONFIG_STACK_VALIDATION */ > >+#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && >!defined(BUILD_VDSO32) >+/* >+ * This macro marks functions as AC-safe, that is, it is safe to call >from an >+ * EFLAGS.AC enabled region (typically user_access_begin() / >+ * user_access_end()). >+ * >+ * These functions in turn will only call AC-safe functions themselves >(which >+ * precludes tracing, including __fentry__ and scheduling, including >+ * preempt_enable). >+ * >+ * AC-safe functions will obviously also not change EFLAGS.AC >themselves. >+ * >+ * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO >builds >+ * (and the generated symbol reference will in fact cause link >failures). >+ */ >+#define AC_SAFE(func) \ >+ static void __used __section(.discard.ac_safe) \ >+ *__func_ac_safe_##func = func >+ >+#else >+#define AC_SAFE(func) >+#endif >+ > #endif /* _LINUX_FRAME_H */ >diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h >index b0d7dc3d71b5..48327099466d 100644 >--- a/tools/objtool/arch.h >+++ b/tools/objtool/arch.h >@@ -33,7 +33,9 @@ > #define INSN_STACK 8 > #define INSN_BUG 9 > #define INSN_NOP 10 >-#define INSN_OTHER 11 >+#define INSN_STAC 11 >+#define INSN_CLAC 12 >+#define INSN_OTHER 13 > #define INSN_LAST INSN_OTHER > > enum op_dest_type { >diff --git a/tools/objtool/arch/x86/decode.c >b/tools/objtool/arch/x86/decode.c >index 540a209b78ab..d1e99d1460a5 100644 >--- a/tools/objtool/arch/x86/decode.c >+++ b/tools/objtool/arch/x86/decode.c >@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, >struct section *sec, > > case 0x0f: > >- if (op2 >= 0x80 && op2 <= 0x8f) { >+ if (op2 == 0x01) { >+ >+ if (modrm == 0xca) { >+ >+ *type = INSN_CLAC; >+ >+ } else if (modrm == 0xcb) { >+ >+ *type = INSN_STAC; >+ >+ } >+ >+ } else if (op2 >= 0x80 && op2 <= 0x8f) { > > *type = INSN_JUMP_CONDITIONAL; > >diff --git a/tools/objtool/check.c b/tools/objtool/check.c >index 0414a0d52262..01852602ca31 100644 >--- a/tools/objtool/check.c >+++ b/tools/objtool/check.c >@@ -127,6 +127,24 @@ static bool ignore_func(struct objtool_file *file, >struct symbol *func) > return false; > } > >+static bool ac_safe_func(struct objtool_file *file, struct symbol >*func) >+{ >+ struct rela *rela; >+ >+ /* check for AC_SAFE */ >+ if (file->ac_safe && file->ac_safe->rela) >+ list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) { >+ if (rela->sym->type == STT_SECTION && >+ rela->sym->sec == func->sec && >+ rela->addend == func->offset) >+ return true; >+ if (/* rela->sym->type == STT_FUNC && */ rela->sym == func) >+ return true; >+ } >+ >+ return false; >+} >+ > /* > * This checks to see if the given function is a "noreturn" function. > * >@@ -439,6 +457,8 @@ static void add_ignores(struct objtool_file *file) > > for_each_sec(file, sec) { > list_for_each_entry(func, &sec->symbol_list, list) { >+ func->ac_safe = ac_safe_func(file, func); >+ > if (func->type != STT_FUNC) > continue; > >@@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file >*file, struct instruction *first, > switch (insn->type) { > > case INSN_RETURN: >+ if (state.ac) { >+ WARN_FUNC("return with AC set", sec, insn->offset); >+ return 1; >+ } >+ > if (func && has_modified_stack_frame(&state)) { > WARN_FUNC("return with modified stack frame", > sec, insn->offset); >@@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file >*file, struct instruction *first, > return 0; > > case INSN_CALL: >+ if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { >+ WARN_FUNC("call to %s() with AC set", sec, insn->offset, >+ insn->call_dest->name); >+ return 1; >+ } >+ > if (is_fentry_call(insn)) > break; > >@@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file >*file, struct instruction *first, > > /* fallthrough */ > case INSN_CALL_DYNAMIC: >+ if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { >+ WARN_FUNC("call to %s() with AC set", sec, insn->offset, >+ insn->call_dest->name); >+ return 1; >+ } > if (!no_fp && func && !has_valid_stack_frame(&state)) { > WARN_FUNC("call without frame pointer save/setup", > sec, insn->offset); >@@ -1980,6 +2016,26 @@ static int validate_branch(struct objtool_file >*file, struct instruction *first, > > break; > >+ case INSN_STAC: >+ if (state.ac_safe || state.ac) { >+ WARN_FUNC("recursive STAC", sec, insn->offset); >+ return 1; >+ } >+ state.ac = true; >+ break; >+ >+ case INSN_CLAC: >+ if (!state.ac) { >+ WARN_FUNC("redundant CLAC", sec, insn->offset); >+ return 1; >+ } >+ if (state.ac_safe) { >+ WARN_FUNC("AC-safe clears AC", sec, insn->offset); >+ return 1; >+ } >+ state.ac = false; >+ break; >+ > default: > break; > } >@@ -2141,6 +2197,8 @@ static int validate_functions(struct objtool_file >*file) > if (!insn || insn->ignore) > continue; > >+ state.ac_safe = func->ac_safe; >+ > ret = validate_branch(file, insn, state); > warnings += ret; > } >@@ -2198,6 +2256,7 @@ int check(const char *_objname, bool orc) > INIT_LIST_HEAD(&file.insn_list); > hash_init(file.insn_hash); > file.whitelist = find_section_by_name(file.elf, >".discard.func_stack_frame_non_standard"); >+ file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe"); > file.c_file = find_section_by_name(file.elf, ".comment"); > file.ignore_unreachables = no_unreachable; > file.hints = false; >diff --git a/tools/objtool/check.h b/tools/objtool/check.h >index e6e8a655b556..c31ec3ca78f3 100644 >--- a/tools/objtool/check.h >+++ b/tools/objtool/check.h >@@ -31,7 +31,7 @@ struct insn_state { > int stack_size; > unsigned char type; > bool bp_scratch; >- bool drap, end; >+ bool drap, end, ac, ac_safe; > int drap_reg, drap_offset; > struct cfi_reg vals[CFI_NUM_REGS]; > }; >@@ -61,6 +61,7 @@ struct objtool_file { > struct list_head insn_list; > DECLARE_HASHTABLE(insn_hash, 16); > struct section *whitelist; >+ struct section *ac_safe; > bool ignore_unreachables, c_file, hints, rodata; > }; > >diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h >index bc97ed86b9cd..064c3df31e40 100644 >--- a/tools/objtool/elf.h >+++ b/tools/objtool/elf.h >@@ -62,6 +62,7 @@ struct symbol { > unsigned long offset; > unsigned int len; > struct symbol *pfunc, *cfunc; >+ bool ac_safe; > }; > > struct rela { I like it. Objtool could also detect CLAC-STAC or STAC-CLAC sequences without memory operations and remove them; don't know how often that happens, but I know it *does* happen.
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote: > > > But correct :) > > > I agree, that a function which is doing the actual copy should be callable, > > but random other functions? NO! > > So find the below patch -- which spotted fail in ptrace.c > > It has an AC_SAFE(func) annotation which allows marking specific > functions as safe to call. The patch includes 2 instances which were > required to make arch/x86 'build': > > arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set > arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set > > It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the > lack of notrace annotations on functions marked AC_SAFE(): > > arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set > > It builds arch/x86 relatively clean; it only complains about some > redundant CLACs in entry_64.S because it doesn't understand interrupts > and I've not bothered creating an annotation for them yet. > > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC > ... > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC Does objtool understand altinstr? If it understands that this is an altinstr associated with a not-actually-a-fuction (i.e. END not ENDPROC) piece of code, it could suppress this warning. > > -static unsigned long getreg(struct task_struct *task, unsigned long offset) > +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset) > { > switch (offset) { > case offsetof(struct user_regs_struct, cs): > @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset) > > return *pt_regs_access(task_pt_regs(task), offset); > } > +AC_SAFE(getreg); This takes the address and could plausibly suppress some optimizations. > > +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32) > +/* > + * This macro marks functions as AC-safe, that is, it is safe to call from an > + * EFLAGS.AC enabled region (typically user_access_begin() / > + * user_access_end()). > + * > + * These functions in turn will only call AC-safe functions themselves (which > + * precludes tracing, including __fentry__ and scheduling, including > + * preempt_enable). > + * > + * AC-safe functions will obviously also not change EFLAGS.AC themselves. > + * > + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds > + * (and the generated symbol reference will in fact cause link failures). > + */ > +#define AC_SAFE(func) \ > + static void __used __section(.discard.ac_safe) \ > + *__func_ac_safe_##func = func Are we okay with annotating function *declarations* in a way that their addresses get taken whenever the declaration is processed? It would be nice if we could avoid this. I'm wondering if we can just change the code that does getreg() and load_gs_index() so it doesn't do it with AC set. Also, what about paravirt kernels? They'll call into PV code for load_gs_index() with AC set. --Andy
[mailing lists removed because this is a potentially large source of exploits] On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote: > > > But correct :) > > > I agree, that a function which is doing the actual copy should be callable, > > but random other functions? NO! > > So find the below patch -- which spotted fail in ptrace.c > Um, wait a moment. You didn't find an oddity in ptrace.c. You found a giant freaking error in uaccess.h. Am I missing something? How are there not zillions of instances of this that your patch ought to catch? Or is genregs_get() really the only example? diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 780f2b42c8ef..df0571a07b55 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -431,8 +431,10 @@ do { \ ({ \ __label__ __pu_label; \ int __pu_err = -EFAULT; \ + __typeof__(*(ptr)) __pu_val; \ + __pu_val = x; \ __uaccess_begin(); \ - __put_user_size((x), (ptr), (size), __pu_label); \ + __put_user_size(__pu_val, (ptr), (size), __pu_label); \ __pu_err = 0; \ __pu_label: \ __uaccess_end(); \
On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski <luto@kernel.org> wrote: > > [mailing lists removed because this is a potentially large source of exploits] I think you're overly worried. AC doesn't protect against "large source of exploits". If it did, then all CPU's before Broadwell would be insecure. They aren't. They'd better not be, considering that there's a _lot_ of Xeon machines out there based on older microarchitectures. I think some of them might even be reasonably current (eg Xeon E7v3 isn't _that_ old, and is Haswell-based, and doesn't have SMAP afaik). SMAP is a great debugging and development aid, and makes sure that developers - who hopefully run primarily on modern platforms - don't write code that just accesses user space directly (because with SMAP, it won't work). And yes, SMAP can limit the effect of kernel bugs, and turn something that would otherwise be a security issue into "just a bug". But running with AC on isn't a security issue in itself - it just makes SMAP slightly less powerful. The biggest issue of the whole "AC doesn't get saved/redstored" is actually the *reverse* case, where a preemption event could then cause a process that had AC on to be scheduled away, then AC would stay on for some time, but then we might schedule back with AC _clear_, and now you'd have a non-working user access, and a possible DoS attack as a result because you returned EFAULT to a system call that was perfectly fine. See? It's not so much "AC stays on" that is a "sky is falling" issue, it's actually "AC also gets turned off randomly" that actually has real and immediate effects. "AC on" is unfortunate and not great, don't get me wrong, but it's definitely not the end of the world. Particularly not for short sequences. That said: > Um, wait a moment. You didn't find an oddity in ptrace.c. You found > a giant freaking error in uaccess.h. I agree that your patch is good, and should be applied. Mind writing up a changelog and committing it to -tip? > Am I missing something? How are there not zillions of instances of > this that your patch ought to catch? Or is genregs_get() really the > only example? I really do think that it's very unusual to do "get/put_user()" with complicated value arguments. So while your patch is obviously the right thing to do, I really don't think this is a huge worry, or all _that_ surprising that this issue apparently found just a single case of a function call. With all that said, I didn't even react to this part of PeterZ's patch, but it's a good call, and I think it's also a great validation of the objtool approach to validating AC. So cheers for that! Linus
On Fri, Feb 22, 2019 at 5:12 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, Feb 22, 2019 at 4:34 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > [mailing lists removed because this is a potentially large source of exploits] > > I think you're overly worried. > > AC doesn't protect against "large source of exploits". What I meant was: a potentially large grab bag of ways to help turn a bug into an exploit. > That said: > > > Um, wait a moment. You didn't find an oddity in ptrace.c. You found > > a giant freaking error in uaccess.h. > > I agree that your patch is good, and should be applied. Mind writing > up a changelog and committing it to -tip? I don't have commit access :) But I shall email it out once I test it a bit. --Andy
On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski <luto@kernel.org> wrote: > > What I meant was: a potentially large grab bag of ways to help turn a > bug into an exploit. Well, apparently there's only one place, and that one looks pretty harmless. Of course, I don't think PeterZ said what his config was. Maybe it was a very minimal config and only found that one case because all the truly scary cases were configured out ;) Linus
On Fri, Feb 22, 2019 at 5:17 PM Andy Lutomirski <luto@kernel.org> wrote: > > I don't have commit access :) But I shall email it out once I test it a bit. Btw, looking at it, with that change every user of __put_user_size() now passes in the properly type-cast value in 'x'. Which means that you could remove the odd typecast in the 8-byte case: case 8: \ __put_user_goto_u64((__typeof__(*ptr))(x), ptr, label); \ and make it match all the other cases: case 8: \ __put_user_goto_u64(x, ptr, label); \ but that's just from looking at the patch, and maybe I'm missing something. Linus
On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote: > On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@infradead.org> wrote: > > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC > > Does objtool understand altinstr? Yep, otherwise it would've never found any of the STAC/CLAC instructions to begin with, they're all alternatives. The emitted code is all NOPs. > If it understands that this is an > altinstr associated with a not-actually-a-fuction (i.e. END not > ENDPROC) piece of code, it could suppress this warning. Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for these 'functions', so yes, I can try and ignore this warning for those. > > +#define AC_SAFE(func) \ > > + static void __used __section(.discard.ac_safe) \ > > + *__func_ac_safe_##func = func > > Are we okay with annotating function *declarations* in a way that > their addresses get taken whenever the declaration is processed? It > would be nice if we could avoid this. > > I'm wondering if we can just change the code that does getreg() and > load_gs_index() so it doesn't do it with AC set. Also, what about > paravirt kernels? They'll call into PV code for load_gs_index() with > AC set. Fixing that code would be fine; but we need that annotation regardless, read Linus' email a little further back, he wants things like copy_{to,from}_user_unsafe(). Those really would need to be marked AC-safe, there's no inlining that. That said, yes these annotations _suck_. But it's what we had and works, I'm just copying it (from STACK_FRAME_NON_STANDARD). That said; maybe we can do something like: #define AC_SAFE(func) \ asm (".pushsection .discard.ac_safe_sym\n\t" \ "999: .ascii \"" #func "\"\n\t" \ ".popsection\n\t" \ ".pushsection .discard.ac_safe\n\t" \ _ASM_PTR " 999b\n\t" \ ".popsection") I just don't have a clue on how to read that in objtool; weak elf foo. I'll see if I can make it work.
On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote: > Objtool could also detect CLAC-STAC or STAC-CLAC sequences without > memory operations and remove them; don't know how often that happens, > but I know it *does* happen. Objtool doesn't know about memops; that'd be a lot of work. Also, objtool doesn't actually rewrite the text, at best it could warn about such occurences.
On Fri, Feb 22, 2019 at 03:34:00PM -0800, Linus Torvalds wrote:
> Can you make it do DF too while at it?
Sure.
On Sat, Feb 23, 2019 at 09:37:06AM +0100, Peter Zijlstra wrote: > On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote: > > On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC > > > > Does objtool understand altinstr? > > Yep, otherwise it would've never found any of the STAC/CLAC instructions > to begin with, they're all alternatives. The emitted code is all NOPs. > > > If it understands that this is an > > altinstr associated with a not-actually-a-fuction (i.e. END not > > ENDPROC) piece of code, it could suppress this warning. > > Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for > these 'functions', so yes, I can try and ignore this warning for those. > > > > +#define AC_SAFE(func) \ > > > + static void __used __section(.discard.ac_safe) \ > > > + *__func_ac_safe_##func = func > > > > Are we okay with annotating function *declarations* in a way that > > their addresses get taken whenever the declaration is processed? It > > would be nice if we could avoid this. > > > > I'm wondering if we can just change the code that does getreg() and > > load_gs_index() so it doesn't do it with AC set. Also, what about > > paravirt kernels? They'll call into PV code for load_gs_index() with > > AC set. > > Fixing that code would be fine; but we need that annotation regardless, > read Linus' email a little further back, he wants things like > copy_{to,from}_user_unsafe(). > > Those really would need to be marked AC-safe, there's no inlining that. > > That said, yes these annotations _suck_. But it's what we had and works, > I'm just copying it (from STACK_FRAME_NON_STANDARD). > > That said; maybe we can do something like: > > #define AC_SAFE(func) \ > asm (".pushsection .discard.ac_safe_sym\n\t" \ > "999: .ascii \"" #func "\"\n\t" \ > ".popsection\n\t" \ > ".pushsection .discard.ac_safe\n\t" \ > _ASM_PTR " 999b\n\t" \ > ".popsection") > > I just don't have a clue on how to read that in objtool; weak elf foo. > I'll see if I can make it work. Fixed all that. Should probably also convert that NON_STANDARD stuff to this new shiny thing. Retained the ptrace muck because it's a nice test case, your patch is obviously a better fix there. Haven't bothered looking at alternatives yet, this is a defconfig+tracing build. Weekend now. --- arch/x86/include/asm/special_insns.h | 2 + arch/x86/kernel/ptrace.c | 3 +- include/linux/frame.h | 38 ++++++++++++ tools/objtool/Makefile | 2 +- tools/objtool/arch.h | 6 +- tools/objtool/arch/x86/decode.c | 22 ++++++- tools/objtool/check.c | 117 ++++++++++++++++++++++++++++++++++- tools/objtool/check.h | 2 +- tools/objtool/elf.h | 1 + 9 files changed, 187 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 43c029cdc3fe..cd31e4433f4c 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -5,6 +5,7 @@ #ifdef __KERNEL__ +#include <linux/frame.h> #include <asm/nops.h> /* @@ -135,6 +136,7 @@ static inline void native_wbinvd(void) } extern asmlinkage void native_load_gs_index(unsigned); +AC_SAFE(native_load_gs_index); static inline unsigned long __read_cr4(void) { diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index 4b8ee05dd6ad..e278b4055a8b 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -420,7 +420,7 @@ static int putreg(struct task_struct *child, return 0; } -static unsigned long getreg(struct task_struct *task, unsigned long offset) +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset) { switch (offset) { case offsetof(struct user_regs_struct, cs): @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset) return *pt_regs_access(task_pt_regs(task), offset); } +AC_SAFE(getreg); static int genregs_get(struct task_struct *target, const struct user_regset *regset, diff --git a/include/linux/frame.h b/include/linux/frame.h index 02d3ca2d9598..870a894bc586 100644 --- a/include/linux/frame.h +++ b/include/linux/frame.h @@ -21,4 +21,42 @@ #endif /* CONFIG_STACK_VALIDATION */ +#if defined(CONFIG_STACK_VALIDATION) +/* + * This macro marks functions as AC-safe, that is, it is safe to call from an + * EFLAGS.AC enabled region (typically user_access_begin() / + * user_access_end()). + * + * These functions in turn will only call AC-safe functions themselves (which + * precludes tracing, including __fentry__ and scheduling, including + * preempt_enable). + * + * AC-safe functions will obviously also not change EFLAGS.AC themselves. + */ +#define AC_SAFE(func) \ + asm (".pushsection .discard.ac_safe_sym, \"S\", @3\n\t" \ + "999: .ascii \"" #func "\"\n\t" \ + " .byte 0\n\t" \ + ".popsection\n\t" \ + ".pushsection .discard.ac_safe\n\t" \ + ".long 999b - .\n\t" \ + ".popsection") + +/* + * SHT_STRTAB sayeth: + * + * - The initial byte of a string table is \0. This allows an string offset + * value of zero to denote the NULL string. + * + * Works just fine without this, but since we set the proper section type, lets + * not confuse anybody reading this. + */ +asm(".pushsection .discard.ac_safe_sym, \"S\", @3\n\t" + ".byte 0\n\t" + ".popsection\n\t"); + +#else +#define AC_SAFE(func) +#endif + #endif /* _LINUX_FRAME_H */ diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile index c9d038f91af6..5a64e5fb9d7a 100644 --- a/tools/objtool/Makefile +++ b/tools/objtool/Makefile @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \ -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \ -I$(srctree)/tools/objtool/arch/$(ARCH)/include WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -CFLAGS += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) +CFLAGS += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) -ggdb3 LDFLAGS += -lelf $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) # Allow old libelf to be used: diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index b0d7dc3d71b5..9795d83cbc01 100644 --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -33,7 +33,11 @@ #define INSN_STACK 8 #define INSN_BUG 9 #define INSN_NOP 10 -#define INSN_OTHER 11 +#define INSN_STAC 11 +#define INSN_CLAC 12 +#define INSN_STD 13 +#define INSN_CLD 14 +#define INSN_OTHER 15 #define INSN_LAST INSN_OTHER enum op_dest_type { diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index 540a209b78ab..bee32ad53ecf 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, case 0x0f: - if (op2 >= 0x80 && op2 <= 0x8f) { + if (op2 == 0x01) { + + if (modrm == 0xca) { + + *type = INSN_CLAC; + + } else if (modrm == 0xcb) { + + *type = INSN_STAC; + + } + + } else if (op2 >= 0x80 && op2 <= 0x8f) { *type = INSN_JUMP_CONDITIONAL; @@ -444,6 +456,14 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, *type = INSN_CALL; break; + case 0xfc: + *type = INSN_CLD; + break; + + case 0xfd: + *type = INSN_STD; + break; + case 0xff: if (modrm_reg == 2 || modrm_reg == 3) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 0414a0d52262..3dedfa19cb09 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -451,6 +451,41 @@ static void add_ignores(struct objtool_file *file) } } +static int add_ac_safe(struct objtool_file *file) +{ + struct section *sec_sym, *sec; + struct symbol *func; + struct rela *rela; + const char *name; + + sec = find_section_by_name(file->elf, ".rela.discard.ac_safe"); + if (!sec) + return 0; + + sec_sym = find_section_by_name(file->elf, ".discard.ac_safe_sym"); + if (!sec_sym) { + WARN("missing AC-safe symbols"); + return -1; + } + + list_for_each_entry(rela, &sec->rela_list, list) { + if (rela->sym->type != STT_SECTION) { + WARN("unexpected relocation symbol type in %s", sec->name); + return -1; + } + + name = elf_strptr(file->elf->elf, sec_sym->idx, rela->addend); + + func = find_symbol_by_name(file->elf, name); + if (!func) + continue; + + func->ac_safe = true; + } + + return 0; +} + /* * FIXME: For now, just ignore any alternatives which add retpolines. This is * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline. @@ -695,6 +730,7 @@ static int handle_group_alt(struct objtool_file *file, last_new_insn = insn; insn->ignore = orig_insn->ignore_alts; + insn->func = orig_insn->func; if (insn->type != INSN_JUMP_CONDITIONAL && insn->type != INSN_JUMP_UNCONDITIONAL) @@ -1239,6 +1275,10 @@ static int decode_sections(struct objtool_file *file) add_ignores(file); + ret = add_ac_safe(file); + if (ret) + return ret; + ret = add_nospec_ignores(file); if (ret) return ret; @@ -1902,6 +1942,15 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, switch (insn->type) { case INSN_RETURN: + if (state.ac) { + WARN_FUNC("return with AC set", sec, insn->offset); + return 1; + } + if (state.df) { + WARN_FUNC("return with DF set", sec, insn->offset); + return 1; + } + if (func && has_modified_stack_frame(&state)) { WARN_FUNC("return with modified stack frame", sec, insn->offset); @@ -1917,6 +1966,17 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, return 0; case INSN_CALL: + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { + WARN_FUNC("call to %s() with AC set", sec, insn->offset, + insn->call_dest->name); + return 1; + } + if (state.df) { + WARN_FUNC("call to %s() with DF set", sec, insn->offset, + insn->call_dest->name); + return 1; + } + if (is_fentry_call(insn)) break; @@ -1926,8 +1986,25 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, if (ret == -1) return 1; - /* fallthrough */ + if (!no_fp && func && !has_valid_stack_frame(&state)) { + WARN_FUNC("call without frame pointer save/setup", + sec, insn->offset); + return 1; + } + break; + case INSN_CALL_DYNAMIC: + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { + WARN_FUNC("call to %s() with AC set", sec, insn->offset, + insn->call_dest->name); + return 1; + } + if (state.df) { + WARN_FUNC("call to %s() with DF set", sec, insn->offset, + insn->call_dest->name); + return 1; + } + if (!no_fp && func && !has_valid_stack_frame(&state)) { WARN_FUNC("call without frame pointer save/setup", sec, insn->offset); @@ -1980,6 +2057,42 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, break; + case INSN_STAC: + if (state.ac_safe || state.ac) { + WARN_FUNC("recursive STAC", sec, insn->offset); + return 1; + } + state.ac = true; + break; + + case INSN_CLAC: + if (!state.ac && insn->func) { + WARN_FUNC("redundant CLAC", sec, insn->offset); + return 1; + } + if (state.ac_safe) { + WARN_FUNC("AC-safe clears AC", sec, insn->offset); + return 1; + } + state.ac = false; + break; + + case INSN_STD: + if (state.df) { + WARN_FUNC("recursive STD", sec, insn->offset); + return 1; + } + state.df = true; + break; + + case INSN_CLD: + if (!state.df && insn->func) { + WARN_FUNC("redundant CLD", sec, insn->offset); + return 1; + } + state.df = false; + break; + default: break; } @@ -2141,6 +2254,8 @@ static int validate_functions(struct objtool_file *file) if (!insn || insn->ignore) continue; + state.ac_safe = func->ac_safe; + ret = validate_branch(file, insn, state); warnings += ret; } diff --git a/tools/objtool/check.h b/tools/objtool/check.h index e6e8a655b556..602634792151 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -31,7 +31,7 @@ struct insn_state { int stack_size; unsigned char type; bool bp_scratch; - bool drap, end; + bool drap, end, ac, ac_safe, df; int drap_reg, drap_offset; struct cfi_reg vals[CFI_NUM_REGS]; }; diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h index bc97ed86b9cd..064c3df31e40 100644 --- a/tools/objtool/elf.h +++ b/tools/objtool/elf.h @@ -62,6 +62,7 @@ struct symbol { unsigned long offset; unsigned int len; struct symbol *pfunc, *cfunc; + bool ac_safe; }; struct rela {
On 22/02/2019 22:26, Peter Zijlstra wrote: > On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote: > >> But correct :) > >> I agree, that a function which is doing the actual copy should be callable, >> but random other functions? NO! > > So find the below patch -- which spotted fail in ptrace.c > > It has an AC_SAFE(func) annotation which allows marking specific > functions as safe to call. The patch includes 2 instances which were > required to make arch/x86 'build': > > arch/x86/ia32/ia32_signal.o: warning: objtool: ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set > arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to getreg() with AC set > > It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the > lack of notrace annotations on functions marked AC_SAFE(): > > arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to __fentry__() with AC set > > It builds arch/x86 relatively clean; it only complains about some > redundant CLACs in entry_64.S because it doesn't understand interrupts > and I've not bothered creating an annotation for them yet. > > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d: redundant CLAC > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a: redundant CLAC > ... > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC > > Also, I realized we don't need special annotations for preempt_count; > preempt_disable() emits a CALL instruction which should readily trigger > the warnings added here. > > The VDSO thing is a bit of a hack, but I couldn't quickly find anything > better. > > Comments? I haven't looked at all the details. But could the annotation be called UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since this is not an x86 only issue and the AC flags only exists for x86. Cheers, Julien > > --- > arch/x86/include/asm/special_insns.h | 2 ++ > arch/x86/kernel/ptrace.c | 3 +- > include/linux/frame.h | 23 ++++++++++++++ > tools/objtool/arch.h | 4 ++- > tools/objtool/arch/x86/decode.c | 14 ++++++++- > tools/objtool/check.c | 59 ++++++++++++++++++++++++++++++++++++ > tools/objtool/check.h | 3 +- > tools/objtool/elf.h | 1 + > 8 files changed, 105 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h > index 43c029cdc3fe..cd31e4433f4c 100644 > --- a/arch/x86/include/asm/special_insns.h > +++ b/arch/x86/include/asm/special_insns.h > @@ -5,6 +5,7 @@ > > #ifdef __KERNEL__ > > +#include <linux/frame.h> > #include <asm/nops.h> > > /* > @@ -135,6 +136,7 @@ static inline void native_wbinvd(void) > } > > extern asmlinkage void native_load_gs_index(unsigned); > +AC_SAFE(native_load_gs_index); > > static inline unsigned long __read_cr4(void) > { > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c > index 4b8ee05dd6ad..e278b4055a8b 100644 > --- a/arch/x86/kernel/ptrace.c > +++ b/arch/x86/kernel/ptrace.c > @@ -420,7 +420,7 @@ static int putreg(struct task_struct *child, > return 0; > } > > -static unsigned long getreg(struct task_struct *task, unsigned long offset) > +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset) > { > switch (offset) { > case offsetof(struct user_regs_struct, cs): > @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset) > > return *pt_regs_access(task_pt_regs(task), offset); > } > +AC_SAFE(getreg); > > static int genregs_get(struct task_struct *target, > const struct user_regset *regset, > diff --git a/include/linux/frame.h b/include/linux/frame.h > index 02d3ca2d9598..5d354cf42a56 100644 > --- a/include/linux/frame.h > +++ b/include/linux/frame.h > @@ -21,4 +21,27 @@ > > #endif /* CONFIG_STACK_VALIDATION */ > > +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32) > +/* > + * This macro marks functions as AC-safe, that is, it is safe to call from an > + * EFLAGS.AC enabled region (typically user_access_begin() / > + * user_access_end()). > + * > + * These functions in turn will only call AC-safe functions themselves (which > + * precludes tracing, including __fentry__ and scheduling, including > + * preempt_enable). > + * > + * AC-safe functions will obviously also not change EFLAGS.AC themselves. > + * > + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds > + * (and the generated symbol reference will in fact cause link failures). > + */ > +#define AC_SAFE(func) \ > + static void __used __section(.discard.ac_safe) \ > + *__func_ac_safe_##func = func > + > +#else > +#define AC_SAFE(func) > +#endif > + > #endif /* _LINUX_FRAME_H */ > diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h > index b0d7dc3d71b5..48327099466d 100644 > --- a/tools/objtool/arch.h > +++ b/tools/objtool/arch.h > @@ -33,7 +33,9 @@ > #define INSN_STACK 8 > #define INSN_BUG 9 > #define INSN_NOP 10 > -#define INSN_OTHER 11 > +#define INSN_STAC 11 > +#define INSN_CLAC 12 > +#define INSN_OTHER 13 > #define INSN_LAST INSN_OTHER > > enum op_dest_type { > diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c > index 540a209b78ab..d1e99d1460a5 100644 > --- a/tools/objtool/arch/x86/decode.c > +++ b/tools/objtool/arch/x86/decode.c > @@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, > > case 0x0f: > > - if (op2 >= 0x80 && op2 <= 0x8f) { > + if (op2 == 0x01) { > + > + if (modrm == 0xca) { > + > + *type = INSN_CLAC; > + > + } else if (modrm == 0xcb) { > + > + *type = INSN_STAC; > + > + } > + > + } else if (op2 >= 0x80 && op2 <= 0x8f) { > > *type = INSN_JUMP_CONDITIONAL; > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 0414a0d52262..01852602ca31 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -127,6 +127,24 @@ static bool ignore_func(struct objtool_file *file, struct symbol *func) > return false; > } > > +static bool ac_safe_func(struct objtool_file *file, struct symbol *func) > +{ > + struct rela *rela; > + > + /* check for AC_SAFE */ > + if (file->ac_safe && file->ac_safe->rela) > + list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) { > + if (rela->sym->type == STT_SECTION && > + rela->sym->sec == func->sec && > + rela->addend == func->offset) > + return true; > + if (/* rela->sym->type == STT_FUNC && */ rela->sym == func) > + return true; > + } > + > + return false; > +} > + > /* > * This checks to see if the given function is a "noreturn" function. > * > @@ -439,6 +457,8 @@ static void add_ignores(struct objtool_file *file) > > for_each_sec(file, sec) { > list_for_each_entry(func, &sec->symbol_list, list) { > + func->ac_safe = ac_safe_func(file, func); > + > if (func->type != STT_FUNC) > continue; > > @@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, > switch (insn->type) { > > case INSN_RETURN: > + if (state.ac) { > + WARN_FUNC("return with AC set", sec, insn->offset); > + return 1; > + } > + > if (func && has_modified_stack_frame(&state)) { > WARN_FUNC("return with modified stack frame", > sec, insn->offset); > @@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, > return 0; > > case INSN_CALL: > + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { > + WARN_FUNC("call to %s() with AC set", sec, insn->offset, > + insn->call_dest->name); > + return 1; > + } > + > if (is_fentry_call(insn)) > break; > > @@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, > > /* fallthrough */ > case INSN_CALL_DYNAMIC: > + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { > + WARN_FUNC("call to %s() with AC set", sec, insn->offset, > + insn->call_dest->name); > + return 1; > + } > if (!no_fp && func && !has_valid_stack_frame(&state)) { > WARN_FUNC("call without frame pointer save/setup", > sec, insn->offset); > @@ -1980,6 +2016,26 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, > > break; > > + case INSN_STAC: > + if (state.ac_safe || state.ac) { > + WARN_FUNC("recursive STAC", sec, insn->offset); > + return 1; > + } > + state.ac = true; > + break; > + > + case INSN_CLAC: > + if (!state.ac) { > + WARN_FUNC("redundant CLAC", sec, insn->offset); > + return 1; > + } > + if (state.ac_safe) { > + WARN_FUNC("AC-safe clears AC", sec, insn->offset); > + return 1; > + } > + state.ac = false; > + break; > + > default: > break; > } > @@ -2141,6 +2197,8 @@ static int validate_functions(struct objtool_file *file) > if (!insn || insn->ignore) > continue; > > + state.ac_safe = func->ac_safe; > + > ret = validate_branch(file, insn, state); > warnings += ret; > } > @@ -2198,6 +2256,7 @@ int check(const char *_objname, bool orc) > INIT_LIST_HEAD(&file.insn_list); > hash_init(file.insn_hash); > file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard"); > + file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe"); > file.c_file = find_section_by_name(file.elf, ".comment"); > file.ignore_unreachables = no_unreachable; > file.hints = false; > diff --git a/tools/objtool/check.h b/tools/objtool/check.h > index e6e8a655b556..c31ec3ca78f3 100644 > --- a/tools/objtool/check.h > +++ b/tools/objtool/check.h > @@ -31,7 +31,7 @@ struct insn_state { > int stack_size; > unsigned char type; > bool bp_scratch; > - bool drap, end; > + bool drap, end, ac, ac_safe; > int drap_reg, drap_offset; > struct cfi_reg vals[CFI_NUM_REGS]; > }; > @@ -61,6 +61,7 @@ struct objtool_file { > struct list_head insn_list; > DECLARE_HASHTABLE(insn_hash, 16); > struct section *whitelist; > + struct section *ac_safe; > bool ignore_unreachables, c_file, hints, rodata; > }; > > diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h > index bc97ed86b9cd..064c3df31e40 100644 > --- a/tools/objtool/elf.h > +++ b/tools/objtool/elf.h > @@ -62,6 +62,7 @@ struct symbol { > unsigned long offset; > unsigned int len; > struct symbol *pfunc, *cfunc; > + bool ac_safe; > }; > > struct rela { >
On February 23, 2019 12:39:42 AM PST, Peter Zijlstra <peterz@infradead.org> wrote: >On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote: >> Objtool could also detect CLAC-STAC or STAC-CLAC sequences without >> memory operations and remove them; don't know how often that happens, >> but I know it *does* happen. > >Objtool doesn't know about memops; that'd be a lot of work. Also, >objtool doesn't actually rewrite the text, at best it could warn about >such occurences. It doesn't have to understand the contents of the memop, but it seems that the presence of a modrm with mode ≠ 3 should be plenty. It needs to know that much in order to know the length of instructions anyway. For extra credit, ignore LEA or hinting instructions.
On February 23, 2019 12:39:42 AM PST, Peter Zijlstra <peterz@infradead.org> wrote: >On Fri, Feb 22, 2019 at 03:39:48PM -0800, hpa@zytor.com wrote: >> Objtool could also detect CLAC-STAC or STAC-CLAC sequences without >> memory operations and remove them; don't know how often that happens, >> but I know it *does* happen. > >Objtool doesn't know about memops; that'd be a lot of work. Also, >objtool doesn't actually rewrite the text, at best it could warn about >such occurences. It doesn't even have to change the text per se, just nullify the altinstr.
On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote: > I'm wondering if we can just change the code that does getreg() and > load_gs_index() so it doesn't do it with AC set. Also, what about > paravirt kernels? They'll call into PV code for load_gs_index() with > AC set. Paravirt can go bugger off. There's no sane way to fix that. Luckily the load_gs_index() thing is part of the paravirt me harder crap and so nobody sane should ever hit that. I don't fully understand that code at all; I also have no clue why GS has paravirt bits on but the other segments do not. But it looks like we want to do that RELOAD_SEG() crap under SMAP because of the GET_SEG() -> get_user_ex() thing. Anyway, I only see 3 options here: 1) live with the paravirt me harder builds complaining 2) exclude the AC validation from the paravirt me harder builds 3) rewrite this code to no need that stupid call in the first place 2 seems like an exceptionally bad ideal, 3 would need someone that understands this, so for now I'll pick 1 :-) *thought*... we could delay the actual set_user_seg() thing until after the get_user_catch(), would that work?
On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote: > On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote: > > I'm wondering if we can just change the code that does getreg() and > > load_gs_index() so it doesn't do it with AC set. Also, what about > > paravirt kernels? They'll call into PV code for load_gs_index() with > > AC set. > > Paravirt can go bugger off. There's no sane way to fix that. > I don't fully understand that code at all; I also have no clue why GS > has paravirt bits on but the other segments do not. *sigh* SWAPGS > *thought*... we could delay the actual set_user_seg() thing until after > the get_user_catch(), would that work? How horrible / broken is this? --- diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c index 321fe5f5d0e9..67c866943102 100644 --- a/arch/x86/ia32/ia32_signal.c +++ b/arch/x86/ia32/ia32_signal.c @@ -60,17 +60,21 @@ regs->seg = GET_SEG(seg) | 3; \ } while (0) -#define RELOAD_SEG(seg) { \ - unsigned int pre = GET_SEG(seg); \ - unsigned int cur = get_user_seg(seg); \ - pre |= 3; \ - if (pre != cur) \ - set_user_seg(seg, pre); \ +#define LOAD_SEG(seg) { \ + pre_##seg = 3 | GET_SEG(seg); \ + cur_##seg = get_user_seg(seg); \ +} + +#define RELOAD_SEG(seg) { \ + if (pre_##seg != cur_##seg) \ + set_user_seg(seg, pre_##seg); \ } static int ia32_restore_sigcontext(struct pt_regs *regs, struct sigcontext_32 __user *sc) { + u16 pre_gs, pre_fs, pre_ds, pre_es; + u16 cur_gs, cur_fs, cur_ds, cur_es; unsigned int tmpflags, err = 0; void __user *buf; u32 tmp; @@ -85,10 +89,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, * the handler, but does not clobber them at least in the * normal case. */ - RELOAD_SEG(gs); - RELOAD_SEG(fs); - RELOAD_SEG(ds); - RELOAD_SEG(es); + LOAD_SEG(gs); + LOAD_SEG(fs); + LOAD_SEG(ds); + LOAD_SEG(es); COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx); COPY(dx); COPY(cx); COPY(ip); COPY(ax); @@ -106,6 +110,11 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, buf = compat_ptr(tmp); } get_user_catch(err); + RELOAD_SEG(gs); + RELOAD_SEG(fs); + RELOAD_SEG(ds); + RELOAD_SEG(es); + err |= fpu__restore_sig(buf, 1); force_iret();
On Mon, Feb 25, 2019 at 08:33:35AM +0000, Julien Thierry wrote: > > It has an AC_SAFE(func) annotation which allows marking specific > > functions as safe to call. The patch includes 2 instances which were > > required to make arch/x86 'build': > I haven't looked at all the details. But could the annotation be called > UACCESS_SAFE() (and corresponding naming in the objtool checks)? Since > this is not an x86 only issue and the AC flags only exists for x86. Sure that works. Lemme sed the patches.
On Mon, Feb 25, 2019 at 12:47:00AM -0800, hpa@zytor.com wrote: > It doesn't have to understand the contents of the memop, but it seems > that the presence of a modrm with mode ≠ 3 should be plenty. It needs > to know that much in order to know the length of instructions anyway. > For extra credit, ignore LEA or hinting instructions. A little something like so then? arch/x86/kernel/fpu/signal.o: warning: objtool: .altinstr_replacement+0x9c: UACCESS disable without MEMOPs: copy_fpstate_to_sigframe() 00000000023c 000200000002 R_X86_64_PC32 0000000000000000 .text + 604 000000000240 000700000002 R_X86_64_PC32 0000000000000000 .altinstr_replacement + 99 000000000249 000200000002 R_X86_64_PC32 0000000000000000 .text + 610 00000000024d 000700000002 R_X86_64_PC32 0000000000000000 .altinstr_replacement + 9c .text 604: 90 nop 605: 90 nop 606: 90 nop 607: 83 ce 03 or $0x3,%esi 60a: 89 b3 00 02 00 00 mov %esi,0x200(%rbx) 610: 90 nop 611: 90 nop 612: 90 nop .altinstr_replacement 99: 0f 01 cb stac 9c: 0f 01 ca clac Which looks like the tool failed to recognise that MOV as a memop. --- --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -37,7 +37,8 @@ #define INSN_CLAC 12 #define INSN_STD 13 #define INSN_CLD 14 -#define INSN_OTHER 15 +#define INSN_MEMOP 15 +#define INSN_OTHER 16 #define INSN_LAST INSN_OTHER enum op_dest_type { --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -123,6 +123,9 @@ int arch_decode_instruction(struct elf * modrm_mod = X86_MODRM_MOD(modrm); modrm_reg = X86_MODRM_REG(modrm); modrm_rm = X86_MODRM_RM(modrm); + + if (modrm_mod != 3) + *type = INSN_MEMOP; } if (insn.sib.nbytes) --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -2047,6 +2047,7 @@ static int validate_branch(struct objtoo WARN_FUNC("recursive UACCESS enable", sec, insn->offset); state.uaccess = true; + state.memop = false; break; case INSN_CLAC: @@ -2058,6 +2059,9 @@ static int validate_branch(struct objtoo break; } + if (!state.memop && insn->func) + WARN_FUNC("UACCESS disable without MEMOPs: %s()", sec, insn->offset, insn->func->name); + state.uaccess = false; break; @@ -2075,6 +2079,10 @@ static int validate_branch(struct objtoo state.df = false; break; + case INSN_MEMOP: + state.memop = true; + break; + default: break; } --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -31,7 +31,7 @@ struct insn_state { int stack_size; unsigned char type; bool bp_scratch; - bool drap, end, uaccess, uaccess_safe, df; + bool drap, end, uaccess, uaccess_safe, df, memop; int drap_reg, drap_offset; struct cfi_reg vals[CFI_NUM_REGS]; };
> On Feb 25, 2019, at 3:53 AM, Peter Zijlstra <peterz@infradead.org> wrote: > >> On Mon, Feb 25, 2019 at 11:51:44AM +0100, Peter Zijlstra wrote: >>> On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote: >>> I'm wondering if we can just change the code that does getreg() and >>> load_gs_index() so it doesn't do it with AC set. Also, what about >>> paravirt kernels? They'll call into PV code for load_gs_index() with >>> AC set. >> >> Paravirt can go bugger off. There's no sane way to fix that. > >> I don't fully understand that code at all; I also have no clue why GS >> has paravirt bits on but the other segments do not. > > *sigh* SWAPGS > >> *thought*... we could delay the actual set_user_seg() thing until after >> the get_user_catch(), would that work? > > > How horrible / broken is this? > > --- > > diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c > index 321fe5f5d0e9..67c866943102 100644 > --- a/arch/x86/ia32/ia32_signal.c > +++ b/arch/x86/ia32/ia32_signal.c > @@ -60,17 +60,21 @@ > regs->seg = GET_SEG(seg) | 3; \ > } while (0) > > -#define RELOAD_SEG(seg) { \ > - unsigned int pre = GET_SEG(seg); \ > - unsigned int cur = get_user_seg(seg); \ > - pre |= 3; \ > - if (pre != cur) \ > - set_user_seg(seg, pre); \ > +#define LOAD_SEG(seg) { \ > + pre_##seg = 3 | GET_SEG(seg); \ > + cur_##seg = get_user_seg(seg); \ > +} > + > +#define RELOAD_SEG(seg) { \ > + if (pre_##seg != cur_##seg) \ > + set_user_seg(seg, pre_##seg); \ > } > > static int ia32_restore_sigcontext(struct pt_regs *regs, > struct sigcontext_32 __user *sc) > { > + u16 pre_gs, pre_fs, pre_ds, pre_es; > + u16 cur_gs, cur_fs, cur_ds, cur_es; > unsigned int tmpflags, err = 0; > void __user *buf; > u32 tmp; > @@ -85,10 +89,10 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, > * the handler, but does not clobber them at least in the > * normal case. > */ > - RELOAD_SEG(gs); > - RELOAD_SEG(fs); > - RELOAD_SEG(ds); > - RELOAD_SEG(es); > + LOAD_SEG(gs); > + LOAD_SEG(fs); > + LOAD_SEG(ds); > + LOAD_SEG(es); > > COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx); > COPY(dx); COPY(cx); COPY(ip); COPY(ax); > @@ -106,6 +110,11 @@ static int ia32_restore_sigcontext(struct pt_regs *regs, > buf = compat_ptr(tmp); > } get_user_catch(err); > > + RELOAD_SEG(gs); > + RELOAD_SEG(fs); > + RELOAD_SEG(ds); > + RELOAD_SEG(es); > + > err |= fpu__restore_sig(buf, 1); > > force_iret(); I would call this pretty horrible. How about we do it without macros? :) But yes, deferring the segment load until after the read seems fine to me. Frankly, we could also just copy_from_user the whole thing up front — thus code is not really a serious fast path.
On Mon, Feb 25, 2019 at 02:21:03PM +0100, Peter Zijlstra wrote: > On Mon, Feb 25, 2019 at 12:47:00AM -0800, hpa@zytor.com wrote: > > It doesn't have to understand the contents of the memop, but it seems > > that the presence of a modrm with mode ≠ 3 should be plenty. It needs > > to know that much in order to know the length of instructions anyway. > > For extra credit, ignore LEA or hinting instructions. > > A little something like so then? $ ./objtool check --no-fp --backtrace ../../defconfig-build/arch/x86/lib/usercopy_64.o ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x3: UACCESS disable without MEMOPs: __clear_user() ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x3a: (alt) ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x2e: (branch) ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x18: (branch) ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch) ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x5: (alt) ../../defconfig-build/arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x0: <=== (func) 0000000000000000 <__clear_user>: 0: e8 00 00 00 00 callq 5 <__clear_user+0x5> 1: R_X86_64_PLT32 __fentry__-0x4 5: 90 nop 6: 90 nop 7: 90 nop 8: 48 89 f0 mov %rsi,%rax b: 48 c1 ee 03 shr $0x3,%rsi f: 83 e0 07 and $0x7,%eax 12: 48 89 f1 mov %rsi,%rcx 15: 48 85 c9 test %rcx,%rcx 18: 74 0f je 29 <__clear_user+0x29> 1a: 48 c7 07 00 00 00 00 movq $0x0,(%rdi) 21: 48 83 c7 08 add $0x8,%rdi 25: ff c9 dec %ecx 27: 75 f1 jne 1a <__clear_user+0x1a> 29: 48 89 c1 mov %rax,%rcx 2c: 85 c9 test %ecx,%ecx 2e: 74 0a je 3a <__clear_user+0x3a> 30: c6 07 00 movb $0x0,(%rdi) 33: 48 ff c7 inc %rdi 36: ff c9 dec %ecx 38: 75 f6 jne 30 <__clear_user+0x30> 3a: 90 nop 3b: 90 nop 3c: 90 nop 3d: 48 89 c8 mov %rcx,%rax 40: c3 retq Seems correct. Not sure you want to go fix that though. Let me know if you want more output.
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h index 43c029cdc3fe..cd31e4433f4c 100644 --- a/arch/x86/include/asm/special_insns.h +++ b/arch/x86/include/asm/special_insns.h @@ -5,6 +5,7 @@ #ifdef __KERNEL__ +#include <linux/frame.h> #include <asm/nops.h> /* @@ -135,6 +136,7 @@ static inline void native_wbinvd(void) } extern asmlinkage void native_load_gs_index(unsigned); +AC_SAFE(native_load_gs_index); static inline unsigned long __read_cr4(void) { diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c index 4b8ee05dd6ad..e278b4055a8b 100644 --- a/arch/x86/kernel/ptrace.c +++ b/arch/x86/kernel/ptrace.c @@ -420,7 +420,7 @@ static int putreg(struct task_struct *child, return 0; } -static unsigned long getreg(struct task_struct *task, unsigned long offset) +static notrace unsigned long getreg(struct task_struct *task, unsigned long offset) { switch (offset) { case offsetof(struct user_regs_struct, cs): @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset) return *pt_regs_access(task_pt_regs(task), offset); } +AC_SAFE(getreg); static int genregs_get(struct task_struct *target, const struct user_regset *regset, diff --git a/include/linux/frame.h b/include/linux/frame.h index 02d3ca2d9598..5d354cf42a56 100644 --- a/include/linux/frame.h +++ b/include/linux/frame.h @@ -21,4 +21,27 @@ #endif /* CONFIG_STACK_VALIDATION */ +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) && !defined(BUILD_VDSO32) +/* + * This macro marks functions as AC-safe, that is, it is safe to call from an + * EFLAGS.AC enabled region (typically user_access_begin() / + * user_access_end()). + * + * These functions in turn will only call AC-safe functions themselves (which + * precludes tracing, including __fentry__ and scheduling, including + * preempt_enable). + * + * AC-safe functions will obviously also not change EFLAGS.AC themselves. + * + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds + * (and the generated symbol reference will in fact cause link failures). + */ +#define AC_SAFE(func) \ + static void __used __section(.discard.ac_safe) \ + *__func_ac_safe_##func = func + +#else +#define AC_SAFE(func) +#endif + #endif /* _LINUX_FRAME_H */ diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index b0d7dc3d71b5..48327099466d 100644 --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -33,7 +33,9 @@ #define INSN_STACK 8 #define INSN_BUG 9 #define INSN_NOP 10 -#define INSN_OTHER 11 +#define INSN_STAC 11 +#define INSN_CLAC 12 +#define INSN_OTHER 13 #define INSN_LAST INSN_OTHER enum op_dest_type { diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index 540a209b78ab..d1e99d1460a5 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, case 0x0f: - if (op2 >= 0x80 && op2 <= 0x8f) { + if (op2 == 0x01) { + + if (modrm == 0xca) { + + *type = INSN_CLAC; + + } else if (modrm == 0xcb) { + + *type = INSN_STAC; + + } + + } else if (op2 >= 0x80 && op2 <= 0x8f) { *type = INSN_JUMP_CONDITIONAL; diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 0414a0d52262..01852602ca31 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -127,6 +127,24 @@ static bool ignore_func(struct objtool_file *file, struct symbol *func) return false; } +static bool ac_safe_func(struct objtool_file *file, struct symbol *func) +{ + struct rela *rela; + + /* check for AC_SAFE */ + if (file->ac_safe && file->ac_safe->rela) + list_for_each_entry(rela, &file->ac_safe->rela->rela_list, list) { + if (rela->sym->type == STT_SECTION && + rela->sym->sec == func->sec && + rela->addend == func->offset) + return true; + if (/* rela->sym->type == STT_FUNC && */ rela->sym == func) + return true; + } + + return false; +} + /* * This checks to see if the given function is a "noreturn" function. * @@ -439,6 +457,8 @@ static void add_ignores(struct objtool_file *file) for_each_sec(file, sec) { list_for_each_entry(func, &sec->symbol_list, list) { + func->ac_safe = ac_safe_func(file, func); + if (func->type != STT_FUNC) continue; @@ -1902,6 +1922,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, switch (insn->type) { case INSN_RETURN: + if (state.ac) { + WARN_FUNC("return with AC set", sec, insn->offset); + return 1; + } + if (func && has_modified_stack_frame(&state)) { WARN_FUNC("return with modified stack frame", sec, insn->offset); @@ -1917,6 +1942,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, return 0; case INSN_CALL: + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { + WARN_FUNC("call to %s() with AC set", sec, insn->offset, + insn->call_dest->name); + return 1; + } + if (is_fentry_call(insn)) break; @@ -1928,6 +1959,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, /* fallthrough */ case INSN_CALL_DYNAMIC: + if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) { + WARN_FUNC("call to %s() with AC set", sec, insn->offset, + insn->call_dest->name); + return 1; + } if (!no_fp && func && !has_valid_stack_frame(&state)) { WARN_FUNC("call without frame pointer save/setup", sec, insn->offset); @@ -1980,6 +2016,26 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, break; + case INSN_STAC: + if (state.ac_safe || state.ac) { + WARN_FUNC("recursive STAC", sec, insn->offset); + return 1; + } + state.ac = true; + break; + + case INSN_CLAC: + if (!state.ac) { + WARN_FUNC("redundant CLAC", sec, insn->offset); + return 1; + } + if (state.ac_safe) { + WARN_FUNC("AC-safe clears AC", sec, insn->offset); + return 1; + } + state.ac = false; + break; + default: break; } @@ -2141,6 +2197,8 @@ static int validate_functions(struct objtool_file *file) if (!insn || insn->ignore) continue; + state.ac_safe = func->ac_safe; + ret = validate_branch(file, insn, state); warnings += ret; } @@ -2198,6 +2256,7 @@ int check(const char *_objname, bool orc) INIT_LIST_HEAD(&file.insn_list); hash_init(file.insn_hash); file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard"); + file.ac_safe = find_section_by_name(file.elf, ".discard.ac_safe"); file.c_file = find_section_by_name(file.elf, ".comment"); file.ignore_unreachables = no_unreachable; file.hints = false; diff --git a/tools/objtool/check.h b/tools/objtool/check.h index e6e8a655b556..c31ec3ca78f3 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -31,7 +31,7 @@ struct insn_state { int stack_size; unsigned char type; bool bp_scratch; - bool drap, end; + bool drap, end, ac, ac_safe; int drap_reg, drap_offset; struct cfi_reg vals[CFI_NUM_REGS]; }; @@ -61,6 +61,7 @@ struct objtool_file { struct list_head insn_list; DECLARE_HASHTABLE(insn_hash, 16); struct section *whitelist; + struct section *ac_safe; bool ignore_unreachables, c_file, hints, rodata; }; diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h index bc97ed86b9cd..064c3df31e40 100644 --- a/tools/objtool/elf.h +++ b/tools/objtool/elf.h @@ -62,6 +62,7 @@ struct symbol { unsigned long offset; unsigned int len; struct symbol *pfunc, *cfunc; + bool ac_safe; }; struct rela {