Messages in this thread | | | Subject | Re: [RFC][PATCH] objtool: STAC/CLAC validation | From | Julien Thierry <> | Date | Mon, 25 Feb 2019 08:33:35 +0000 |
| |
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 { >
-- Julien Thierry
| |