Messages in this thread | | | Date | Fri, 8 Mar 2019 15:02:09 -0600 | From | Josh Poimboeuf <> | Subject | Re: [PATCH 18/20] objtool: Add UACCESS validation |
| |
On Thu, Mar 07, 2019 at 12:45:29PM +0100, Peter Zijlstra wrote: > --- a/tools/objtool/arch/x86/decode.c > +++ b/tools/objtool/arch/x86/decode.c > @@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf * > > case 0x0f: > > - if (op2 >= 0x80 && op2 <= 0x8f) { > + if (op2 == 0x01) { > + > + if (modrm == 0xca) { > + > + *type = INSN_CLAC; > + > + } else if (modrm == 0xcb) { > + > + *type = INSN_STAC; > + > + }
Style nit, no need for all those brackets and newlines.
> --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -442,6 +442,81 @@ static void add_ignores(struct objtool_f > } > } > > +static const char *uaccess_safe_builtin[] = { > + /* KASAN */
A short comment would be good here, something describing why a function might be added to the list.
> + "memset_orig", /* XXX why not memset_erms */ > + "__memset", > + "kasan_poison_shadow", > + "kasan_unpoison_shadow", > + "__asan_poison_stack_memory", > + "__asan_unpoison_stack_memory", > + "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;
This won't work if the function name changes due to IPA optimizations. I assume these are all global functions so maybe it's fine?
> @@ -1914,6 +2008,16 @@ static int validate_branch(struct objtoo > 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); > @@ -1929,17 +2033,32 @@ static int validate_branch(struct objtoo > return 0; > > case INSN_CALL: > - if (is_fentry_call(insn)) > - break; > + case INSN_CALL_DYNAMIC: > +do_call: > + if (state.uaccess && !func_uaccess_safe(insn->call_dest)) { > + WARN_FUNC("call to %s() with UACCESS enabled", > + sec, insn->offset, insn_dest_name(insn)); > + return 1; > + } > > - ret = dead_end_function(file, insn->call_dest); > - if (ret == 1) > + if (insn->type == INSN_JUMP_UNCONDITIONAL || > + insn->type == INSN_JUMP_DYNAMIC) > return 0; > - if (ret == -1) > - return 1; > > - /* fallthrough */ > - case INSN_CALL_DYNAMIC: > + if (insn->type == INSN_JUMP_CONDITIONAL) > + break; > + > + 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; > + } > + > if (!no_fp && func && !has_valid_stack_frame(&state)) { > WARN_FUNC("call without frame pointer save/setup", > sec, insn->offset); > @@ -1956,6 +2075,8 @@ static int validate_branch(struct objtoo > sec, insn->offset); > return 1; > } > + goto do_call; > +
These gotos make my head spin. Again I would much prefer a small amount of code duplication over this.
> +++ b/tools/objtool/special.c > @@ -42,6 +42,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; > @@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf > * It has been requested that we don't validate the !POPCNT > * feature path which is a "very very small percentage of > * machines". > + * > + * Also, unconditionally enable SMAP; this avoids seeing paths > + * that pass through the STAC alternative and through the CLAC > + * NOPs.
Why is this a problem?
> + * > + * XXX: We could do this for all binary NOP/single-INSN > + * alternatives.
Same question here.
> */ > - if (feature == X86_FEATURE_POPCNT) > + if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP) > alt->skip_orig = true; > } > > >
-- Josh
| |