Re: [PATCH 18/20] objtool: Add UACCESS validation

[Josh Poimboeuf]

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