From ff05ab2305aaeb21a3002ae95a17e176c198b71b Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Mon, 18 Mar 2019 14:33:07 +0100 Subject: x86/nospec, objtool: Introduce ANNOTATE_IGNORE_ALTERNATIVE To facillitate other usage of ignoring alternatives; rename ANNOTATE_NOSPEC_IGNORE to ANNOTATE_IGNORE_ALTERNATIVE. Signed-off-by: Peter Zijlstra (Intel) Cc: Borislav Petkov Cc: Josh Poimboeuf Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/check.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 5dde107083c6..110ea3d84772 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -457,13 +457,13 @@ static void add_ignores(struct objtool_file *file) * But it at least allows objtool to understand the control flow *around* the * retpoline. */ -static int add_nospec_ignores(struct objtool_file *file) +static int add_ignore_alternatives(struct objtool_file *file) { struct section *sec; struct rela *rela; struct instruction *insn; - sec = find_section_by_name(file->elf, ".rela.discard.nospec"); + sec = find_section_by_name(file->elf, ".rela.discard.ignore_alts"); if (!sec) return 0; @@ -475,7 +475,7 @@ static int add_nospec_ignores(struct objtool_file *file) insn = find_insn(file, rela->sym->sec, rela->addend); if (!insn) { - WARN("bad .discard.nospec entry"); + WARN("bad .discard.ignore_alts entry"); return -1; } @@ -1239,7 +1239,7 @@ static int decode_sections(struct objtool_file *file) add_ignores(file); - ret = add_nospec_ignores(file); + ret = add_ignore_alternatives(file); if (ret) return ret; -- cgit v1.2.3 From a4d09dde9093a04a9b48fb9e5ef3177bdfaff199 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Mon, 25 Feb 2019 10:31:24 +0100 Subject: objtool: Set insn->func for alternatives In preparation of function attributes, we need each instruction to have a valid link back to its function. Therefore make sure we set the function association for alternative instruction sequences; they are, after all, still part of the function. Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/check.c | 1 + 1 file changed, 1 insertion(+) (limited to 'tools/objtool') diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 110ea3d84772..950d0f62d22b 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -695,6 +695,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) -- cgit v1.2.3 From 09f30d83d33029faf6377a86f5ae80a658af9254 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Thu, 28 Feb 2019 14:17:50 +0100 Subject: objtool: Handle function aliases Function aliases result in different symbols for the same set of instructions; track a canonical symbol so there is a unique point of access. This again prepares the way for function attributes. And in particular the need for aliases comes from how KASAN uses them. Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/elf.c | 15 +++++++++++---- tools/objtool/elf.h | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c index b8f3cca8e58b..dd198d53387d 100644 --- a/tools/objtool/elf.c +++ b/tools/objtool/elf.c @@ -219,7 +219,7 @@ static int read_sections(struct elf *elf) static int read_symbols(struct elf *elf) { struct section *symtab, *sec; - struct symbol *sym, *pfunc; + struct symbol *sym, *pfunc, *alias; struct list_head *entry, *tmp; int symbols_nr, i; char *coldstr; @@ -239,6 +239,7 @@ static int read_symbols(struct elf *elf) return -1; } memset(sym, 0, sizeof(*sym)); + alias = sym; sym->idx = i; @@ -288,11 +289,17 @@ static int read_symbols(struct elf *elf) break; } - if (sym->offset == s->offset && sym->len >= s->len) { - entry = tmp; - break; + if (sym->offset == s->offset) { + if (sym->len == s->len && alias == sym) + alias = s; + + if (sym->len >= s->len) { + entry = tmp; + break; + } } } + sym->alias = alias; list_add(&sym->list, entry); hash_add(sym->sec->symbol_hash, &sym->hash, sym->idx); } diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h index bc97ed86b9cd..968265b4b4cd 100644 --- a/tools/objtool/elf.h +++ b/tools/objtool/elf.h @@ -61,7 +61,7 @@ struct symbol { unsigned char bind, type; unsigned long offset; unsigned int len; - struct symbol *pfunc, *cfunc; + struct symbol *pfunc, *cfunc, *alias; }; struct rela { -- cgit v1.2.3 From aaf5c623b915d64beba676b8c2e9708d1fda94d6 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Wed, 27 Feb 2019 14:04:13 +0100 Subject: objtool: Rewrite add_ignores() The whole add_ignores() thing was wildly weird; rewrite it according to 'modern' ways. Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/check.c | 51 ++++++++++++++++++++------------------------------- tools/objtool/check.h | 1 - 2 files changed, 20 insertions(+), 32 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 950d0f62d22b..8d8191f25381 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -104,29 +104,6 @@ static struct instruction *next_insn_same_func(struct objtool_file *file, for (insn = next_insn_same_sec(file, insn); insn; \ insn = next_insn_same_sec(file, insn)) -/* - * Check if the function has been manually whitelisted with the - * STACK_FRAME_NON_STANDARD macro, or if it should be automatically whitelisted - * due to its use of a context switching instruction. - */ -static bool ignore_func(struct objtool_file *file, struct symbol *func) -{ - struct rela *rela; - - /* check for STACK_FRAME_NON_STANDARD */ - if (file->whitelist && file->whitelist->rela) - list_for_each_entry(rela, &file->whitelist->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. * @@ -436,18 +413,31 @@ static void add_ignores(struct objtool_file *file) struct instruction *insn; struct section *sec; struct symbol *func; + struct rela *rela; - for_each_sec(file, sec) { - list_for_each_entry(func, &sec->symbol_list, list) { - if (func->type != STT_FUNC) - continue; + sec = find_section_by_name(file->elf, ".rela.discard.func_stack_frame_non_standard"); + if (!sec) + return; - if (!ignore_func(file, func)) + list_for_each_entry(rela, &sec->rela_list, list) { + switch (rela->sym->type) { + case STT_FUNC: + func = rela->sym; + break; + + case STT_SECTION: + func = find_symbol_by_offset(rela->sym->sec, rela->addend); + if (!func || func->type != STT_FUNC) continue; + break; - func_for_each_insn_all(file, func, insn) - insn->ignore = true; + default: + WARN("unexpected relocation symbol type in %s: %d", sec->name, rela->sym->type); + continue; } + + func_for_each_insn_all(file, func, insn) + insn->ignore = true; } } @@ -2199,7 +2189,6 @@ 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.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..d8896eb43521 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -60,7 +60,6 @@ struct objtool_file { struct elf *elf; struct list_head insn_list; DECLARE_HASHTABLE(insn_hash, 16); - struct section *whitelist; bool ignore_unreachables, c_file, hints, rodata; }; -- cgit v1.2.3 From 7697eee3ddd768a1fd78c1e687afaa6c5aa5072d Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Fri, 1 Mar 2019 11:15:49 +0100 Subject: objtool: Add --backtrace support For when you want to know the path that reached your fail state: $ ./objtool check --no-fp --backtrace arch/x86/lib/usercopy_64.o arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0x3: UACCESS disable without MEMOPs: __clear_user() arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x3a: (alt) arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x2e: (branch) arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x18: (branch) arch/x86/lib/usercopy_64.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch) arch/x86/lib/usercopy_64.o: warning: objtool: __clear_user()+0x5: (alt) 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 Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/builtin-check.c | 3 ++- tools/objtool/builtin.h | 2 +- tools/objtool/check.c | 18 ++++++++++++++---- tools/objtool/warn.h | 8 ++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index 694abc628e9b..99f10c585cbe 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -29,7 +29,7 @@ #include "builtin.h" #include "check.h" -bool no_fp, no_unreachable, retpoline, module; +bool no_fp, no_unreachable, retpoline, module, backtrace; static const char * const check_usage[] = { "objtool check [] file.o", @@ -41,6 +41,7 @@ const struct option check_options[] = { OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"), OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"), OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"), + OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"), OPT_END(), }; diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h index 28ff40e19a14..65fd3cc3c98b 100644 --- a/tools/objtool/builtin.h +++ b/tools/objtool/builtin.h @@ -20,7 +20,7 @@ #include extern const struct option check_options[]; -extern bool no_fp, no_unreachable, retpoline, module; +extern bool no_fp, no_unreachable, retpoline, module, backtrace; extern int cmd_check(int argc, const char **argv); extern int cmd_orc(int argc, const char **argv); diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 8d8191f25381..ccc66af5907f 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1885,8 +1885,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, if (!insn->ignore_alts) { list_for_each_entry(alt, &insn->alts, list) { ret = validate_branch(file, alt->insn, state); - if (ret) - return 1; + if (ret) { + if (backtrace) + BT_FUNC("(alt)", insn); + return ret; + } } } @@ -1933,8 +1936,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, insn->jump_dest->func->pfunc == func)) { ret = validate_branch(file, insn->jump_dest, state); - if (ret) - return 1; + if (ret) { + if (backtrace) + BT_FUNC("(branch)", insn); + return ret; + } } else if (func && has_modified_stack_frame(&state)) { WARN_FUNC("sibling call from callable instruction with modified stack frame", @@ -2005,6 +2011,8 @@ static int validate_unwind_hints(struct objtool_file *file) for_each_insn(file, insn) { if (insn->hint && !insn->visited) { ret = validate_branch(file, insn, state); + if (ret && backtrace) + BT_FUNC("<=== (hint)", insn); warnings += ret; } } @@ -2133,6 +2141,8 @@ static int validate_functions(struct objtool_file *file) continue; ret = validate_branch(file, insn, state); + if (ret && backtrace) + BT_FUNC("<=== (func)", insn); warnings += ret; } } diff --git a/tools/objtool/warn.h b/tools/objtool/warn.h index afd9f7a05f6d..f4fbb972b611 100644 --- a/tools/objtool/warn.h +++ b/tools/objtool/warn.h @@ -64,6 +64,14 @@ static inline char *offstr(struct section *sec, unsigned long offset) free(_str); \ }) +#define BT_FUNC(format, insn, ...) \ +({ \ + struct instruction *_insn = (insn); \ + char *_str = offstr(_insn->sec, _insn->offset); \ + WARN(" %s: " format, _str, ##__VA_ARGS__); \ + free(_str); \ +}) + #define WARN_ELF(format, ...) \ WARN(format ": %s", ##__VA_ARGS__, elf_errmsg(-1)) -- cgit v1.2.3 From 764eef4b109ae11e6c987de9c14fc7c482041be0 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Fri, 1 Mar 2019 11:19:03 +0100 Subject: objtool: Rewrite alt->skip_orig Really skip the original instruction flow, instead of letting it continue with NOPs. Since the alternative code flow already continues after the original instructions, only the alt-original is skipped. Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/check.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/check.c b/tools/objtool/check.c index ccc66af5907f..5264a305d658 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -31,6 +31,7 @@ struct alternative { struct list_head list; struct instruction *insn; + bool skip_orig; }; const char *objname; @@ -623,9 +624,6 @@ static int add_call_destinations(struct objtool_file *file) * conditionally jumps to the _end_ of the entry. We have to modify these * jumps' destinations to point back to .text rather than the end of the * entry in .altinstr_replacement. - * - * 4. It has been requested that we don't validate the !POPCNT feature path - * which is a "very very small percentage of machines". */ static int handle_group_alt(struct objtool_file *file, struct special_alt *special_alt, @@ -641,9 +639,6 @@ static int handle_group_alt(struct objtool_file *file, if (insn->offset >= special_alt->orig_off + special_alt->orig_len) break; - if (special_alt->skip_orig) - insn->type = INSN_NOP; - insn->alt_group = true; last_orig_insn = insn; } @@ -808,6 +803,7 @@ static int add_special_section_alts(struct objtool_file *file) } alt->insn = new_insn; + alt->skip_orig = special_alt->skip_orig; list_add_tail(&alt->list, &orig_insn->alts); list_del(&special_alt->list); @@ -1883,7 +1879,12 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, insn->visited = true; if (!insn->ignore_alts) { + bool skip_orig = false; + list_for_each_entry(alt, &insn->alts, list) { + if (alt->skip_orig) + skip_orig = true; + ret = validate_branch(file, alt->insn, state); if (ret) { if (backtrace) @@ -1891,6 +1892,9 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, return ret; } } + + if (skip_orig) + return 0; } switch (insn->type) { -- cgit v1.2.3 From 54262aa2830151f89699fa8a6c5aa05f0992e672 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Wed, 6 Mar 2019 12:58:15 +0100 Subject: objtool: Fix sibling call detection It turned out that we failed to detect some sibling calls; specifically those without relocation records; like: $ ./objdump-func.sh defconfig-build/mm/kasan/generic.o __asan_loadN 0000 0000000000000840 <__asan_loadN>: 0000 840: 48 8b 0c 24 mov (%rsp),%rcx 0004 844: 31 d2 xor %edx,%edx 0006 846: e9 45 fe ff ff jmpq 690 So extend the cross-function jump to also consider those that are not between known (or newly detected) parent/child functions, as sibling-cals when they jump to the start of the function. The second part of that condition is to deal with random jumps to the middle of other function, as can be found in arch/x86/lib/copy_user_64.S for example. This then (with later patches applied) makes the above recognise the sibling call: mm/kasan/generic.o: warning: objtool: __asan_loadN()+0x6: call to check_memory_region() with UACCESS enabled Also make sure to set insn->call_dest for sibling calls so we can know who we're calling. This is useful information when printing validation warnings later. Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/check.c | 86 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 31 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 5264a305d658..8118361295dd 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -515,7 +515,8 @@ static int add_jump_destinations(struct objtool_file *file) continue; } else { /* sibling call */ - insn->jump_dest = 0; + insn->call_dest = rela->sym; + insn->jump_dest = NULL; continue; } @@ -537,25 +538,38 @@ static int add_jump_destinations(struct objtool_file *file) } /* - * For GCC 8+, create parent/child links for any cold - * subfunctions. This is _mostly_ redundant with a similar - * initialization in read_symbols(). - * - * If a function has aliases, we want the *first* such function - * in the symbol table to be the subfunction's parent. In that - * case we overwrite the initialization done in read_symbols(). - * - * However this code can't completely replace the - * read_symbols() code because this doesn't detect the case - * where the parent function's only reference to a subfunction - * is through a switch table. + * Cross-function jump. */ if (insn->func && insn->jump_dest->func && - insn->func != insn->jump_dest->func && - !strstr(insn->func->name, ".cold.") && - strstr(insn->jump_dest->func->name, ".cold.")) { - insn->func->cfunc = insn->jump_dest->func; - insn->jump_dest->func->pfunc = insn->func; + insn->func != insn->jump_dest->func) { + + /* + * For GCC 8+, create parent/child links for any cold + * subfunctions. This is _mostly_ redundant with a + * similar initialization in read_symbols(). + * + * If a function has aliases, we want the *first* such + * function in the symbol table to be the subfunction's + * parent. In that case we overwrite the + * initialization done in read_symbols(). + * + * However this code can't completely replace the + * read_symbols() code because this doesn't detect the + * case where the parent function's only reference to a + * subfunction is through a switch table. + */ + if (!strstr(insn->func->name, ".cold.") && + strstr(insn->jump_dest->func->name, ".cold.")) { + insn->func->cfunc = insn->jump_dest->func; + insn->jump_dest->func->pfunc = insn->func; + + } else if (insn->jump_dest->func->pfunc != insn->func->pfunc && + insn->jump_dest->offset == insn->jump_dest->func->offset) { + + /* sibling class */ + insn->call_dest = insn->jump_dest->func; + insn->jump_dest = NULL; + } } } @@ -1785,6 +1799,17 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state) return false; } +static int validate_sibling_call(struct instruction *insn, struct insn_state *state) +{ + if (has_modified_stack_frame(state)) { + WARN_FUNC("sibling call from callable instruction with modified stack frame", + insn->sec, insn->offset); + return 1; + } + + return 0; +} + /* * Follow the branch starting at the given instruction, and recursively follow * any other branches (jumps). Meanwhile, track the frame pointer state at @@ -1935,9 +1960,14 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, case INSN_JUMP_CONDITIONAL: case INSN_JUMP_UNCONDITIONAL: - if (insn->jump_dest && - (!func || !insn->jump_dest->func || - insn->jump_dest->func->pfunc == func)) { + if (func && !insn->jump_dest) { + ret = validate_sibling_call(insn, &state); + if (ret) + return ret; + + } else if (insn->jump_dest && + (!func || !insn->jump_dest->func || + insn->jump_dest->func->pfunc == func)) { ret = validate_branch(file, insn->jump_dest, state); if (ret) { @@ -1945,11 +1975,6 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, BT_FUNC("(branch)", insn); return ret; } - - } else if (func && has_modified_stack_frame(&state)) { - WARN_FUNC("sibling call from callable instruction with modified stack frame", - sec, insn->offset); - return 1; } if (insn->type == INSN_JUMP_UNCONDITIONAL) @@ -1958,11 +1983,10 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, break; case INSN_JUMP_DYNAMIC: - if (func && list_empty(&insn->alts) && - has_modified_stack_frame(&state)) { - WARN_FUNC("sibling call from callable instruction with modified stack frame", - sec, insn->offset); - return 1; + if (func && list_empty(&insn->alts)) { + ret = validate_sibling_call(insn, &state); + if (ret) + return ret; } return 0; -- cgit v1.2.3 From ea24213d8088f9da73e1b6aadf7abd2435b70397 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Mon, 25 Feb 2019 12:50:09 +0100 Subject: objtool: Add UACCESS validation It is important that UACCESS regions are as small as possible; furthermore the UACCESS state is not scheduled, so doing anything that might directly call into the scheduler will cause random code to be ran with UACCESS enabled. Teach objtool too track UACCESS state and warn about any CALL made while UACCESS is enabled. This very much includes the __fentry__() and __preempt_schedule() calls. Note that exceptions _do_ save/restore the UACCESS state, and therefore they can drive preemption. This also means that all exception handlers must have an otherwise redundant UACCESS disable instruction; therefore ignore this warning for !STT_FUNC code (exception handlers are not normal functions). Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- scripts/Makefile.build | 3 + tools/objtool/arch.h | 6 +- tools/objtool/arch/x86/decode.c | 13 ++- tools/objtool/builtin-check.c | 3 +- tools/objtool/builtin.h | 2 +- tools/objtool/check.c | 197 +++++++++++++++++++++++++++++++++++++--- tools/objtool/check.h | 3 +- tools/objtool/elf.h | 1 + tools/objtool/special.c | 18 ++++ tools/objtool/special.h | 1 + 10 files changed, 226 insertions(+), 21 deletions(-) (limited to 'tools/objtool') diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 76ca30cc4791..0c5969fa795f 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -222,6 +222,9 @@ endif ifdef CONFIG_RETPOLINE objtool_args += --retpoline endif +ifdef CONFIG_X86_SMAP + objtool_args += --uaccess +endif # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index b0d7dc3d71b5..467c2fe798a9 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 { @@ -41,6 +43,7 @@ enum op_dest_type { OP_DEST_REG_INDIRECT, OP_DEST_MEM, OP_DEST_PUSH, + OP_DEST_PUSHF, OP_DEST_LEAVE, }; @@ -55,6 +58,7 @@ enum op_src_type { OP_SRC_REG_INDIRECT, OP_SRC_CONST, OP_SRC_POP, + OP_SRC_POPF, OP_SRC_ADD, OP_SRC_AND, }; diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index 540a209b78ab..ab20a96fee50 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -357,19 +357,26 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, /* pushf */ *type = INSN_STACK; op->src.type = OP_SRC_CONST; - op->dest.type = OP_DEST_PUSH; + op->dest.type = OP_DEST_PUSHF; break; case 0x9d: /* popf */ *type = INSN_STACK; - op->src.type = OP_SRC_POP; + op->src.type = OP_SRC_POPF; op->dest.type = OP_DEST_MEM; break; 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/builtin-check.c b/tools/objtool/builtin-check.c index 99f10c585cbe..f3b378126011 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -29,7 +29,7 @@ #include "builtin.h" #include "check.h" -bool no_fp, no_unreachable, retpoline, module, backtrace; +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess; static const char * const check_usage[] = { "objtool check [] file.o", @@ -42,6 +42,7 @@ const struct option check_options[] = { OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"), OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"), OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"), + OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"), OPT_END(), }; diff --git a/tools/objtool/builtin.h b/tools/objtool/builtin.h index 65fd3cc3c98b..69762f9c5602 100644 --- a/tools/objtool/builtin.h +++ b/tools/objtool/builtin.h @@ -20,7 +20,7 @@ #include extern const struct option check_options[]; -extern bool no_fp, no_unreachable, retpoline, module, backtrace; +extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess; extern int cmd_check(int argc, const char **argv); extern int cmd_orc(int argc, const char **argv); diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 8118361295dd..965e954e07f4 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -442,6 +442,82 @@ static void add_ignores(struct objtool_file *file) } } +/* + * This is a whitelist of functions that is allowed to be called with AC set. + * The list is meant to be minimal and only contains compiler instrumentation + * ABI and a few functions used to implement *_{to,from}_user() functions. + * + * These functions must not directly change AC, but may PUSHF/POPF. + */ +static const char *uaccess_safe_builtin[] = { + /* KASAN */ + "kasan_report", + "check_memory_region", + /* KASAN out-of-line */ + "__asan_loadN_noabort", + "__asan_load1_noabort", + "__asan_load2_noabort", + "__asan_load4_noabort", + "__asan_load8_noabort", + "__asan_load16_noabort", + "__asan_storeN_noabort", + "__asan_store1_noabort", + "__asan_store2_noabort", + "__asan_store4_noabort", + "__asan_store8_noabort", + "__asan_store16_noabort", + /* KASAN in-line */ + "__asan_report_load_n_noabort", + "__asan_report_load1_noabort", + "__asan_report_load2_noabort", + "__asan_report_load4_noabort", + "__asan_report_load8_noabort", + "__asan_report_load16_noabort", + "__asan_report_store_n_noabort", + "__asan_report_store1_noabort", + "__asan_report_store2_noabort", + "__asan_report_store4_noabort", + "__asan_report_store8_noabort", + "__asan_report_store16_noabort", + /* KCOV */ + "write_comp_data", + "__sanitizer_cov_trace_pc", + "__sanitizer_cov_trace_const_cmp1", + "__sanitizer_cov_trace_const_cmp2", + "__sanitizer_cov_trace_const_cmp4", + "__sanitizer_cov_trace_const_cmp8", + "__sanitizer_cov_trace_cmp1", + "__sanitizer_cov_trace_cmp2", + "__sanitizer_cov_trace_cmp4", + "__sanitizer_cov_trace_cmp8", + /* UBSAN */ + "ubsan_type_mismatch_common", + "__ubsan_handle_type_mismatch", + "__ubsan_handle_type_mismatch_v1", + /* misc */ + "csum_partial_copy_generic", + "__memcpy_mcsafe", + "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */ + NULL +}; + +static void add_uaccess_safe(struct objtool_file *file) +{ + struct symbol *func; + const char **name; + + if (!uaccess) + return; + + for (name = uaccess_safe_builtin; *name; name++) { + func = find_symbol_by_name(file->elf, *name); + if (!func) + continue; + + func->alias->uaccess_safe = true; + } +} + /* * 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. @@ -818,6 +894,7 @@ static int add_special_section_alts(struct objtool_file *file) alt->insn = new_insn; alt->skip_orig = special_alt->skip_orig; + orig_insn->ignore_alts |= special_alt->skip_alt; list_add_tail(&alt->list, &orig_insn->alts); list_del(&special_alt->list); @@ -1239,6 +1316,7 @@ static int decode_sections(struct objtool_file *file) return ret; add_ignores(file); + add_uaccess_safe(file); ret = add_ignore_alternatives(file); if (ret) @@ -1320,11 +1398,11 @@ static int update_insn_state_regs(struct instruction *insn, struct insn_state *s return 0; /* push */ - if (op->dest.type == OP_DEST_PUSH) + if (op->dest.type == OP_DEST_PUSH || op->dest.type == OP_DEST_PUSHF) cfa->offset += 8; /* pop */ - if (op->src.type == OP_SRC_POP) + if (op->src.type == OP_SRC_POP || op->src.type == OP_SRC_POPF) cfa->offset -= 8; /* add immediate to sp */ @@ -1581,6 +1659,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state) break; case OP_SRC_POP: + case OP_SRC_POPF: if (!state->drap && op->dest.type == OP_DEST_REG && op->dest.reg == cfa->base) { @@ -1645,6 +1724,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state) break; case OP_DEST_PUSH: + case OP_DEST_PUSHF: state->stack_size += 8; if (cfa->base == CFI_SP) cfa->offset += 8; @@ -1735,7 +1815,7 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state) break; case OP_DEST_MEM: - if (op->src.type != OP_SRC_POP) { + if (op->src.type != OP_SRC_POP && op->src.type != OP_SRC_POPF) { WARN_FUNC("unknown stack-related memory operation", insn->sec, insn->offset); return -1; @@ -1799,6 +1879,33 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state) return false; } +static inline bool func_uaccess_safe(struct symbol *func) +{ + if (func) + return func->alias->uaccess_safe; + + return false; +} + +static inline const char *insn_dest_name(struct instruction *insn) +{ + if (insn->call_dest) + return insn->call_dest->name; + + return "{dynamic}"; +} + +static int validate_call(struct instruction *insn, struct insn_state *state) +{ + if (state->uaccess && !func_uaccess_safe(insn->call_dest)) { + WARN_FUNC("call to %s() with UACCESS enabled", + insn->sec, insn->offset, insn_dest_name(insn)); + return 1; + } + + return 0; +} + static int validate_sibling_call(struct instruction *insn, struct insn_state *state) { if (has_modified_stack_frame(state)) { @@ -1807,7 +1914,7 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st return 1; } - return 0; + return validate_call(insn, state); } /* @@ -1855,7 +1962,9 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, if (!insn->hint && !insn_state_match(insn, &state)) return 1; - return 0; + /* If we were here with AC=0, but now have AC=1, go again */ + if (insn->state.uaccess || !state.uaccess) + return 0; } if (insn->hint) { @@ -1925,6 +2034,16 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, switch (insn->type) { case INSN_RETURN: + if (state.uaccess && !func_uaccess_safe(func)) { + WARN_FUNC("return with UACCESS enabled", sec, insn->offset); + return 1; + } + + if (!state.uaccess && func_uaccess_safe(func)) { + WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function", sec, insn->offset); + return 1; + } + if (func && has_modified_stack_frame(&state)) { WARN_FUNC("return with modified stack frame", sec, insn->offset); @@ -1940,17 +2059,22 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, return 0; case INSN_CALL: - if (is_fentry_call(insn)) - break; + case INSN_CALL_DYNAMIC: + ret = validate_call(insn, &state); + if (ret) + return ret; - ret = dead_end_function(file, insn->call_dest); - if (ret == 1) - return 0; - if (ret == -1) - return 1; + if (insn->type == INSN_CALL) { + if (is_fentry_call(insn)) + break; + + ret = dead_end_function(file, insn->call_dest); + if (ret == 1) + return 0; + if (ret == -1) + return 1; + } - /* fallthrough */ - case INSN_CALL_DYNAMIC: if (!no_fp && func && !has_valid_stack_frame(&state)) { WARN_FUNC("call without frame pointer save/setup", sec, insn->offset); @@ -2003,6 +2127,49 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, if (update_insn_state(insn, &state)) return 1; + if (insn->stack_op.dest.type == OP_DEST_PUSHF) { + if (!state.uaccess_stack) { + state.uaccess_stack = 1; + } else if (state.uaccess_stack >> 31) { + WARN_FUNC("PUSHF stack exhausted", sec, insn->offset); + return 1; + } + state.uaccess_stack <<= 1; + state.uaccess_stack |= state.uaccess; + } + + if (insn->stack_op.src.type == OP_SRC_POPF) { + if (state.uaccess_stack) { + state.uaccess = state.uaccess_stack & 1; + state.uaccess_stack >>= 1; + if (state.uaccess_stack == 1) + state.uaccess_stack = 0; + } + } + + break; + + case INSN_STAC: + if (state.uaccess) { + WARN_FUNC("recursive UACCESS enable", sec, insn->offset); + return 1; + } + + state.uaccess = true; + break; + + case INSN_CLAC: + if (!state.uaccess && insn->func) { + WARN_FUNC("redundant UACCESS disable", sec, insn->offset); + return 1; + } + + if (func_uaccess_safe(func) && !state.uaccess_stack) { + WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset); + return 1; + } + + state.uaccess = false; break; default: @@ -2168,6 +2335,8 @@ static int validate_functions(struct objtool_file *file) if (!insn || insn->ignore) continue; + state.uaccess = func->alias->uaccess_safe; + ret = validate_branch(file, insn, state); if (ret && backtrace) BT_FUNC("<=== (func)", insn); diff --git a/tools/objtool/check.h b/tools/objtool/check.h index d8896eb43521..78a95d06c165 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -31,7 +31,8 @@ struct insn_state { int stack_size; unsigned char type; bool bp_scratch; - bool drap, end; + bool drap, end, uaccess; + unsigned int uaccess_stack; 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 968265b4b4cd..2cc2ed49322d 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, *alias; + bool uaccess_safe; }; struct rela { diff --git a/tools/objtool/special.c b/tools/objtool/special.c index 50af4e1274b3..4e50563d87c6 100644 --- a/tools/objtool/special.c +++ b/tools/objtool/special.c @@ -23,6 +23,7 @@ #include #include +#include "builtin.h" #include "special.h" #include "warn.h" @@ -42,6 +43,7 @@ #define ALT_NEW_LEN_OFFSET 11 #define X86_FEATURE_POPCNT (4*32+23) +#define X86_FEATURE_SMAP (9*32+20) struct special_entry { const char *sec; @@ -110,6 +112,22 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry, */ if (feature == X86_FEATURE_POPCNT) alt->skip_orig = true; + + /* + * If UACCESS validation is enabled; force that alternative; + * otherwise force it the other way. + * + * What we want to avoid is having both the original and the + * alternative code flow at the same time, in that case we can + * find paths that see the STAC but take the NOP instead of + * CLAC and the other way around. + */ + if (feature == X86_FEATURE_SMAP) { + if (uaccess) + alt->skip_orig = true; + else + alt->skip_alt = true; + } } orig_rela = find_rela_by_dest(sec, offset + entry->orig); diff --git a/tools/objtool/special.h b/tools/objtool/special.h index fad1d092f679..d5c062e718ef 100644 --- a/tools/objtool/special.h +++ b/tools/objtool/special.h @@ -26,6 +26,7 @@ struct special_alt { bool group; bool skip_orig; + bool skip_alt; bool jump_or_nop; struct section *orig_sec; -- cgit v1.2.3 From 2f0f9e9ad7b3459c5c54ef2c03145a98e65dd158 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Mon, 25 Feb 2019 11:10:55 +0100 Subject: objtool: Add Direction Flag validation Having DF escape is BAD(tm). Linus; you suggested this one, but since DF really is only used from ASM and the failure case is fairly obvious, do we really need this? OTOH the patch is fairly small and simple, so let's just do this to demonstrate objtool's superior awesomeness. Suggested-by: Linus Torvalds Signed-off-by: Peter Zijlstra (Intel) Acked-by: Josh Poimboeuf Cc: Borislav Petkov Cc: Peter Zijlstra Cc: Thomas Gleixner Signed-off-by: Ingo Molnar --- tools/objtool/arch.h | 4 +++- tools/objtool/arch/x86/decode.c | 8 ++++++++ tools/objtool/check.c | 25 +++++++++++++++++++++++++ tools/objtool/check.h | 2 +- 4 files changed, 37 insertions(+), 2 deletions(-) (limited to 'tools/objtool') diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index 467c2fe798a9..7a111a77b7aa 100644 --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -35,7 +35,9 @@ #define INSN_NOP 10 #define INSN_STAC 11 #define INSN_CLAC 12 -#define INSN_OTHER 13 +#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 ab20a96fee50..472e991f6512 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -451,6 +451,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 965e954e07f4..38b0517dc49e 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1903,6 +1903,12 @@ static int validate_call(struct instruction *insn, struct insn_state *state) return 1; } + if (state->df) { + WARN_FUNC("call to %s() with DF set", + insn->sec, insn->offset, insn_dest_name(insn)); + return 1; + } + return 0; } @@ -2044,6 +2050,11 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, 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); @@ -2172,6 +2183,20 @@ static int validate_branch(struct objtool_file *file, struct instruction *first, state.uaccess = false; break; + case INSN_STD: + if (state.df) + WARN_FUNC("recursive STD", sec, insn->offset); + + state.df = true; + break; + + case INSN_CLD: + if (!state.df && insn->func) + WARN_FUNC("redundant CLD", sec, insn->offset); + + state.df = false; + break; + default: break; } diff --git a/tools/objtool/check.h b/tools/objtool/check.h index 78a95d06c165..71e54f97dbcd 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, uaccess; + bool drap, end, uaccess, df; unsigned int uaccess_stack; int drap_reg, drap_offset; struct cfi_reg vals[CFI_NUM_REGS]; -- cgit v1.2.3